From 0b24fa0cf2b2a6d4a5a8096d46f1409fcde4e39d Mon Sep 17 00:00:00 2001 From: Joel Dice Date: Wed, 18 Dec 2024 14:22:45 -0700 Subject: [PATCH] fix overly-aggressive pruning of types based on features Previously, `ast::Resolve` would sometimes assign a `Stability` to types according to the first function it found that type in as a parameter or result. That's a problem if such a function is marked `@unstable`, since it will effectively poison that type and prevent it from being used by any other function -- even if the type itself is ungated. This fixes the problem by removing the `&Stability` parameter from `resolve_params` and `resolve_results` and instead passing `&Stability::Unknown` to `resolve_type` from those functions. Additionally, I've added a new `find_stability` function which recursively inspects a `&TypeDefKind`, looking for a non-unknown stability. If it finds one, it will use that; otherwise, it will default to `Stability::Unknown`. For example, if `option` appears as a parameter type in a function, we'll start by assuming that type has unknown stability, then look to see if `T` has a stability (directly or transitively) and use that if so. Things get interesting in the case of e.g. `tuple`, where `T`, `U`, and `V` each have their own stability. Currently we just use the first known stability we find, but perhaps there's a better approach to consider? Signed-off-by: Joel Dice --- crates/wit-parser/src/ast/resolve.rs | 87 ++++++++++++++++--- crates/wit-parser/tests/ui/feature-types.wit | 13 +++ .../tests/ui/feature-types.wit.json | 70 +++++++++++++++ 3 files changed, 160 insertions(+), 10 deletions(-) create mode 100644 crates/wit-parser/tests/ui/feature-types.wit create mode 100644 crates/wit-parser/tests/ui/feature-types.wit.json diff --git a/crates/wit-parser/src/ast/resolve.rs b/crates/wit-parser/src/ast/resolve.rs index 714257308b..1ce7cb71dd 100644 --- a/crates/wit-parser/src/ast/resolve.rs +++ b/crates/wit-parser/src/ast/resolve.rs @@ -1045,8 +1045,8 @@ impl<'a> Resolver<'a> { ) -> Result { let docs = self.docs(docs); let stability = self.stability(attrs)?; - let params = self.resolve_params(&func.params, &kind, func.span, &stability)?; - let results = self.resolve_results(&func.results, &kind, func.span, &stability)?; + let params = self.resolve_params(&func.params, &kind, func.span)?; + let results = self.resolve_results(&func.results, &kind, func.span)?; Ok(Function { docs, stability, @@ -1293,6 +1293,68 @@ impl<'a> Resolver<'a> { } } + /// If `stability` is `Stability::Unknown`, recursively inspect the + /// specified `kind` until we either bottom out or find a type which has a + /// stability that's _not_ unknown. If we find such a type, return a clone + /// of its stability; otherwise return `Stability::Unknown`. + /// + /// The idea here is that e.g. `option` should inherit `T`'s stability. + /// This gets a little ambiguous in the case of e.g. `tuple`; for + /// now, we just pick the first one has a known stability, if any. + fn find_stability(&self, kind: &TypeDefKind, stability: &Stability) -> Stability { + fn find_in_type(types: &Arena, ty: Type) -> Option<&Stability> { + if let Type::Id(id) = ty { + let ty = &types[id]; + if !matches!(&ty.stability, Stability::Unknown) { + Some(&ty.stability) + } else { + find_in_kind(types, &ty.kind) + } + } else { + None + } + } + + fn find_in_kind<'a>( + types: &'a Arena, + kind: &TypeDefKind, + ) -> Option<&'a Stability> { + match kind { + TypeDefKind::Type(ty) => find_in_type(types, *ty), + TypeDefKind::Handle(Handle::Borrow(id) | Handle::Own(id)) => { + find_in_type(types, Type::Id(*id)) + } + TypeDefKind::Tuple(t) => t.types.iter().find_map(|ty| find_in_type(types, *ty)), + TypeDefKind::List(ty) | TypeDefKind::Stream(ty) | TypeDefKind::Option(ty) => { + find_in_type(types, *ty) + } + TypeDefKind::Future(ty) => ty.as_ref().and_then(|ty| find_in_type(types, *ty)), + TypeDefKind::Result(r) => { + r.ok.as_ref() + .and_then(|ty| find_in_type(types, *ty)) + .or_else(|| r.err.as_ref().and_then(|ty| find_in_type(types, *ty))) + } + // Assume these are named types which will be annotated with an + // explicit stability if applicable: + TypeDefKind::ErrorContext + | TypeDefKind::Resource + | TypeDefKind::Variant(_) + | TypeDefKind::Record(_) + | TypeDefKind::Flags(_) + | TypeDefKind::Enum(_) + | TypeDefKind::Unknown => None, + } + } + + if let Stability::Unknown = stability { + find_in_kind(&self.types, kind) + .cloned() + .unwrap_or(Stability::Unknown) + } else { + stability.clone() + } + } + fn resolve_type(&mut self, ty: &super::Type<'_>, stability: &Stability) -> Result { // Resources must be declared at the top level to have their methods // processed appropriately, but resources also shouldn't show up @@ -1302,12 +1364,13 @@ impl<'a> Resolver<'a> { _ => {} } let kind = self.resolve_type_def(ty, stability)?; + let stability = self.find_stability(&kind, stability); Ok(self.anon_type_def( TypeDef { kind, name: None, docs: Docs::default(), - stability: stability.clone(), + stability, owner: TypeOwner::None, }, ty.span(), @@ -1455,7 +1518,6 @@ impl<'a> Resolver<'a> { params: &ParamList<'_>, kind: &FunctionKind, span: Span, - stability: &Stability, ) -> Result { let mut ret = IndexMap::new(); match *kind { @@ -1467,11 +1529,13 @@ impl<'a> Resolver<'a> { // Methods automatically get a `self` initial argument so insert // that here before processing the normal parameters. FunctionKind::Method(id) => { + let kind = TypeDefKind::Handle(Handle::Borrow(id)); + let stability = self.find_stability(&kind, &Stability::Unknown); let shared = self.anon_type_def( TypeDef { docs: Docs::default(), - stability: stability.clone(), - kind: TypeDefKind::Handle(Handle::Borrow(id)), + stability, + kind, name: None, owner: TypeOwner::None, }, @@ -1481,7 +1545,10 @@ impl<'a> Resolver<'a> { } } for (name, ty) in params { - let prev = ret.insert(name.name.to_string(), self.resolve_type(ty, stability)?); + let prev = ret.insert( + name.name.to_string(), + self.resolve_type(ty, &Stability::Unknown)?, + ); if prev.is_some() { bail!(Error::new( name.span, @@ -1497,7 +1564,6 @@ impl<'a> Resolver<'a> { results: &ResultList<'_>, kind: &FunctionKind, span: Span, - stability: &Stability, ) -> Result { match *kind { // These kinds of methods don't have any adjustments to the return @@ -1508,9 +1574,10 @@ impl<'a> Resolver<'a> { rs, &FunctionKind::Freestanding, span, - stability, )?)), - ResultList::Anon(ty) => Ok(Results::Anon(self.resolve_type(ty, stability)?)), + ResultList::Anon(ty) => { + Ok(Results::Anon(self.resolve_type(ty, &Stability::Unknown)?)) + } } } diff --git a/crates/wit-parser/tests/ui/feature-types.wit b/crates/wit-parser/tests/ui/feature-types.wit new file mode 100644 index 0000000000..055d6b8aaa --- /dev/null +++ b/crates/wit-parser/tests/ui/feature-types.wit @@ -0,0 +1,13 @@ +package a:b; + +interface foo { + variant bar { + x, + y + } + + @unstable(feature = my-feature) + my-unstable: func(p: bar) -> result<_, bar>; + + my-stable: func(p: bar) -> result<_, bar>; +} diff --git a/crates/wit-parser/tests/ui/feature-types.wit.json b/crates/wit-parser/tests/ui/feature-types.wit.json new file mode 100644 index 0000000000..e8e5263a1b --- /dev/null +++ b/crates/wit-parser/tests/ui/feature-types.wit.json @@ -0,0 +1,70 @@ +{ + "worlds": [], + "interfaces": [ + { + "name": "foo", + "types": { + "bar": 0 + }, + "functions": { + "my-stable": { + "name": "my-stable", + "kind": "freestanding", + "params": [ + { + "name": "p", + "type": 0 + } + ], + "results": [ + { + "type": 1 + } + ] + } + }, + "package": 0 + } + ], + "types": [ + { + "name": "bar", + "kind": { + "variant": { + "cases": [ + { + "name": "x", + "type": null + }, + { + "name": "y", + "type": null + } + ] + } + }, + "owner": { + "interface": 0 + } + }, + { + "name": null, + "kind": { + "result": { + "ok": null, + "err": 0 + } + }, + "owner": null + } + ], + "packages": [ + { + "name": "a:b", + "interfaces": { + "foo": 0 + }, + "worlds": {} + } + ] +} \ No newline at end of file