fix(wit-parser): improve checking for stable feature gates

[vados-cosmonic]

This commit introduces some extra checking (and panicking) for feature gates that are stable (i.e. Stability::Stable) AKA @since with a version and/or features specified.

There are two primary changes:

• Make referring to a future version of the package an error (for now)
• Ensure that if a feature is specified that it is checked before inclusion

In the past Stability::Stable feature gates were simply treated as Unknown, which hides the information necessary for downstream crates/consumers to use (and created the question around whether referring to future package versions is valid).

See also the relevant Zulip discussion

[alexcrichton]

Thanks! Could you be sure to add a test for this too? Something in tests/cli/* should suffice with a // FAIL: ... header

[vados-cosmonic]

Hey @alexcrichton so looking back at the WIT design doc (while I was trying to change it):

To enable a smooth transition (during which producer toolchains are targeting a version earlier than the @since-specified version), the @SInCE gate contains an optional feature field that, when present, says to enable the feature when either the target version is greator-or-equal or the feature name is explicitly enabled by the developer. Thus, c is enabled if the version is 0.2.2 or newer or the fancy-foo feature is explicitly enabled by the developer. The feature field can be removed once producer toolchains have updated their default version to enable the feature by default.

It looks like there is an expectation for feature to be used along with version to decide when to process a gated item...

Maybe this PR should reflect that, rather than changing it and then updating the docs upstream?

[alexcrichton]

Ah I had missed that as well, good catch! That seems reasonable to me to implement yeah, want to do so as part of this PR?

[vados-cosmonic]

Done!

I needed to add some changes (and update some tests), would appreciate some extra close eyes on this change set (and of course the rest of the PR).

Requiring the feature be specified is making a bunch of interfaces be missing, along with the deprecated flag (which wasn't being taken into account before), just want to make sure I'm reading the test output right and it passes a human spot check that isn't mine.

[vados-cosmonic]

Actually, some more thinking here @alexcrichton ... The way that the note is phrased, we actually must allow @since with versions in the future right?

Thus, c is enabled if the version is 0.2.2 or newer or the fancy-foo feature is explicitly enabled by the developer. The feature field can be removed once producer toolchains have updated their default version to enable the feature by default.

Given this sentence it assumes it is possible for c to be at a version that is after the package version -- otherwise it's just a feature check (because right now we consider future versions a bug).

I think to match this expected functionality we have to allow @since(version = x) where x is a future version.

I'm updating the code now to match

[alexcrichton]

Hm I think you're right on your interpretation of the text, but now I'm having second thoughts about supporting it exactly as-is. How about reverting back to:

• With @since the feature is ignored and is purely informational in the WIT itself
• The @since version must be <= the current package version
• Using @since requires the package to have a version
• Mixing @deprecated with @since has no effect on whether it's included or not

That to me feels like a more straightforward way to interpret these attributes and if you agree I can work on sending a PR upstream as well.

[vados-cosmonic]

Hey so after some discussion at the JS meeting with @guybedford we actually realized another issue -- the AND vs OR combination of version and feature makes a bunch of things easier w/ respect to figuring out dependencies (i.e. if you know some world imports a, and a has @since(version = v.v.v, feature = f) you can intuit the required package and enabled features of the relevant package).

There's also the benefit (possibly anti-benefit?) of being able to span a feature over multiple versions (changing how a certain item works over two different versions). An example of this that might make sense is something like:

@since(0.2.0, feature = async)
@since(0.3.0, feature = async)

I'm I think there might be a bunch of surprise at the ORing of those, does it make sense to actually hammer this out/give everyone a chance to chat about it in an issue on component-model?

[alexcrichton]

I'm not sure I quite follow what you mean by AND vs OR? But regardless it seems fine to discuss some more of the finer details on the component-model repository as well.

[vados-cosmonic]

Ah so before I make the other issue -- basically the idea of whether version and feature should be AND or OR'd --

The logic right now in the WIT design describes an OR (valid version or feature enabled), but it might make sense/be unsurprising to have an AND -- i.e. valid version and feature enabled to see the import at all.

So there are 3 options:

• OR (current WIT design description)
• AND (new)
• Strictly @unstable -> @since with ignored feature

[alexcrichton]

Ah ok I see. Personally I don't think an AND is viable given how features/since/unstable are planned to be used in WASI. I also don't think that the OR requirement as stated in WIT.md necessarily makes sense because it doesn't really make sense for since to list a version in the future, but if it's always <= the current package version then the OR-ness is like it's OR-ing with true which doesn't affect too much.

In any case I agree with your 3 options, and probably good to discuss upstream!

[alexcrichton]

Thanks! One last comment here and I think this is good to go

[vados-cosmonic]

Seems like actions/upload-artifact is having a bit of an outage, hopefully this that step can just be re-run and pass soon!

[EDIT] Rebasing one more time just to try and re-trigger CI