Skip to content
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

Add @feature and @since gates to WIT #332

Merged
merged 10 commits into from
May 28, 2024
Merged

Add @feature and @since gates to WIT #332

merged 10 commits into from
May 28, 2024

Conversation

lukewagner
Copy link
Member

Based on a number of other folks' ideas and feedback (esp. @yoshuawuyts, @ricochet, @alexcrichton), this proposal adds purely-WIT-level "gates" in order to reduce the churn necessary to make minor version releases (in preparation for WASI 0.2.1).

The motivation for adding these gates is that, without them, when adding a new function or type to an interface as part of a minor release, the transition is fairly toilsome and may require multiple copies of WIT documents be maintained (e.g., a "stable" snapshot, a "candidate" snapshot, and an "actively-developed" draft). By definition, minor updates must be purely additive, so the idea is to "compress" these snapshots into a single document, by syntactically annotating the new feature with what minor version they were introduced in. Importantly, these gates do not show up in a component or runtime; they are erased as part of the process of building a component (selecting the target version and experimental features to enable).

(At some point, we may add function-level subtyping, at which point minor updates won't just add new functions, but also modify existing function types and so we'll need to nuance gating. But I think that should be possible and until then we don't have to worry about it.)

The PR explains the new WIT syntax and give some examples.

@yordis
Copy link

yordis commented Apr 1, 2024

Should the spec include any tagging with @[tag name] and reserved tags such as @feature and @since? We have something similar in the Elixir land.

@lukewagner
Copy link
Member Author

I might be misunderstanding what you're suggesting, but my hope here wasn't to add a generic annotation syntax since I think that opens a can of worms that require a lot more thought and discussion. Technically, all @[tag name]s (other than @feature and @since) are still reserved after this PR since they are all rejected by the WIT parser.

@Mossaka
Copy link
Contributor

Mossaka commented Apr 3, 2024

I like these additions to WIT!

Why are @feature and @since gates only apply withint the interface scope? I think the same motivation applies to worlds too, no?

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great; I believe this should cover everything it needs to. Thank you @lukewagner!

@lukewagner
Copy link
Member Author

@Mossaka Great point! I should probably add the gates to world-items as well.

@yordis
Copy link

yordis commented Apr 3, 2024

but my hope here wasn't to add a generic annotation syntax

@lukewagner, that is what I meant 😅 ignore me for now; the conversation could happen another day in a different space!

@theduke
Copy link

theduke commented Apr 3, 2024

For reference: discussion about generic annotations: #58

design/mvp/WIT.md Outdated Show resolved Hide resolved
@lukewagner
Copy link
Member Author

As an update, based on @lann's suggestion, I renamed @feature to @unstable. Doing this made me realize that, instead of allowing these annotations to both be applied, it's much nicer to read if we allow the feature field (of @unstable) to be optionally present in @since.

Comment on lines 972 to 978
`@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.
Copy link
Contributor

@lann lann Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this imply that the feature fancy-foo was stabilized in version 0.2.2? Would it make sense to prohibit @unstable(feature = "X") and @since(..., feature = "X") appearing in the same package?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question! I suppose you could imagine that someone might want to define an umbrella feature flag name and then the parts of that feature get standardized in chunks with separate @since gates, but all referring to the same feature flag... I don't know if that's an anti-pattern, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I can imagine that. I'm not sure if it's an antipattern but I do think the proposed syntax here could be quite confusing if used that way.

@@ -934,7 +934,48 @@ include-names-item ::= id 'as' id
## Item: `interface`

Interfaces can be defined in a `wit` file. Interfaces have a name and a
sequence of items and functions.
sequence of items and functions, all of which can be gated on an `id` or a
semantic version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be putting the cart before the horse a bit since we're a ways away from 1.0, but maybe it's worth discussing now anyway: what would an example of a traditional semver patch version be in the WIT context? I've spent a bit trying to think of an example, and I can't seem to think of one, except for something trivial like a comment typo fix.

I generally take a patch bump to mean "the interface is exactly the same as before, but the implementation has changed in some non-semantically-affecting way". But a WIT file is all interface, no implementation. So it seems impossible to make a patch-only change? Ex: if I have local:[email protected], except for (maybe?) comments, I believe the *.wit for local:[email protected] must be identical to be valid semver.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that patch versions don't seem to immediately have a real use case for an interface-only language like WIT. Comments are indeed to main, and I think important, use case for them in this context though: just as implementation code, comments can contain bugs, and the use of patch versions allow us to fix those.

))
)
```
Thus, `@since` and `@unstable` gates are not part of the runtime semantics of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious what happens if I erroneously compile against an interface that is too far forward? That is, I compile against 0.2.2, but then try to run on a runtime that only knows about 0.2.1. So in this example, my consumer will potentially try to use method c on this interface, but the runtime (or component generic implementor, if we're not talking about WASI) won't know about it yet.

Is reading of this document correct that there's nothing we can do about this at compile time, since we can't deduce which runtime we'll be executing on at that stage? And if so, it seems like there's nothing we can do about it at runtime either due to the restriction in this section.

This might end up being a frustrating experience for users. I'm not well-versed enough in the details yet to know if the consumer will fail at start time (when it fails to find the function to fulfill the import it expects), or while running (when it makes a call to c that can't be handled). The latter of those seems especially challenging, as one could easily miss it during testing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is reading of this document correct that there's nothing we can do about this at compile time, since we can't deduce which runtime we'll be executing on at that stage?

That's correct, yes. The only real way to do something about this is to have the toolchain target a world (and version) that is known to be supported by the targeted runtime. I think that's pretty fundamentally true, and not even just for WIT: to give another example, if in a natively-compiled application you target a specific version of an operating system and make use of functionality not available in older versions, your application won't work.

And if so, it seems like there's nothing we can do about it at runtime either due to the restriction in this section.

Can you say which restrictions you mean, and how they could be changed to address this?

One thing we do want to do, but that's separate from this change, is to support optional imports/exports. Those would allow developers to make use of functionality if it is available, but not forcibly rely on it.

And separately, note that the Component Model does have other ways to address all this. Specifically, since all APIs can be virtualized, it's possible to eliminate imports by wrapping a component in another one that provides an implementation of that import in terms of other functionality. As just one scenario, this could be done as part of a deployment pipeline when that pipeline detects that the runtime environment is lacking some APIs.

This might end up being a frustrating experience for users. I'm not well-versed enough in the details yet to know if the consumer will fail at start time (when it fails to find the function to fulfill the import it expects), or while running (when it makes a call to c that can't be handled). The latter of those seems especially challenging, as one could easily miss it during testing.

This would show up as a link-time error, not at runtime. But again, besides what I described above it's not clear what could be done about this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one other feature of runtimes that helps mitigate this scenario that @azaslavsky is describing (which, iiuc, is what is currently implemented in Wasmtime): let's say I compile my component targeting a world that imports [email protected] but my component only uses features also present in [email protected]. In this scenario, Wasmtime will ignore the minor version part of [email protected], and just see if it has an implementation of i@1. If the runtime only has an implementation of [email protected], then linking will succeed (again, assuming the component is only using functions also present in [email protected]). Thus, we can leverage semantic versioning to be a bit more permissive than otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, yes. The only real way to do something about this is to have the toolchain target a world (and version) that is known to be supported by the targeted runtime. I think that's pretty fundamentally true, and not even just for WIT: to give another example, if in a natively-compiled application you target a specific version of an operating system and make use of functionality not available in older versions, your application won't work.

That's true, though on (most) traditional Unix/Linux systems, your interfaces with the outside world are either an arbitrary copy of libc (if you're talking to the system itself), or an untyped, random-bytes-on-the-wire IPC message, if you are talking to another user space program. The orchestrating system doesn't have much visibility into which kinds of interactions the binary it's running expects.

Since all WASM interfaces with the outside world are so well-described and typed, I was imagining that we could provide more information than say, a Unix would when you try to use a binary compiled for an interface ahead of what your system implements.

Taking a variant of the example above, of function c that exists in 0.2.2 but not 0.2.1, I could imagine the following error scenarios, listed in order of perceived developer ergonomics:

  1. Arbitrary runtime failure a la unix. It seems like we avoid this in all cases by checking the interfaces at link time.
  2. A link time error with no further description: "The interfaces of the consumer and implementor did not match".
  3. A link time error with an explanation of what is wrong: "The consuming component expected c, but the implementing component does not provide c.
  4. A link time error with an explanation of what is wrong, plus version info: "The consuming component (version 0.2.3) expected c, but the implementing component (version 0.2.1) does not provide c." I think this the best we could do as things stand.
  5. A link time error with an explanation of what is wrong, plus a recommendation on how to fix: "The consuming component expected c, but the implementing component does not provide c. It was added in version 0.2.2." I think this especially useful for someone composing complex software from a tree of many components, without necessarily knowing much about any of their specifics.

All I'm saying is that the latter is the most ergonomic and actionable error for a linker to provide: it tells you exactly which version to bump to and why. But I could see an argument that such things are best handled by the package manager service/client that you use to pull your components (wa.dev, etc), though that would prevent for example local debug runs from seeing messaging like the above.

Anyway, I see the downsides of exposing this information at link time too, like it becoming load-bearing in unexpected ways. Maybe it is something left to some future revision, or not implemented at all. :)

Can you say which restrictions you mean, and how they could be changed to address this?

My reading of this document is that it prohibits exposing the since or feature annotations in a manner that allows runtimes to see them.

There's one other feature of runtimes that helps mitigate this scenario that @azaslavsky is describing (which, iiuc, is what is currently implemented in Wasmtime): let's say I compile my component targeting a world that imports [email protected] but my component only uses features also present in [email protected]. In this scenario, Wasmtime will ignore the minor version part of [email protected], and just see if it has an implementation of i@1.

I see, that seems like it covers most of this use case. Is this diffing aware of the versions, or is that information already erased, and it just checks to see if everything that the consumer requires happens to be provided by the implementor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version of the imported interface is present in the component's import string (see the grammar for interfacename) and thus when there is an error (let's say my component is using a function added by 1.1.0 that isn't present in the host's 1.0.0), the host should be able to synthesize a nice error message explaining that the component requires 1.1.0 but the host only has 1.0.0 (which is the error it would've given unconditionally if we didn't do this permissive "ignore the minor/patch version when resolving names" trick).

Copy link
Collaborator

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In implementing this in addition to a few notes below I've made a few other decisions along the way which I want to bring back here to discuss as well.

Referencing ungated items

I'm planning on considering this an error when the feature foo is not active:

interface i {
    @unstable(feature = "foo")
    type t1 = u32;
    type t2 = t1;
}

Notably this is an issue where when the feature foo is not active that means that t2 now references a type that doesn't exist. During parsing t1 exists but later on when a full document is created with inactive features filtered out it's no longer valid.

Attributes on interfaces and worlds

Not currently in this PR, but I'm assuming that we'll want to be able to do:

@unstable(feature = "foo")
interface i { .. }
@unstable(feature = "foo")
world w { .. }

That way WASI will be able to introduce new unstable interfaces in existing packages such as wasi:clocks/timezone perhaps (just a strawman example).

If this is desired then there's a question of:

@unstable(feature = "foo")
interface i {
    @unstable(feature = "foo") // is this attribute required?
    type t1 = u32;
}

Currently what I'm implementing the answer is "yes", if an interface is itself unstable then everything within requires unstable annotations as well.

Attributes on world items

Similar to above I'm assuming that we'll additionally want:

world foo {
    @unstable(feature = "foo")
    import my-new-interface;
}

That would enable, for example, WASI 0.2.1 to conditionally include the wasi:clocks/timezone interface in the command world depending on whether a feature is active or not.

Semantic operations expected on WIT packages

Currently all I'm thinking of implementing is "slurp up WIT documents with this set of features active" which produces a Resolve as-if all un-activated features were never there in the first place. All annotations are preserved for active items (e.g. since/unstable/etc), however.

Are there, though, other operations that others are keen to see implemented? For example this PR has operations such as viewing a 0.2.2 document at the version 0.2.0. While possible I wasn't planning on implementing that yet. I bring this up because the changes required by this feature are going to affect basically every single processor of WIT, e.g. all bindings generators, macros, etc. My plan so far was to add a list-of-strings "features" option to all of them, but if other common operations like versions need to come into play I'd prefer to plan for that sooner rather than later.

design/mvp/WIT.md Outdated Show resolved Hide resolved
g: func()
}
```
is encoded as the following component when the target version is `1.0.0`:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My impression is that the main intention for @since and `@unstable is less-so targetting old version of WASI but rather enabling a story for in-progress feature-development. Along those lines I do not yet plan on implementing (not that it can't be done, I just don't plan on doing it initially) support for "view this WIT from version 0.2.0". Instead what I plan on implementing is "view this WIT with these features active".

Given that would it perhaps make more sense to change this example to showcase the gating in that regard?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to talk through the workflow you're planning for how 0.2.1 is released: I imagined that, once 0.2.1 becomes official via the WASI SG, the interfaces/functions added by 0.2.1 would receive the @since(version = "0.2.1") gate so that producer toolchains can immediately pull in the new WITs but keep their default version at 0.2.0 for the transition period where most runtimes don't yet have 0.2.1 deployed (so that the default output continues to run in most places). If that was the case, then I would imagine you would need @since (in addition to @unstable) in the short-term. But are you planning the roll-out of 0.2.1 with a different sequencing or use of gates?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my assumption has been that when 0.2.1 is released everyone updates on their own schedule. If guest languages update before runtimes that's ok because a runtime would see an 0.2.1 import but realize it has an 0.2.0 version and would work ok. The only bad case would be when you use something only available in 0.2.1 and run it on an 0.2.0 runtime.

Given that there's no need for languages to pull in 0.2.1 WITs but pretend they're 0.2.0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there perhaps value in an intermediate state in which:

  • The newer (0.2.1) WIT is present in the toolchain, and so it's possible to use it if you want it.
  • But it's not yet made available by default because, once it's available, various things might pull it in unnecessarily leading to real failures (because the feature is actually being used) unnecessarily. Hypothetical examples I can think of include: (1) standard libraries that use the feature if it's present, even though they had a fallback path that didn't need the feature, (2) developers who use it because not because they absolutely needed it, but because it was there and they didn't know it was going to be an issue.

@alexcrichton
Copy link
Collaborator

Ok I have an initial implementation of this for wasm-tools over at bytecodealliance/wasm-tools#1508 which has a lot more nitty-gritty as well. If folks want to kick the tires on that and make sure it covers all the use cases as well that'd be appreciated.

@lukewagner
Copy link
Member Author

Great points @alexcrichton! Replying to them individually:

  • Referencing ungated items: That sounds right. I extended the final encoding example here to include that case.
  • Attributes on interfaces and worlds: Yep, I forgot all about them, but they make sense too. Added here, which meant factoring out the gate production into its own subsection before the others.
    • // is this attribute required?: I see the EIBTI argument for requiring compatible gates on all items of a gated interface or world, but I wonder if it's worth the visual noise and redundancy or if we could just say that any item inside a gated interface/world is implicitly gated by the containing gate?
  • Gates on world items: Yep, I forgot about them too. Added here

@alexcrichton
Copy link
Collaborator

Oh to be clear though for referencing ungated items the snippet I pasted is intended to be an error when the feature is disabled. The example added here shows an un-tagged item referencing a tagged item, which would be an error if the tagged item were omitted. In that sense should the example be updated to have a tag for t2 as well? The prose mentions that t2 is transitively disabled but I wasn't planning on doing that, instead making it a WIT-parse-error requiring the WIT document to get updated to pass parsing.

Personally when it comes to feature gating I'm a fan of explicitness, but I'll note that I'm biased here. In Rust we require all stable items to have #[stable] (individually) and it's only #[unstable] which is inherited internally. I'd personally be in favor of tagging everything as @since and @unstable in WIT.

@lukewagner
Copy link
Member Author

lukewagner commented Apr 19, 2024

In that sense should the example be updated to have a tag for t2 as well?

Oh yeah, good point, that would be simpler and less magical. Updated here to just mention it as a validation rule.

I'd personally be in favor of tagging everything as @SInCE and @unstable in WIT.

Yeah, I suppose it makes sense, and I'm also seeing the symmetry with the validation rule above. Added. Syntax errors in examples fixed.

```wit
@since(version = 1.0.2)
interface i {
foo: func(); // error: no gate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I certainly understand the reasoning behind requiring the redundant gate, but this does sound like a maintenance annoyance. Is it possible that this restriction could be lifted in the future and the default would be come that items inherit the gates of the parent item?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing it either way isn't really an issue, so in my mind it comes down to other reasons. At least with Rust #[stable] is exclusively used by the standard library so ergonomics aren't necessarily a high-priority concern as only a few authors interact with it. Additionally many methods/functions often have dozens-to-hundreds of lines of documentation in modules with dozens of functions, so the distance between @since on an interface to a func itself may actually be quite large.

That's what personally makes me lean towards requiring @since on items everywhere as it makes it easier to read primarily.

@brooksmtownsend
Copy link

brooksmtownsend commented May 1, 2024

  1. Good job GH, tagging users with the @
    image
  2. Does this have support for "overwriting" or "shadowing" existing definitions with newer versions? For example, say we updated function a to return a string in version 0.2.1. Is this valid?:
interface foo {
  a: func();
  @since(version = 0.2.1)
  a: func() -> string;
}

No hard requirement here, I think it makes more sense for the above example to be @since(version = 0.3.0) to simulate a breaking change, but I figured it's worth considering either way

@lukewagner
Copy link
Member Author

@brooksmtownsend Unfortunately not since, particularly in the absence of function-level subtyping, that would be a breaking change and we're only talking about @since for minor version changes. For major changes, the workflow is still "create a new directory with a copied set of WITs and then just edit them".

@brooksmtownsend
Copy link

I think that makes sense. That will probably generally encourage forwards compatible changes as well, rather than breaking existing methods when moving from version to version. Thanks for the quick response!

Copy link

@pchickey pchickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all for this design and discussion. Let’s start using this in the Wasi specs. As we push it through all the tooling, we can learn and iterate on it before we make any Wasi releases that will make it hard to change.

@alexcrichton alexcrichton merged commit 8d7a3df into main May 28, 2024
2 checks passed
@alexcrichton
Copy link
Collaborator

Er I apologize for the perhaps early merge for this, I'm apparently still waking up and I thought that this PR was bytecodealliance/wasm-tools#1508. I was planning on merging the implementation there to help get this merged. I think there's general agreement on this, but if anyone would like I'm happy to revert this on the component-model repository and make a second PR. Again sorry for jumping the gun!

@lukewagner lukewagner deleted the add-wit-gates branch May 28, 2024 19:13
@lukewagner
Copy link
Member Author

Hah, no worries; I was planning to merge as soon as we had a working implementation merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.