Add wildcard imports

[stefnotch]

Fix #130
This adds a simple wildcard imports mechanism with safeguards to reduce the risk of libraries accidentally publishing breaking changes.

See also https://hackmd.io/@stefnotch/SyKtKk8uWx for some prior thoughts on this

[Arne-Berner]

This is very confusing to me. There was an ongoing discussion in issues. The hackmd was neither mentioned on discord nor in the issue. So it seems to me, that there is an internal design team consisting of two people that decides on this. This pr has not been merged yet, but it is weird to me, that the discussion switched to the hackmd. (Bevy uses hackmd too, but it often gets referenced in issues, discussions or on discord.)

It states that library authors don't expect adding new items to be a breaking change. But the issue discusses this. This premise does not seem as clear as it is stated here. Most people find this in rust to be a non issue in that discussion.

The whole "private vs public by default" discussion seems decided here.

It mostly took the original proposal in the discussion from the author and renamed ambient to prelude. This still seems to me like a "magic keyword" that would confuse me a lot as a library author and user. Especially since bevy uses it subtly different.

I couldn't find anything about this, but this PR raised the following question for me:
How is wesl organized and what is the workflow for decisions?

The PRs here might just be there for lose discussion, but it assumes some things on such a fundamental level that I'm not sure it is that. (Like private by default)

[stefnotch]

@Arne-Berner Most of the discussions happened on the WESL Discord servers or internally, since that is a fast way to get feedback on ideas. The hackmd note are thoughts that I wrote down after that. With that, this proposal here is one of the few designs that I'm reasonably confident in.

That said, this PR here is for discussion before I push through something unreasonable.

We are leaning towards having private by default. Could you point out the place where this proposal assumes that? If it does, then that is likely an oversight on my part.

Regarding

It states that library authors don't expect adding new items to be a breaking change.

The cargo documentation explicitly calls out this corner case, since it is unexpected. One of our goals for WESL is to allow for safe library evolution beyond the Rust ecosystem. This means taking into account what npm users expect of semver, and also designing a safe path for future languages that we'll add. Semver breaking changes depend on how and when the package manager will do patch upgrades.

It mostly took the original proposal in the discussion from the author and renamed ambient to prelude. This still seems to me like a "magic keyword" that would confuse me a lot as a library author and user. Especially since bevy uses it subtly different.

This really wasn't my intent. My intent was to surface the corner cases via diagnostics. The prelude case was purely added for convenience. I am happy to remove it or replace it with an opt-in for prelude-like modules.

Though, how does Bevy's different usage look like?

[Arne-Berner]

We are leaning towards having private by default. Could you point out the place where this proposal assumes that? If it does, then that is likely an oversight on my part.

I'm sorry for not being clear there. When I said "public by default vs private by default" I was talking about using "export" on a module, to make it available for users.

Though, how does Bevy's different usage look like?

Bevy uses prelude as a marker for utilities that are most likely needed. It is a helper for the "most important functions". Naming a crate prelude does not have any function, besides being a convenient function.
If I tried to write a wesl library myself and most people would use the import package::prelude::* syntax I would assume that wildcard imports are possible everywhere. (At least if I didn't read this proposal) There would be no indicator in the code to let me assume that "prelude" is a keyword in the language that helps me export a module. Especially if I'm coming from rust or bevy. Since I would assume that "prelude" was chosen in a way that it is used in Rust. (an implicit marker, not a functional part of the language)

I must have misunderstood

See also https://hackmd.io/@stefnotch/SyKtKk8uWx for some prior thoughts on this

English is not my first language. If you wrote "See also ... for my prior thoughts on this" I wouldn't have assumed that the hackmd was the official discussion threat that lead to this decision. Getting all subtleties in english sometimes is hard for me. I've been lurking in the discord and must have missed further discussion about this topic.
I asked about the workflow for decisions because in the issue thread it was stated that Internally, I think we found a new favorite scheme but we need to write it up.. I didn't understand that this meant the discord server in general.

[mighdoll]

Hi @Arne-Berner our project is younger and smaller than Bevy and we haven't polished a procedure about where we discuss. We're still small enough that we sometimes save issues for live meetings, rather than doing things async. Undoubtedly we'll grow more standard over time, especially if more folks want to engage! Here Stefan is trying to recall/refresh our earlier discussions, I'm sure we'll discuss/modify further before the final form lands in the spec. We aim for unanimity among the three core committers before we commit spec changes.

[k2d222]

@Arne-Berner First to answer this:

How is wesl organized and what is the workflow for decisions?

We always submit ideas to the community in the shape of Discord discussions, GitHub issues and GitHub PRs (in that order) before merging. So far the "leadership" has been rather natural, since there are three main contributors (@stefnotch, @mighdoll and me) and reaching consensus has always been possible. Though, since you're raising a valid concern, I'll open a separate issue for further discussion and link it here. EDIT: #184

---

Back on topic

I was strongly in favor of unrestricted wildcards. As you said, they are usually not an issue. But the proposal written by @stefnotch may give you the wrong idea, at a first glance, because it goes deep into the edge cases. To clarify this PR:

• you can always wildcard-import anything you like.
• ...however, if you have two wildcard-import in the same file, AND they come from external dependencies, you will have to silence a diagnostic -- otherwise, the compiler will let you know why with a nice error message.
• ...unless they are named prelude. In this case, changing the API of a prelude module must be a breaking change.

In other words, a large majority of users would just never meet these edge cases. And if they do, it's a compile-time diagnostic that can be silenced.

[Arne-Berner]

Back on topic

I was strongly in favor of unrestricted wildcards. As you said, they are usually not an issue. But the proposal written by @stefnotch may give you the wrong idea, at a first glance, because it goes deep into the edge cases. To clarify this PR:

• you can always wildcard-import anything you like.
• ...however, if you have two wildcard-import in the same file, AND they come from external dependencies, you will have to silence a diagnostic -- otherwise, the compiler will let you know why with a nice error message.
• ...unless they are named prelude. In this case, changing the API of a prelude module must be a breaking change.

In other words, a large majority of users would just never meet these edge cases. And if they do, it's a compile-time diagnostic that can be silenced.

No, that was very clear! Also I personally really like the proposal (except for the prelude addition). Probably that prelude decision was throwing me off track the most since there seemed to be some sound arguments in the issue about it.

I also think public vs private by default would change this proposal a lot. But that might just be me.

Using opt in diagnostics as a user feels very clever to me :)

[BenjaminBrienen]

I don't think stopping people from using glob imports makes sense beyond giving a configurable diagnostic. Triggering special behavior on using 2 glob imports also seems weird if the collision can already occur with only 1 (if a library adds a function that shadows a builtin)

[cart]

I find this current proposal slightly more agreeable than the previous one:

• It uses existing public/private semantics / keywords rather than introducing new ones (or at least, it doesn't explicitly define new things in this space, so I'm assuming this)
• It prescribes prelude (has existing uses in the wider language ecosystem) for the "wildcard" module name instead of ambient (a brand new word for this space). Given that in Bevy we want to call our wildcard modules prelude, this is nice.
• It gives users autonomy. They can wildcard import anything provided they suppress the diagnostic.

I still have concerns, but I'll create separate threads for them / respond to existing ones, so we can keep this organized:

Major concerns that I would personally block on:

• I strongly disagree that prelude should be special-cased here / be the only module that supports this. Looks like I either misread or was reviewing an older version. The current wording states that this will ultimately be configurable.

Minor concerns:

• I think wildcard imports on modules that aren't flagged as wildcard importable should emit warnings, not errors.
• I still think this approach introduces language complexity and user-facing pain / friction for nearly intangible benefits. I won't create a thread for this as I've already said my piece in the issue, and we're still here.

[cart]

I do think this proposal only really makes sense in the context of "private by default". Otherwise a bunch of new concerns are introduced (which I discussed in the issue).

[cart]

Given that code can still compile with this, and it only protects against potential / theoretical future breaking changes, I think it should be a warning. This feels like a "best practices enforcement thing" (akin to using the wrong naming conventions). Why break peoples' development flow for it?

[mighdoll]

Thanks @cart for clarifying which issues seem blocking for getting going with bevy usage/users. The language design issues tend to interrelate, and I would like to help make sure things are unblocked for when bevy is ready to use WESL.

On whether occasional library semver breakage is an acceptable cost for language simplicity wrt to wildcards, can you see the perspective of non-rust communities? Many language communities have chosen more restrictions rather than relying solely on community convention + occasional client cleanup to achieve safe wildcard use. I don't think it's fair to say that other communities reasoning is intangible or only theoretical. It's just a different choice on a spectrum of choices balancing stability vs user flexibility.

In practice for WebGPU/WESL, the js community expects semver to work and we embed in their library ecosystem. So we have to educate every new js user coming over to WESL and WebGPU libraries on rules for using wildcards safely. I want to use our linker/linter/ide tools to guide them in the right direction.

Personally, I managed engineering teams using unrestricted wildcards fruitfully for many years in Scala. I like using them just fine, even though I think one of these middle ground solutions is better for the WebGPU/WESL community.

If we can fully deliver cross platform WebGPU shader libraries, we'll have something very valuable for the community. That's a prize I like to keep an eye on. I don't like to compromise on simplicity goals, but I'm willing to do it here if it helps us get to compatible libraries.

[stefnotch]

I've pushed a new version of the proposal, but now that I've written out the longer diagnostics option, I'm not entirely convinced that it is the best solution.

The part about glob imports in libraries causing issues is preventable by making the globs explicit upon publishing. As in, if bevy has

import stefnotch_lib::*;
import evil_lib::*;

then when publishing bevy, we could expand the globs. (import stefnotch_lib::{foo, bar}; import evil_lib::{qux};)
In which case no matter what evil_lib does, it can't break bevy.

In user code, I don't think I care too much about a glob import that has just broken. It is an easy fix for me.

The part about accidentally shadowing a builtin seems impossible to fully prevent without #185

[Arne-Berner]

Using prelude still seems out of place in this context and very restricting to the naming scheme of library authors.

[Arne-Berner]

I've pushed a new version of the proposal, but now that I've written out the longer diagnostics option, I'm not entirely convinced that it is the best solution.

The part about glob imports in libraries causing issues is preventable by making the globs explicit upon publishing. As in, if bevy has

import stefnotch_lib::*;
import evil_lib::*;

then when publishing bevy, we could expand the globs. (import stefnotch_lib::{foo, bar}; import evil_lib::{qux};) In which case no matter what evil_lib does, it can't break bevy.

In user code, I don't think I care too much about a glob import that has just broken. It is an easy fix for me.

The part about accidentally shadowing a builtin seems impossible to fully prevent without #185

I'm not sure I understand this correctly, so I hope it's fine to ask:
What exactly do you mean by publishing bevy? Is this the same as building the package via build.rs or wesl package?
That would mean, it is not user facing code, correct? I would write something like

import stefnotch_lib::*;
import evil_lib::*;

And the user of bevy would not get to see this:

import stefnotch_lib::{foo, bar}; 
import evil_lib::{qux};

Unless they are looking at the definition of a bevy function they use in their library?
If so I was assuming that the proposal worked like that already and only libraries that were directly used by me would be affected by adding functions. If any node in the dependency tree would always publish all the wildcards imported I do see your problem of it being risky. The probability for such a conflict would be much higher than I thought.

[stefnotch]

@Arne-Berner
It's the same as wesl package. Rust doesn't have a clear cut equivalent, because build scripts are executed on the machine of whoever uses the library.

And yes, that is a correct description. The name clashes caused by wildcards can happen at any place in the dependency tree.

[mighdoll]

In user code, I don't think I care too much about a glob import that has just broken. It is an easy fix for me.

sure, for you it's easy :-).

But we can't expect that the user handling errors after an update is an aspiring WESL programmer sitting in their IDE who's read our blog post on how handle glob related errors.. In the js world, npm update, npm user expectations, npm dependency tools, etc. all treat patch/minor failures as upstream bugs. We can't hope that the npm ecosystem will change that expectation for WebGPU.

Imagine the user running npm (or cargo) update is not the shader programmer on their team. Or imagine a web dev team, updating html statement loading a shader library from a CDN (no package mgr, no IDE). Or imagine a bot trained to handle npm updates and classify upstream bugs. In all these cases, they're not well positioned (or expecting) to make shader fixes - they just want to get the security update or stay up to date or whatever.

AFAICT, we need patch/minor updates to succeed for default users. It's desirable anyway, but also a consequence of choosing to cross publish libraries to the npm ecosystem.

[Arne-Berner]

In user code, I don't think I care too much about a glob import that has just broken. It is an easy fix for me.

sure, for you it's easy :-).

But we can't expect that the user handling errors after an update is an aspiring WESL programmer sitting in their IDE who's read our blog post on how handle glob related errors.. In the js world, npm update, npm user expectations, npm dependency tools, etc. all treat patch/minor failures as upstream bugs. We can't hope that the npm ecosystem will change that expectation for WebGPU.

Isn't that in the context of using warn diagnostics when importing two libraries via wildcard? So someone would have to have used the "don't warn me about that" before it's relevant. And at that point it would mean that it's not an upstream library that has to change one wildcard import and you have to wait for them to fix your problem, but you can fix it yourself.

Imagine the user running npm (or cargo) update is not the shader programmer on their team. Or imagine a web dev team, updating html statement loading a shader library from a CDN (no package mgr, no IDE). Or imagine a bot trained to handle npm updates and classify upstream bugs. In all these cases, they're not well positioned (or expecting) to make shader fixes - they just want to get the security update or stay up to date or whatever.

I have only worked as a JS/TS dev for about halve a year, but if some package that is needed by the shader programmer breaks the whole library I think most people would pin that library and update the security issue in another library.
I do think that you are right and this is an issue that could be a problem for certain workflows. But I am not sure if it is a dealbreaker for most people instead of a compromise for incorporating a different programming language into js.
And wesl is another programming language than javascript and npm has not been optimized for wesl.

Lastly I would argue that people who get used to something like this:

[] + []        // ""
[] + {}        // "[object Object]"
{} + []        // 0   (in many consoles this is parsed as a block + unary +)
"5" - 1        // 4
"5" + 1        // "51"

Might tolerate a very rare breaking change on npm update for a pure wesl library that has a security patch, whilst the responsible developer is not there.

[stefnotch]

I am proposing that we re-evaluate the @!moduleAttribute() syntax. It would solve the pattern of "modules that exist conditionally". It is a common pattern in Rust and we currently are lacking a way of doing this in WESL. Here's an example of such a conditional module in Rust.

Here's a WESL example where it is important that it is only compiled on targets where there's support for f16. If we included this into the generated WGSL code on a target that doesn't have f16, then the entire shader would fail to compile

@!if(HAS_F16)

enable f16;
fn something_that_relies(on: f16) -> f16 {
}

This isn't doable with the module syntax.

@if(HAS_F16)
module; // this would just make the module keyword disappear

And the other reason is consistency with doc comments.

//! This is a comment for the current module. Notice the !
@!wildcardable // and this is a module level attribute, notice the !

/// This is a comment for function foo
@compute // and this is a normal attribute
fn foo()

[k2d222]

You convinced me. It works well in rust, and it seems to be the best ways of achieving "conditional modules". also, the module; directive is a bit ackward on its own, since it does nothing without attributes.

[mighdoll]

@! works for me. I'll update the PR.

I imagine we keep the placement of @! below the imports, so custom attributes like @!play_version("0.47") can use imported short names.

We give up on "every attribute is just @foo" consistency for dual module-or-declaration level attributes noted in #186. But teaching users that @foo before a module statement affects the whole module would have been confusing.

@!if conditional-module semantics are interesting, but let's defer. I think conditional imports plus lazy linking already cover most of the cases. And once you're swapping implementations you'll want an interface system to keep the alternatives aligned, and that may influence the syntax. But @! gives us the choice to decide later.

As a bonus, @! keeps the module keyword even more free, e.g., for a module { ... } block for module parameters off if we dip a little more into the ML language style for @param const (#164) and friends (as noted earlier).

[stefnotch]

I thought about this a bit more and there's a simpler rule:

Adding items to a wildcardable module is a semver breaking change. Publish accordingly.

We just have to note that and then we can get rid of this section. We can add this special trick later if we want.

[mighdoll]

The problem is that user communities reject having every add to an API be a major semver release. It causes too much churn. Theoretically, we could be the first lang community to convince our users differently, seems righteous. But that's probably not where we'd want to spend our limited teaching-users-new-things tokens.

[stefnotch]

For what it's worth, Cargo says that wildcards are just minor changes but that they can be breaking changes. And then discourages wildcard imports

Glob imports of items from external crates should be avoided.
https://doc.rust-lang.org/cargo/reference/semver.html#item-new

I'm assuming that that only applies to libraries.

[stefnotch]

One problem is that I don't yet see a way for wesl-rs to actually implement the rewriting behaviour in a sane way

https://github.com/webgpu-tools/wesl-spec/pull/183/changes#r3473134717

Which is frustrating, because I'm the one who came up with this idea in the first place.

[mighdoll]

Okay, let's go back to discouraging cross-package wildcard imports in library code, as a suppressible error.

While I'm sure some libraries do code generation in every host language ecosystem, it may be awkward. So let's not require that. And we can't get the golang/js/jvm communities to change their library expectations. So lowest common denominator for cross-host-language libraries: no ecosystem-required publish tool and no ecosytem-required occasional library breakage.

Which is frustrating, because I'm the one who came up with this idea in the first place.

Expansion can remain an optional packaging feature. We may eventually want a packaging step for other reasons besides wildcards: to fill in runtime visible library metadata (web runnable tests, viewable examples), incremental translation. But we should try to keep the packaging step optional.

I'll revise the section to lead with the diagnostic and move expansion to optional tooling.

For the diagnostic, tooling can usually infer library vs. application context from package metadata such as Cargo.toml or package.json. If that proves insufficient, we can later add an explicit wesl.toml flag.

[stefnotch]

Thank you! That alleviates my implementation concerns

[mighdoll]

revised draft.

[stefnotch]

We'll have to figure out some wesl.toml way of saying "this is a library" for the diagnostic to be implementable. Happy to defer that though.

(I think the JS ecosystem has private: "true". The Cargo ecosystem doesn't have a direct equivalent from what I remember.)

[mighdoll]

@! works for me. I'll update the PR.

PR is updated: ca3c45b

[stefnotch]

Commented. I'm happy with the overall idea of the PR. Remains a couple things to clarify:

* an earlier explanation of _why_ we impose restrictions to wildcards.

* a clearer explanation of `wilcard_shadow`, `wildcard_import` and `wildcardable` diagnostics in one section. Maybe `wildcardable` could be a module-scope diagnostic to avoid introducing new syntax?

* I'm unsure about the necessity of the "scope precedence" section if it mirrors WGSL's idea.

* I don't think the `wildcard_shadow` is necessary. There is no diagnostic in WGSL either for shadowed built-ins.

#183 (review)

@k2d222 Which of these concerns still apply?

[k2d222]

I couldn't keep up with the conversation (150 messages!), so here are my comments on the current draft. I think the spec can be improved in its clarity, but I approve of the ideas. I'm ok with merging and improving the prose later.

[k2d222]

"wins by precedence": only in the case where the diagnostic is silenced? otherwise it's an error? not clear here.

[mighdoll]

Not an error in either case. wildcard_shadow is a warning; the local declaration or named import wins by precedence whether or not the warning is suppressed. Suppressing changes only the reporting, not the resolution. I'll reword to make that clear.

[mighdoll]

revised draft

[k2d222]

I think user declarations and named imports are at the same level. Can you evert have a declaration and named import with the same name?

[k2d222]

user declarations/named imports silently shadow package names, but they trigger a diagnostic when shadowing wildcard-imported names.

[k2d222]

note that with #193 (comment), package names will no longer be in this list.

[mighdoll]

Can you ever have a declaration and named import with the same name?

Nope, I'll change the precedence list so they share a level. (good catch. I was trying to fit in wildcards, not change semantics of existing!)

user declarations/named imports silently shadow package names, but they
trigger a diagnostic when shadowing wildcard-imported names.

The wildcard warning is intentional. A wildcard import doesn't say which names it brings in, so a local declaration that collides with an imported name is a latent override: two plausible referents, with precedence quietly picking the local. Beyond confusing readers, there's a maintenance risk. Renaming or deleting a declaration is normally backstopped by the tools (every stale call site errors), but with a same-named wildcard import underneath, stale call sites silently rebind to the library's version instead if the signatures line up. And the collision trap can arm itself with no edit to your file, when the library adds the name in a minor bump. The warning flags the collision when it forms, and suppressing it documents intent.

Shadowing a package name is a different kind of thing. #159's import light::foo; plus fn light() {} - it silently breaks every light:: path, with no escape hatch. We'll fix with #193.

note that with #193, package names will no longer be in this list.

Agreed. That'll get us to the namespace split you suggested in May, and it also fixes the breakage above: module references are always followed by ::, so once path heads resolve in their own namespace the collision can't occur. I'll clean this section up in #193.

[mighdoll]

revised draft (for the non #193 stuff)

[stefnotch]

Current revision looks good to me, I'm happy with this being merged.

[k2d222]

I know I'm stepping into bikeshed territory.. but now I have some comments about the diagnostic names.

Essentially I look at @wilcardable wildcard_import, wildcard_shadow, library_wilcard and builtin_shadow and feel quite confused. There are 4 new diagnostics with relatively similar names, and I feel I would have a hard time remembering which is which, how they are spelled and what they do exactly: "was it 'shadow_wilcard', or 'wildcard_shadow'?

[k2d222]

it is suppressed at the declaration site of the offending declaration.

[k2d222]

Also, we haven't talked about whether these new diagnostics can be suppressed at the global module scope? Yes I suppose? would be good to mention it here.

[mighdoll]

it is suppressed at the declaration site of the offending declaration.

Yes, best on the offending declaration; module-wide via the global diagnostic directive also works. I'll rev draft and spell out better

[mighdoll]

Also, we haven't talked about whether these new diagnostics can be suppressed at the global module scope? Yes I suppose? would be good to mention it here.

Yes. Those work like other diagnostic filters, I'll clarify.

[mighdoll]

now I have some comments about the diagnostic names.

posted a new rev 9feb566

[k2d222]

Happy to merge in the current state, good work! 👍