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

refactor: Remove optional feature attribute from stable feature gates #1741

Conversation

vados-cosmonic
Copy link
Contributor

With an eye towards making the discussion in component-model a reality, the commits in this PR update the wasm-tools to remove

Note that the change to wit-parser is a semver breaking change, as we're removing a piece of the Stability enum.

This commit removes the optional `feature` specification from feature
gates (`@since`, in particular).

This change should simplify the usage of feature gates (see
WebAssembly/component-model#387) for more
discussion.

This is a breaking change for those who were depending on `wit-parser`
as `feature` now no longer present.

Signed-off-by: Victor Adossi <[email protected]>
This commit updates `wit-component` to remove reliance on the
`feature` option when dealing with post-stabilization (`@since`)
feature gates.

Signed-off-by: Victor Adossi <[email protected]>
@vados-cosmonic vados-cosmonic force-pushed the refactor(wit-parser)=remove-feature-from-stable-feature-gates branch from 3914455 to 0bc0714 Compare August 26, 2024 14:09
@alexcrichton
Copy link
Member

Could this perhaps just be removed from enum Stability for now but left in enum Attribute? I'm wary to remove a feature cold turkey lest projects be broken, so having a smoother path to "this does nothing for awhile" before it's removed feels a bit better. Unfortunately there's not a great vector at this time to provide warnings about WIT files so we don't have the option of warning on deprecation of using WIT features at this time.

@pchickey
Copy link
Contributor

For whatever its worth, I think for brand new features like this one, its OK to break this corner case. In a few months I'd be more careful, but I think its pretty unlikely anyone will encounter this.

@vados-cosmonic
Copy link
Contributor Author

I can definitely see it both ways, and with regards to:

so having a smoother path to "this does nothing for awhile" before it's removed feels a bit better.

One thing I thought of might be to just make sure it the code never does anything useful (i.e. always puts None) and possibly outputs a warning.

So putting out the options at least:

    1. Make feature always None (least breaking -- not a breaking change)
    1. Remove from Stability, keep in Attribute (medium breaking)
    1. Remove from both

Could go either way here, but if @alexcrichton you're comfortable with (2) I can go with that, or if you want to go with what @pchickey suggested (which is 3), I can do that as well.

@alexcrichton
Copy link
Member

API-wise it's ok to have breaking changes whenever, that's what the semver-major-bumps are for in this crates on regular releases. Interface-wise-breaking though is more worrisome (e.g. changing WIT syntax). That's because it's expected that WIT with 215.* is compatible with 216.* and vice-versa (where possible). Changing WIT syntax by removing feature = ... breaks the both API and WIT compatibilty, but we can ignore the API parts.

That's basically a longer-winded way of saying, yes, I was adovcating for (2) and Pat for (3). That being said I don't disagree that this is new enough we can probably go with (3), so seems reasonable to just remove it outright (but that will require updating some tests it looks like)

@vados-cosmonic
Copy link
Contributor Author

Yeah thanks for saying some more about it... I actually just looked at jco (where I thought there would definitely be changes), and changes aren't even necessary... Looking around a bit I can't find anyone really using the optional feature...

@vados-cosmonic vados-cosmonic force-pushed the refactor(wit-parser)=remove-feature-from-stable-feature-gates branch from fa646f6 to cfe03bc Compare August 26, 2024 20:25
@alexcrichton alexcrichton enabled auto-merge August 26, 2024 20:29
@alexcrichton alexcrichton added this pull request to the merge queue Aug 26, 2024
Merged via the queue into bytecodealliance:main with commit fbb8abd Aug 26, 2024
29 checks passed
@vados-cosmonic vados-cosmonic deleted the refactor(wit-parser)=remove-feature-from-stable-feature-gates branch August 26, 2024 21:09
ydnar added a commit to bytecodealliance/go-modules that referenced this pull request Aug 31, 2024
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.

3 participants