visibility design and discussion

[mighdoll]

TLDR

1. Module visibility. Three levels: public (visible to any package),
   private (declaring module only), and package (same package, the unmarked
   default).
2. Re-exports. public import re-exports the imported names under the
   current module's path; public import path::* does the same for a wildcard
   (re-uses @wildcardable diagnostics).
3. Host pipeline visibility. The link's root module defines the host-visible
   API: entry points, resource variables, and pipeline-overridable constants
   reach the host only if the root module declares them or public imports them.
4. package.wesl. The file backing a package's top-level module, so its
   items are reachable as <package>::item (e.g., import wgsl_test::*). One
   place a library can put a re-export prelude.

Why these four things are covered:

• module visibility is needed for re-exports
• re-exports are needed for wildcards
• pipeline visibility works best integrated with the module visibility design
• package.wesl is so that we can write e.g., import wgsl_test::*;

(Note that VisibilityLanguages.md is discussion background, it'll be removed before merging.)

Would resolve:

• #65 @publish import
• #98 allow private marking
• #28 public by default (closed)
• #130 wildcards
  (largely covered already; this PR fills in the re-export piece)
• PR #62 comment for lib.wesl

Related

• #103 sub-modules private by default

[s-ol]

This looks very solid to me and would definitely bring a big usability gain to libraries like lygia. The one thing that doesn't seem be discussed in much detail is package.wesl - from what I gather the package structure becomes something like:

• pkgname/
  • package.wesl
  • mod_a.wesl
  • mod_b.wesl
  • mod_b/
    • nested.wesl

It seems like the docs here don't explain why this approach is chosen over an index-style approach, i.e.:

• pkgname/
  • index.wesl
  • mod_a.wesl
  • mod_b/
    • index.wesl
    • nested.wesl

Personally I don't have a strong opinion either way, but there is lots of prior art for the second option (most JS package systems, Python, Rust, Lua) while I can't think of anything that has this strict root/non-root separation off the top of my head.

[stefnotch]

I believe that we should be able to dig up the Rust recommendations for that. IIRC, Rust discourages the mod.rs style, because it leads to a lot of files with the same name. Which means that whenever a tool only or primarily shows a file name, one ends up with very little useful info.

For example, if I have 3 tabs open in VSCode and they all say mod.rs, I will have a hard time finding the right one.

[stefnotch]

Also quick and very inconvenient note: private is somehow not a reserved WGSL word.

[stefnotch]

public import path::* has one notable technical difficulty: It makes import resolution a lot more complex.

Without wildcards, resolving an import means going to the right module and finding the item.

With wildcards, resolving an import means looking through a set of modules to find the right item.

With wildcard re-exports, resolving an import means looking through a set of modules recursively to find the right item. And since modules can have cycles, one now needs to keep track of which modules have been visited during import resolution. In the language server, this is a fair amount of extra complexity.

This particular feature also, I believe, interacts really badly with macros or other compile time code generation. Rust's import system ends up being a very complex fixed point finding algorithm which may sometimes simply fail.

(I think it also interacts with parameterized modules. Since wildcard reexports is the missing piece to make import statements recursive. If imports can cause any sort of computations, which compile time generated code does, and which parameterized modules do, then one gets the halting problem. Or so the story goes.)

Usually I wouldn't object to complexity too much.
In this case, I did want to point it out, since wildcard re-exports enables nothing new. It is a very rarely used feature and I could always manually write it out.

[mighdoll]

Personally I don't have a strong opinion either way, but there is lots of prior art for the second option (most JS package systems, Python, Rust, Lua)

while I can't think of anything that has this strict root/non-root separation off the top of my head.

Not sure, always interesting to look around tho!

Here I was only filling the gap where our current scheme doesn't allow something at the root level. I think small libraries especially might like to enable their users to my_pkg::foo(); or import wgsl_test::*; without adding an extra path level.

It'd be a separate issue, but your comments about consistency... I suppose we could revise the existing rules so that X::X.wesl always flattens the contents to X::. package::package.wesl flattens to package::. lygia::math::math.wesl flattens to lygia::math, etc.

[mighdoll]

Also quick and very inconvenient note: private is somehow not a reserved WGSL word.

wat!? good catch. Perhaps @public @private. I think we'll want to ask the WGSL committee too.

[mighdoll]

Usually I wouldn't object to complexity too much.
In this case, I did want to point it out, since wildcard re-exports enables nothing new. It is a very rarely used feature and I could always manually write it out.

Worth thinking about. I like the idea of implementing the needed features w/o gilding the lily too much.

The cycles/complexity concerns seem heavy for a feature that isn't must-have. And foreclosing options for parameterized modules would be unfortunate.

Also I realize that the PR doesn't address some related issues:

• what if there's a latent collision between wildcard re-exports in a library? That would be something we'd want to catch at publish time, not wait for clients to use the library and find an error.
• what happens if you wildcard re-export from a module with wildcard imports?

I'm thinking best would be to pull wildcard re-exports from this PR.

Side note, if we go with the publish tool route as planned with #183, I suppose we could also expand wildcard re-exports like we're planning for wildcard imports. That would help for the cross package cases.

[mighdoll]

pushed a draft

• @public and @private
• wildcard re-exports deferred

[stefnotch]

We could also go with

• pub, priv
• public, protected (Rust does not have a private keyword, upside of private by default)

Also, what does private mean? Rust goes all in on the hierarchical module system and says

If an item is private, it may be accessed by the current module and its descendants.
https://doc.rust-lang.org/reference/visibility-and-privacy.html

[mighdoll]

* `pub`, `priv`

I like this option too, a bit more cryptic but if perhaps best if we prefer keywords to attributes.

* `public`, `protected` 

I like public clear and readable. I fear confusion around protected - it has a contrary meaning in other languages visibility systems.

I suppose we could do public and priv..

Also, what does private mean? Rust goes all in on the hierarchical module system

just the module, no hierarchy. we could expand hierarchically later, it's a nice idea. But I think of most WESL programs as smallish, so perhaps unnecessary complexity, better to defer.

[stefnotch]

For the public, protected option, I was trying to suggest having the following visibility modifiers

• public (keyword)
• protected (keyword)
• private (the default, thus does not need a keyword)

[mighdoll]

* `private` (the default, thus does not need a keyword)

Ah, that was my first instinct too. I like modularity. But I've come to think that package by default for WESL is better. See Why package by default? in the design discussion doc. package by default is low ceremony for most projects, and works better for wgsl compat at the root module..

[stefnotch]

Also interesting tidbit:
Rust allows for two different styles of visibility. One is a local and hierarchical style, where I only use public and private. And I apply that to both items and modules. A internal (protected) item is modeled by having a public item inside a private module.

The other style is the global style, where I use 3 access modifiers. Public, internal (protected) and private.

And apparently some of the very prolific Rust contributors, including epage who maintains a bunch of extremely well written libraries, vastly prefer the global style. So we are on the right track!

https://www.reddit.com/r/rust/comments/1k5yv6p/two_ways_of_interpreting_visibility_in_rust/

[stefnotch]

More from Rust land:
The reasons for why they went with private by default for fields is to allow for local reasoning and

Visibility is often how soundness is achieved for many types in rust.
https://rust-lang.github.io/rfcs/0001-private-fields.html

They also include statistics which says that private is more common than public.

[stefnotch]

And from Kotlin lang: They went with public by default, only to realize that it might have been a mistake. So defaulting to a more restrictive choice like "internal" seems wise

https://kotlinlang.org/docs/api-guidelines-simplicity.html

[stefnotch]

Note from Java:
Java's default is actually somewhat similar to Rust. In Java, a "package" corresponds to a folder. And classes are visible within a package by default.

[stefnotch]

We really should ask the WGSL committee regarding the private keyword. Would you like to do the honors?

[stefnotch]

I dug into it and asked the WebGPU commitee gpuweb/gpuweb#6264

I couldn't quite figure out where it got lost though. It seems to have happened quite a while ago, judging from Naga's source code

[stefnotch]

I talked to a friend regarding this. priv would be a weird choice. As much as it pains me to say this, I would prefer private as a context sensitive keyword over priv.

[stefnotch]

I suppose one way of properly dealing with override would be to extend WESL to also cover a small bit of the WebGPU API. We'd extend the way createShaderModule works or something along those lines.

[stefnotch]

There is one amusing way of bypassing this restriction.

//- utils.wesl
struct InternalStuff { a: 32 }

//- package.wesl
import package::utils::InternalStuff;
public alias NowItIsPublic = InternalStuff;

[mighdoll]

clever and amusing, indeed!

[stefnotch]

Should we see if we can get #183 over the finish line?

Edit from the future
#193 and the new @!wildcardable syntax do the trick for me.

Old proposal
I do have one alternative proposal for this and #183, which is based around making full use of import statements. Here I'm only listing the upsides.

It would look like this

@wildcardable
public import bevy::prelude;

Upside: No new module keyword for this use-case. Fewer keywords is better.

Sub-modules would be private by default.

public import package::lights;
public import package::shadows;
internal import package::utils;

Upside: Automodules would no longer be a feature. It adds a lot of complexity to the language server. (I also suspect that the current version of automodules is not implementable in browsers, since it relies on web servers returning a "not found" within a reasonable timeframe.)
Upside: Automodules were an anti-pattern for libraries, since libraries should document their modules. Which cannot be done with automodules.

@if would compose nicely. For example, to do a switch between desktop and mobile, I would do

@if(DESKTOP)
internal import package::desktop::raytraced_lights as lights;
@if(!DESKTOP)
internal import package::mobile::lights;

and now package::lights always refers to the correct thing. More importantly, the language server will not suggest package::mobile::lights as the autoimport whenever I use lights. I think this is a big upside. The current proposals do not have a good way of doing this.

internal becomes a keyword. Thus enabling the following, which does not widen the import

internal import package::vertex::vertex_main;

Upside: Enables in-browser unit test runners.
Currently, if I want to unit test my WESL code, I need to set up wesl-js with a bundler for it to discover all .wesl files. Otherwise, given a /shaders/package.wesl URL, the browser has no way of discovering the /shaders/tests/foo.wesl file.
However, if sub-modules are private by default, then it becomes logical to add import statements for them. And thus, the browser can discover the files. This also matches the behaviour of Javascript.

Finally, I took a stab at implementing this. It's about equal in terms of complexity, since this just requires re-exports.

[stefnotch]

New question: If it is an item in the current module, but the item is not accessible, what should happen?

1. Throw an error. This always behaves consistently.
2. Ignore it and continue on. Downside is that name resolution can now depend on where you're importing something from in a very funky way.

[mighdoll]

New question: If it is an item in the current module, but the item is not accessible, what should happen?

1. Throw an error. This always behaves consistently.
2. Ignore it and continue on. Downside is that name resolution can now depend
   on where you're importing something from, in a very funky way.

Option 1 (error). Naming an item that exists but isn't visible to you should be an error. You asked for that specific item, so you should be told you can't have it.

That doesn't block traversal through a same-named submodule, though: if foo has an inaccessible item bar and also a foo/bar.wesl, then foo::bar::baz still resolves into the submodule. Only the terminal foo::bar is the item (and errors if it's inaccessible). Direct resolution (#193) makes this especially clear.

[stefnotch]

New question: The default visibility of an import statement is private, right?

So it's only visible to the current file.
For example, only Nya is accessible from the outside. To access anything else, one has to be in the current file.

// my_file.wesl
import bevy::Bar;
private const Foo = 3;
public const Nya = 5;

But what about self-imports?
For example, should this be legal?

// my_file.wesl
import package::my_file::Foo as Bar;
private const Foo = 3;

Rust argues that yes, it is legal, because

use std::f32::NAN; // creates an item NAN with private visibility

use self::NAN as Nya; // imports NAN, which is allowed, because this is the same file as NAN 

[mighdoll]

Upside: Automodules would no longer be a feature. It adds a lot of complexity
to the language server.

(We've started discussing in #193; I agree direct resolution is better.)

@wildcardable
public import bevy::prelude;

Upside: No new module keyword for this use-case. Fewer keywords is better.

Users consume wildcards per-module, so I think it's better to mark wildcardable per-module. Otherwise we risk inconsistencies between what can be imported directly vs what can be wildcard imported from the same module. That seems likely confusing for users.

Saving a keyword is nice of course, and we found options for that in #183. (I just don't think we like them better.)

Let's discuss further in #183 if we should revisit module syntax.

Sub-modules would be private by default.

public import package::lights;
internal import package::utils;

A module whose items are all non-public is effectively private already, which falls out of item visibility rather than needing a module-private concept.

I'd rather leave future module visibility questions undefined for now. We might not need the separate module-visibility axis.

internal becomes a keyword. Thus enabling the following, which does not widen
the import

internal import package::vertex::vertex_main;

Yep, we could add a keyword if we want package-internal re-exports. As to the keyword, the draft suggests package rather than internal to stay in sync with the package:: syntax. Do we need package-internal re-exports? I think it's certainly reasonable, but debatable. (It's in the Future possibilities section now.)

@if(DESKTOP)
internal import package::desktop::raytraced_lights as lights;
@if(!DESKTOP)
internal import package::mobile::lights;

In the current proposal you can conditionally re-export items for the public API:

/// lights.wesl
@if(DESKTOP)
public import package::desktop::raytraced_lights::{FooLight, BarLight};
@else
public import package::mobile::lights::{FooLight, BarLight};

(I see the ConditionalTranslation spec lags the test suite and wesl-rs/wesl-js on allowing conditions on import statements; we should fix that.)

Also just noting that this kind of thing will be awkward until we have some interface contracts to help keep these desktop vs. mobile APIs cross-compatible.

Re-exports become more ergonomic for library authors if we enable wildcard re-exports too:

/// lights.wesl
@if(DESKTOP)
public import package::desktop::raytraced_lights::*;
@else
public import package::mobile::lights::*;

That's listed in the future section in this draft. It helps library prelude authors with maintenance, but doesn't add anything new for users of wildcards. We can add it later, by having the library packaging tool rewrite wildcard re-exports to named re-exports. The design doc lists a couple of other open questions we'll have to work through (module re-export cycles, etc.; all seem solvable). Library-safe wildcard re-exports are separable, so better in a separate PR when we want it.

Re-export for package internal APIs would make sense in the current proposal if/when we add a package import form (the design doc lists it). I don't think it requires a separate module visibility axis, though it might also motivate the explicit package declaration keyword at some point.

given a /shaders/package.wesl URL, the browser has no way of discovering the /shaders/tests/foo.wesl file.

That's true, but I think the answer is at the tooling layer, not in visibility or imports. I agree that a test runner needs some way to find its tests. It finds them by searching for test files (a manifest, or a filename/path convention), not by traversing imports from package.wesl. Traversal wouldn't reach them anyway, since tests import their target shaders, not the reverse. So I don't think submodules need to be private by default to make tests discoverable.

This also matches the behaviour of Javascript

JavaScript test frameworks typically use tooling to search the filesystem for tests by path/filename suffix. wgsl-test does the same thing currently.

Old school browser-only test frameworks would manually list their tests in a test file, AFAIK.

Should we see if we can get #183 over the finish line?

Yes! I imagine we'll need this one too (#187) to declare success on #183. (The immediate motivation for visibility was because we needed re-exports to merge items from multiple modules for prelude::s.)

[mighdoll]

New question: The default visibility of an import statement is private, right?

So it's only visible to the current file. For example, only Nya is accessible from the outside. To access anything else, one has to be in the current file.

// my_file.wesl
import bevy::Bar;
private const Foo = 3;
public const Nya = 5;

Yes, import statements don't have visibility, only items. So only Nya is importable externally.

But what about self-imports? For example, should this be legal?

// my_file.wesl
import package::my_file::Foo as Bar;
private const Foo = 3;

Yes, legal: import package::my_file::Foo as Bar; is just a local alias, and an item is always visible to its own module.

Note that public import package::my_file::Foo as Bar; would be an error. A re-export can't widen visibility.