Fix nullability for string constants and let namespace be chosen

[eqrion]

Addresses some review comments from #28.

[jakobkummerow]

LGTM with a comment.

[jakobkummerow]

Suggested change

	All imports that reference this namespace must be immutable globals and have a type that matches non-nullable externref (i.e. `(ref extern)` and `externref` are both allowed). If they are not, an eager compile error is emitted.
	All imports that reference this namespace must be immutable globals and have type non-nullable externref (i.e. `(ref extern)`). If they are not, an eager compile error is emitted.

(ref null extern) isn't a subtype of (ref extern), it's the other way round. And there's probably no reason to allow both? Hosts that support js-string-builtins populate these globals with non-null values, and for hosts that don't, supplying "" as a default import value is no harder than supplying null.

[eqrion]

I was imagining that when this option is enabled, the JS-API creates a (global (ref extern)) for each string constant that is the provided value for whatever the user asks to import (as if it was provided by the imports object).

The global import checking rules already allow for an immutable global import to be a supertype of the actual provided value. e.g. a module importing (global anyref) can be instantiated with a (global eqref) value. This only works for immutable globals.

I agree there's probably not much use for it, but I don't want to have special rules here so I thought I'd just match how it works already.

[jakobkummerow]

I'm not arguing for special rules, and I'm aware that a "slot" (global, local, param, etc) of type X can be satisfied with any value whose type is a subtype of X, and I'm not saying that nullable externref wouldn't work.

Line 174 says

the module may define imports of the form (import "'" "%stringConstant%"" (global (ref extern)))

with the type just being tightened to non-nullability in this PR. Line 178 then saying

(ref extern) and externref are both allowed

seems to be at odds with that. I'm suggesting that we pick the simple option: the type of the imports must be (ref extern), the type of the values satisfying them must be (ref extern), subtyping works as it always does (which is trivial in this case). We certainly could certainly allow more freedom, but if we get zero benefit from it, why not be conservative and keep things as simple as possible?

[tlively]

One benefit of allowing magic imports to work anywhere normal imports would is that it would allow producers that don't know about GC and non-nullable references (e.g. LLVM) emit code that uses string builtins and magic imports. In fact, LLVM could emit such code today with no modifications as long as the globals are allowed to have nullable types.

[jakobkummerow]

What do you mean by "allowing magic imports to work anywhere normal imports would"?

(I also don't really see how a legacy module producer that hasn't been taught about this proposal would accidentally generate string constants in the form of imported globals that just so happen to fit this proposal's requirements, with or without nullability.)

[eqrion]

@jakobkummerow

Now this PR in its current form adds the following behaviors IIUC:

(global (import "'" "foo") i32) → OK for WebAssembly.compile, CompileError later during WebAssembly.instantiate. What's the reason for the inconsistency? Why not check during WebAssembly.compile? Why CompileError and not LinkError?
(global (import "'" "foo") (ref extern)) → OK (uncontentious)
(global (import "'" "foo") (ref null extern)) → OK. Why are globals' types more permissive than function signatures?
(global (import "'" "foo") (mut (ref extern)) → OK for WebAssembly.compile, CompileError later during WebAssembly.instantiate. So we do have some extra restrictions for these globals, the principle isn't simply to leave them unconstrained. Why is immutability enforced but non-nullability is not?

For the first and last cases here, I had intended for these to fail WebAssembly.compile with an eager CompilerError, just like when there is a mismatch with functions. That was the purpose of "This check is eager, resulting in a compile error if it fails." line in this PR on line 176.

@tlively

I think I see where @jakobkummerow is coming from here. There are two independent phases: first the names and types of imports are examined to determine what globals (constructed with string values) and functions are automatically added to the supplied imports, and second the normal instantiation process happens with all of its normal import checking rules.

I think the two differences I have is that I was intending for the import names alone to be what determines for global/function type is provided to the 'supplied imports'. Each builtin function/global has a type determined by the spec, the import name does a 'builtin lookup' with it. And then I was intending for the import matching that normal instantiation would do for these 'supplied imports' to happen during compile time instead.

We could have a model where the import names and types are part of the 'builtin lookup', but I'd like to hear the benefit of this. In this case for 'imported strings', it might result in a slightly simpler check when compiling, but I don't think that's worth the irregularity.

If we were to introduce a type import for JS strings (as a subtype of externref), this model would extend naturally so that we could redefine the imported string globals as (global (ref $JSString)) and the normal subtyping rules would ensure that old modules that imported (ref extern) would continue to work.

[eqrion]

@jakobkummerow I'd like to resolve this soon, am I still misunderstanding something? I still have a preference for the current proposed import type checking, but can discuss it more.

[jakobkummerow]

My opinion hasn't changed:

I feel strongly that the current wording doesn't make it clear that the behavior is what you described in your previous comment. Please clarify the wording to make it it unambiguous what kind of error is produced (LinkError or CompileError) and at what time (WA.compile/WA.validate or WA.instantiate).

Further, I still think this PR creates an inconsistency between imported string functions (where we do require one exact type, else LinkError) and imported string constants (where this PR says "any type works, as long as subtyping rules are satisfied"), and I see no reason for this inconsistency, and I'm surprised that others who usually complain loudly about design inconsistencies are apparently happy with this case. But I'm tired of arguing over such a detail, so go ahead and do whatever.

[rossberg]

AFAICS, there is no inconsistency in what @eqrion describes. The difference in subtyping between global and function types is completely consistent with regular imports and just a consequence of the existing requirement to explicitly pre-declare subtyping between the latter, see my earlier explanation.

[eqrion]

@jakobkummerow I agree the overview could go into more detail here. I'll push a commit here to specify things better.

[eqrion]

I expanded the section to give an example and detail the changes to the JS-API in enough detail the semantics should be well specified.

I also opportunistically rolled this together with #32 by changing importedStringConstants to be a USVString? instead of a boolean. This lets the user specify which namespace the constants will be in. No default namespace is implied, one has to always be explicitly given. In the case of a future defined integration with esm-integration, we could decide what default to give the namespace, if any.

I plan on merging this next week if there's no objections before then.

[jakobkummerow]

Thanks, looks better now. One minor comment.

[jakobkummerow]

There's one corner case to consider here: if a Wasm module's only imports are string constants, do we still want step (1) of 'read the imports' to throw a TypeError when no importsObject is provided?

If not, then the algorithm's logic would have to accommodate that somehow, e.g. by rephrasing step (1) as:

"If module.imports excluding the imported string namespace, if specified, is not empty, and...".

Virtually all real-world modules will have additional imports anyway, so this corner case is unlikely to be relevant in practice. It easily comes up in spec compliance tests though.

[eqrion]

Yeah, that's a good idea. I'll add a note.