-
Notifications
You must be signed in to change notification settings - Fork 7
Add wildcard imports #183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add wildcard imports #183
Changes from 10 commits
071d6db
0e146af
3232a88
ae56930
216457c
8fcb4a7
f4940e2
a1ec6c8
b758d95
abfb0e7
81e06f4
7d4bfd0
9e46adf
2a21505
1a46cd8
b58f43b
983259d
eb455ad
fa9e837
946d64d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -22,6 +22,10 @@ import my::geom::sphere::{ draw, default_radius as foobar }; | |||||
|
|
||||||
| // Imports a whole module. Use it with `bevy_ui::name` | ||||||
| import bevy_ui; | ||||||
|
|
||||||
| // Import all items from another module | ||||||
| import bevy::prelude::*; | ||||||
| import wgsl_test::expect::*; | ||||||
| ``` | ||||||
|
|
||||||
| These can then be used anywhere in the source code. | ||||||
|
|
@@ -74,6 +78,7 @@ import_relative: | |||||
| import_path_or_item: | ||||||
| | ident '::' (import_collection | import_path_or_item) | ||||||
| | ident ('as' ident)? | ||||||
| | '*' | ||||||
|
|
||||||
| import_collection: | ||||||
| | '{' (import_path_or_item) (',' (import_path_or_item))* ','? '}' | ||||||
|
|
@@ -91,6 +96,21 @@ An item import imports a single item. The item can be renamed with the `as` keyw | |||||
|
|
||||||
| An import collection imports multiple items, and allows for nested imports. | ||||||
|
|
||||||
| A wildcard import imports all top-level declarations from a module. Submodule names and submodule contents are not imported. | ||||||
|
|
||||||
| WESL also extends WGSL's `global_directive` rule with a `module_attribute` form (used by `@wildcardable`; see [Wildcard imports](#wildcard-imports)): | ||||||
|
|
||||||
| ```ebnf | ||||||
| global_directive: | ||||||
| | ... // existing WGSL forms | ||||||
| | module_attribute | ||||||
|
|
||||||
| module_attribute: | ||||||
| | '@' ident ';' | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this could be a general syntax for all future "module parameterization" features.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very much thinking that. e.g., WESL may grow to have:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will require a hacky condition in the wgsl-analyzer parser, but is doable.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for checking! So the LL parser convenience is nice but not critical:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For module attributes, we should just pick another keyword. Or cram it into a diagnostic, although that might be confusing? Or use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. had same comment. I suggest using a module-scope diagnostic directive.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I think Here's four syntax options. All of these would only be allowed at the top of the
We wouldn't want to use a keyword that's not WGSL-reserved, so just plain For user/lib module metadata syntax, I think we'd naturally want an import wgsl_play::play_version;And I think we'll eventually allow strings in annotations. So I'll write the just @ @wildcardable;
@play_version("0.47");
module @ module @wildcardable;
module @wildcardable @play_version("0.47");
module mixed @ module wildcardable;
module wildcardable @play_version("0.47");
module () module(wildcardable);
module(@play_version("0.47"));
I lean towards |
||||||
| ``` | ||||||
|
|
||||||
| The `@` prefix avoids conflict with WGSL identifiers. | ||||||
|
|
||||||
| ### Import resolution algorithm | ||||||
|
|
||||||
| To resolve the import, the recursive structure is flattened out. This means turning every `import_collection` into multiple separate imports, ending with the items. | ||||||
|
|
@@ -111,8 +131,9 @@ Then, one iterates over each segment from left to right, and looks it up one by | |||||
| 2. We take that as the "current module". | ||||||
| 3. We repeatedly look at the next segment. | ||||||
| 1. Item in current module: Take that item. We must be at the last segment, otherwise it's an error. | ||||||
| 2. (Else if re-exported or inline module in current module: We continue with that module.) | ||||||
| 3. Else go to `current module path/ident.wesl` | ||||||
| 2. Wildcard import: Take all items in the current module. | ||||||
| 3. (Else if re-exported or inline module in current module: We continue with that module.) | ||||||
| 4. Else go to `current module path/ident.wesl` | ||||||
| * File found: We take that file as the current module. | ||||||
| * File not found: We assume an empty module as the current module, and continue with that. | ||||||
| * (Re-exporting changes the path.) | ||||||
|
|
@@ -121,10 +142,7 @@ Then, one iterates over each segment from left to right, and looks it up one by | |||||
| To get an absolute path to a module, one follows the algorithm above. In step 1, one takes the known absolute path of the `super` module, or the package. | ||||||
| The absolute path of the `super` module is always known, since the first loaded WESL file must always be the root module, and children are only discovered from there. | ||||||
|
|
||||||
| Once the import has been resolved, the last segment, or its alias, is brought into scope. | ||||||
|
|
||||||
| The order of the scopes is "user declarations and imported items > package names > predeclared items". | ||||||
| This lets WGSL add more predeclared items without breaking existing WESL code. Package names can shadow predeclared items, but we recommend that authors avoid doing that. | ||||||
| Once the import has been resolved, the last segment, or its `as` alias, is brought into scope. | ||||||
|
|
||||||
|
|
||||||
| ### Example | ||||||
|
|
@@ -245,6 +263,160 @@ const b = a + 1; | |||||
|
|
||||||
| Basic linker implementations do not need to check for this. Generating broken code and letting the underlying shader compiler throw an error is fine. | ||||||
|
|
||||||
| ## Wildcard imports | ||||||
|
|
||||||
| Wildcard imports bring all items from another module into the importing module's | ||||||
| scope. | ||||||
|
|
||||||
| Users can wildcard import: | ||||||
| - from any other module in the local package. | ||||||
| - from any external library module where the library author has added a | ||||||
| `@wildcardable` directive. | ||||||
|
|
||||||
| Wildcard importing brings some stability risk when the imported module adds to | ||||||
| its API. The newly introduced names may conflict with other names in the | ||||||
|
k2d222 marked this conversation as resolved.
|
||||||
| importer's namespace (from local definitions and other imports), leading to | ||||||
| compiler warnings or errors. To help mitigate this risk, WESL provides a | ||||||
| `@wildcardable` annotation that library authors can place on modules that are | ||||||
| designed for wildcard importing. | ||||||
|
|
||||||
| Advanced users who wish to wildcard import from external modules not marked as | ||||||
| `@wildcardable` can do so by suppressing `wildcard_import` (see | ||||||
| [Suppressible diagnostics](#suppressible-diagnostics)). | ||||||
|
|
||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably start by saying something like "The following section applies when the current module has two or more wildcard imports. We specifically deal with the potential name clashes and corner cases. Users are not expected to run into this edge case. It exists to give us certain guarantees in the language itself. This will be useful, for example, to build semver checking tools."
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see any mention of this "two wilcards rule" any more. Is this comment outdated? |
||||||
| ```wesl | ||||||
| // wildcard import from a @wildcardable external | ||||||
| import bevy::prelude::*; | ||||||
| import wgsl_test::expect::*; | ||||||
|
|
||||||
| // wildcard import from within the local package | ||||||
| import package::utils::*; | ||||||
| import super::fun::*; | ||||||
| ``` | ||||||
|
|
||||||
| ### `@wildcardable` directive | ||||||
|
|
||||||
| Library authors mark modules they intend for library consumers to wildcard | ||||||
| import with `@wildcardable`: | ||||||
|
|
||||||
| ```wesl | ||||||
| // math.wesl (in a library) | ||||||
| @wildcardable; | ||||||
|
|
||||||
| fn dot2(a: vec2f, b: vec2f) -> f32 { return a.x*b.x + a.y*b.y; } | ||||||
| fn cross2(a: vec2f, b: vec2f) -> f32 { return a.x*b.y - a.y*b.x; } | ||||||
| ``` | ||||||
|
|
||||||
| `@wildcardable` is a module attribute (see [Grammar](#grammar)). It must | ||||||
| appear after any imports and before any global declarations, and applies only | ||||||
| to the module it appears in. | ||||||
|
|
||||||
| ### Recommendations for `@wildcardable` modules | ||||||
|
|
||||||
|
k2d222 marked this conversation as resolved.
|
||||||
| Because every name in a `@wildcardable` module is a potential collision in | ||||||
| importer code, library authors should curate these modules carefully. | ||||||
|
|
||||||
| **Add hesitantly.** Additions to a `@wildcardable` module are semver minor | ||||||
| version bumps but can break users who have local declarations or import other | ||||||
| `@wildcardable` modules. | ||||||
| - **Bundle** additions into a major release when one is upcoming. | ||||||
| - **Document** additions clearly in changelogs so downstream users debugging | ||||||
| unexpected name resolution can trace them. | ||||||
|
|
||||||
| **Compose with re-exports.** See [Re-exports](#re-exports) (TBD) to collect | ||||||
| items from other modules into a single `@wildcardable` module for user | ||||||
| convenience. | ||||||
|
|
||||||
| **Avoid generic names.** Prefer domain-specific names. Common names like | ||||||
| `Buffer`, `Config`, `Result`, `Vec`, etc. are more likely to collide with user | ||||||
| applications. | ||||||
|
|
||||||
| **Don't shadow WGSL builtins.** Names like `vec3`, `clamp`, `inverseSqrt` have | ||||||
| expected semantics that oughtn't be implicitly overridden with wildcards. | ||||||
| Similarly, avoid experimental Naga/Dawn/Safari builtins. Consumers of the | ||||||
| module will see a `builtin_shadow` warning at the import site. | ||||||
| - If a future WGSL update adds a conflicting builtin name, plan to update the | ||||||
| `@wildcardable` module to rename the conflicting item. | ||||||
|
|
||||||
| ### Library-to-library wildcard imports | ||||||
|
|
||||||
| Libraries that wildcard import from other libraries raise special concerns. If a | ||||||
| user's package manager chooses a newer version of the imported-from library, | ||||||
| the user may see a conflict in library code they don't expect to modify. | ||||||
|
|
||||||
| WESL library publishing tools address this by expanding wildcards to named | ||||||
| imports in the published version of the module: | ||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Hopefully we can easily implement this behaviour in wesl-rs. It might be a challenge, since Cargo defaults to "building from source on the end-user's machine". So there might be no pre-publish step without jumping through hoops.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact it doesn't have to actually be published with expanded wildcards, only processed as such. So yes, we can do that in wesl-rs because the wildcard resolver always knows if it comes from a user or library module.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, would that really work? I thought it wouldn't, because of this case:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @k2d222 I think you'd once floated a publisher-side packaging step for WESL cargo crates (rather than building consumer-side). Is that likely? I guess that would help here. if cargo will follow semver and install a rev on the consumer side different than the rev the prelude publisher used, then things could break (because wildcards can break even on semver minor updates). So somebody on the app time runs cargo update and sees an upstream wesl crate fail to compile.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I got confused / forgot about this issue. Yeah it sounds like it has to be a pre-publish process and that's currently not the case. I'd need to think about how to achieve this. |
||||||
|
|
||||||
| ```wesl | ||||||
| // source | ||||||
| import bevy::prelude::*; | ||||||
| ``` | ||||||
|
|
||||||
| ```wesl | ||||||
| // published artifact | ||||||
| import bevy::prelude::{Color, Mesh, Transform, /* snapshot at publish time */}; | ||||||
| ``` | ||||||
|
|
||||||
| ## Import errors and warnings | ||||||
|
|
||||||
| WESL emits errors for name collisions and for external wildcards from unmarked | ||||||
| modules, and warnings for shadowing that could surprise readers. Genuine | ||||||
| collisions cannot be suppressed; other diagnostics are suppressible via | ||||||
| `@diagnostic`. | ||||||
|
|
||||||
| | Situation | Behavior | | ||||||
| | --- | --- | | ||||||
| | Local declaration conflicts with named import | Error | | ||||||
| | Named import conflicts with named import | Error | | ||||||
| | Wildcard import conflicts with wildcard import (when name is referenced) | Error | | ||||||
| | Wildcard import from a non-`@wildcardable` external module | Error (`wildcard_import`); suppressible | | ||||||
| | Local declaration or named import shadows a wildcard-imported name | Warning (`wildcard_shadow`); suppressible | | ||||||
| | Wildcard import shadows a WGSL builtin | Warning (`builtin_shadow`); suppressible | | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be a bit clearer when it shows, namely, on imports.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pushed rev |
||||||
|
|
||||||
| When multiple wildcard imports are in scope, the same name may be exported by | ||||||
| more than one module: | ||||||
|
|
||||||
| ```wesl | ||||||
| import foo::*; // exports clashing_zap | ||||||
| import bar::*; // exports clashing_zap | ||||||
| ``` | ||||||
|
|
||||||
| If `foo::clashing_zap` and `bar::clashing_zap` re-export the same item, there's | ||||||
| no conflict. Otherwise the potential conflict is dormant unless `clashing_zap` | ||||||
| is referenced. | ||||||
|
|
||||||
| ### Suppressible diagnostics | ||||||
|
|
||||||
| - **`wildcard_import`** fires on a wildcard import from an external module not | ||||||
| marked `@wildcardable`. Suppress with `@diagnostic(off, wildcard_import)` on | ||||||
| the import statement to accept the upgrade risk that future versions of the | ||||||
| imported module may add conflicting names. | ||||||
|
|
||||||
| - **`wildcard_shadow`** fires when a local declaration or named import shadows a | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
where: on the local declaration I suppose?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good question. I think you're right.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. clarified in the PR |
||||||
| name brought in by a wildcard import. The local wins by precedence (see | ||||||
| [Scope precedence](#scope-precedence)). | ||||||
|
|
||||||
| - **`builtin_shadow`** fires when a wildcard import shadows a WGSL builtin such | ||||||
| as `vec3` or `clamp`. Suppress at the import site if the override is | ||||||
| intentional; the suppression itself documents documents to readers that the | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thx fixed. |
||||||
| builtins have changed semantics. | ||||||
|
|
||||||
| ## Scope precedence | ||||||
|
|
||||||
| When a name could resolve to items at multiple precedence levels, the | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In what situation does this precedence rule occur?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the last time this was brought up the counter was "what about associated items, like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed, just documenting it in the table. (Though see also @laundmo suggesting we make it a suppressible diagnostic later)
separable issue from this wildcard stuff?
Note that the proposal for now is to warn only
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pushed a rev moving the builtin_shadow warning to client side, per @laundmo discussions |
||||||
| highest-precedence one wins. The table above describes whether such a resolution | ||||||
| is silent, produces a warning, or produces an error. | ||||||
|
|
||||||
| 1. user declarations | ||||||
| 2. named imports (non-wildcard) | ||||||
| 3. wildcard-imported names | ||||||
| 4. package names | ||||||
| 5. predeclared items (WGSL builtins) | ||||||
|
|
||||||
| Predeclared items rank lowest so that future WGSL spec revisions can add new | ||||||
| builtins without breaking existing WESL code: any name already bound at a | ||||||
| higher level continues to resolve as before. | ||||||
|
|
||||||
| ## Directives | ||||||
| Under discussion, see: <https://github.com/wgsl-tooling-wg/wesl-spec/issues/71> | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest generalizing the example so it doesn't tie the feature to the
preludename. The directive (not the name) carries the meaning, e.g.: