From 77aa6eb9ba8c930d6669e071c9c50ef8c20297c6 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 23 Mar 2021 08:26:21 -0700 Subject: [PATCH] Enable more than one `handle` type in `*.witx` This commit reworks how handle types work at the representation level in `*.witx` file to follow the expected design of resources in the interface types specification. Previously handle types were simply `(handle)`, which meant that there could only be one actual handle type in the world (structurally at least). After this commit, however, there can be multiple handle types. First abstract types must be introduced as a `resource`, for example: (resource $fd) This declares that the module exports a type named `$fd` and it's abstract in that the representation is not known. At the interface layer you can't pass an `$fd` directly because it's representation is not known. To do that, however, you can do: (param $x (handle $fd)) The `handle` type now refers to a particular `resource` that it refers to. Values of type `handle T` can exist and are what's passed at the boundaries. This is all largely just an internal structuring concern at this point. This has no ramifications for WASI which still has an `$fd` type for functions that is represented with an `i32`. This is largely a forward-looking change to allow multiple types of resources defined by different modules and all used by one another. This commit also updates `use` syntax where `use` will pull from either the type or the resource namespace. Furthermore a new `(use ($foo as $bar) ...)` syntax was added to locally renamed something within a module. --- phases/ephemeral/witx/typenames.witx | 3 +- phases/old/snapshot_0/witx/typenames.witx | 3 +- phases/snapshot/witx/typenames.witx | 3 +- tools/witx/src/ast.rs | 53 +++++++++++- tools/witx/src/parser.rs | 58 +++++++++++-- tools/witx/src/toplevel.rs | 15 ---- tools/witx/src/validate.rs | 92 +++++++++++++++++---- tools/witx/tests/witxt.rs | 10 ++- tools/witx/tests/witxt/abi.witxt | 6 +- tools/witx/tests/witxt/anonymous.witxt | 3 +- tools/witx/tests/witxt/resources.witxt | 41 +++++++++ tools/witx/tests/witxt/resources/multi.witx | 11 +++ tools/witx/tests/witxt/resources/other.witx | 2 + 13 files changed, 252 insertions(+), 48 deletions(-) create mode 100644 tools/witx/tests/witxt/resources.witxt create mode 100644 tools/witx/tests/witxt/resources/multi.witx create mode 100644 tools/witx/tests/witxt/resources/other.witx diff --git a/phases/ephemeral/witx/typenames.witx b/phases/ephemeral/witx/typenames.witx index bbd6489df..12b60bc1f 100644 --- a/phases/ephemeral/witx/typenames.witx +++ b/phases/ephemeral/witx/typenames.witx @@ -281,8 +281,9 @@ ) ) +(resource $fd) ;;; A file descriptor handle. -(typename $fd (handle)) +(typename $fd (handle $fd)) ;;; A region of memory for scatter/gather reads. (typename $iovec diff --git a/phases/old/snapshot_0/witx/typenames.witx b/phases/old/snapshot_0/witx/typenames.witx index f4ba78802..a57a6cdd0 100644 --- a/phases/old/snapshot_0/witx/typenames.witx +++ b/phases/old/snapshot_0/witx/typenames.witx @@ -273,8 +273,9 @@ ) ) +(resource $fd) ;;; A file descriptor handle. -(typename $fd (handle)) +(typename $fd (handle $fd)) ;;; A region of memory for scatter/gather reads. (typename $iovec diff --git a/phases/snapshot/witx/typenames.witx b/phases/snapshot/witx/typenames.witx index 311b42233..b50ff3474 100644 --- a/phases/snapshot/witx/typenames.witx +++ b/phases/snapshot/witx/typenames.witx @@ -273,8 +273,9 @@ ) ) +(resource $fd) ;;; A file descriptor handle. -(typename $fd (handle)) +(typename $fd (handle $fd)) ;;; A region of memory for scatter/gather reads. (typename $iovec diff --git a/tools/witx/src/ast.rs b/tools/witx/src/ast.rs index 1f042244e..b36bb8a25 100644 --- a/tools/witx/src/ast.rs +++ b/tools/witx/src/ast.rs @@ -45,6 +45,9 @@ pub struct Module { types: Vec>, type_map: HashMap>, + resources: Vec>, + resource_map: HashMap>, + funcs: Vec>, func_map: HashMap>, @@ -61,6 +64,8 @@ impl Module { module_id, types: Default::default(), type_map: Default::default(), + resources: Default::default(), + resource_map: Default::default(), funcs: Default::default(), func_map: Default::default(), constants: Default::default(), @@ -80,6 +85,14 @@ impl Module { self.types.push(ty); } + pub(crate) fn push_resource(&mut self, r: Rc) { + assert!(self + .resource_map + .insert(r.name.clone(), r.clone()) + .is_none()); + self.resources.push(r); + } + pub(crate) fn push_func(&mut self, func: Rc) { assert!(self .func_map @@ -100,6 +113,14 @@ impl Module { self.types.iter() } + pub fn resource(&self, name: &Id) -> Option> { + self.resource_map.get(name).cloned() + } + + pub fn resources<'a>(&'a self) -> impl Iterator> + 'a { + self.resources.iter() + } + /// All of the (unique) types used as "err" variant of results returned from /// functions. pub fn error_types<'a>(&'a self) -> impl Iterator + 'a { @@ -499,11 +520,37 @@ impl Case { } #[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub struct HandleDatatype {} +pub struct Resource { + /// The local name within the module this resource is defined within. This + /// may differ from the id of the resource itself. + pub name: Id, + /// The unique id assigned to this resource. + pub resource_id: ResourceId, + /// Documentation in the defining module, if any. + pub docs: String, +} + +/// A unique id used to determine whether two handles are nominally referring +/// to the same resource. +/// +/// An id is composed of the definition location (a module id) and the original +/// name within that module. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct ResourceId { + pub name: Id, + pub module_id: ModuleId, +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct HandleDatatype { + /// The resource that this handle references, used for determining if two + /// handle types are nominally equal to one another. + pub resource_id: ResourceId, +} impl HandleDatatype { - pub fn type_equal(&self, _other: &HandleDatatype) -> bool { - true + pub fn type_equal(&self, other: &HandleDatatype) -> bool { + self.resource_id == other.resource_id } } diff --git a/tools/witx/src/parser.rs b/tools/witx/src/parser.rs index 97dde4d78..6633b35f3 100644 --- a/tools/witx/src/parser.rs +++ b/tools/witx/src/parser.rs @@ -34,11 +34,13 @@ mod kw { wast::custom_keyword!(noreturn); wast::custom_keyword!(pointer); wast::custom_keyword!(record); + wast::custom_keyword!(r#as = "as"); wast::custom_keyword!(r#const = "const"); wast::custom_keyword!(r#enum = "enum"); wast::custom_keyword!(r#union = "union"); wast::custom_keyword!(r#use = "use"); wast::custom_keyword!(repr); + wast::custom_keyword!(resource); wast::custom_keyword!(s16); wast::custom_keyword!(s32); wast::custom_keyword!(s64); @@ -227,6 +229,7 @@ impl<'a> Parse<'a> for TopLevelModule<'a> { if parser.peek2::() || parser.peek2::() || parser.peek2::() + || parser.peek2::() { decls.push(Documented { comments, @@ -279,6 +282,7 @@ impl<'a> Parse<'a> for TopLevelSyntax<'a> { #[derive(Debug, Clone)] pub enum DeclSyntax<'a> { Typename(TypenameSyntax<'a>), + Resource(ResourceSyntax<'a>), Const(Documented<'a, ConstSyntax<'a>>), } @@ -289,6 +293,8 @@ impl<'a> Parse<'a> for DeclSyntax<'a> { Ok(DeclSyntax::Typename(parser.parse()?)) } else if l.peek::() { Ok(DeclSyntax::Const(parser.parse()?)) + } else if l.peek::() { + Ok(DeclSyntax::Resource(parser.parse()?)) } else { Err(l.error()) } @@ -313,7 +319,7 @@ impl<'a> Parse<'a> for UseSyntax<'a> { #[derive(Debug, Clone, PartialEq, Eq)] pub enum UsedNames<'a> { - List(Vec>), + List(Vec>), All(wast::Span), } @@ -333,6 +339,32 @@ impl<'a> Parse<'a> for UsedNames<'a> { } } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct UseName<'a> { + pub other_name: wast::Id<'a>, + pub our_name: wast::Id<'a>, +} + +impl<'a> Parse<'a> for UseName<'a> { + fn parse(parser: Parser<'a>) -> Result { + let (other_name, our_name) = if parser.peek::() { + let name = parser.parse()?; + (name, name) + } else { + parser.parens(|p| { + let other_name = p.parse()?; + p.parse::()?; + let our_name = p.parse()?; + Ok((other_name, our_name)) + })? + }; + Ok(UseName { + other_name, + our_name, + }) + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct TypenameSyntax<'a> { pub ident: wast::Id<'a>, @@ -357,7 +389,7 @@ pub enum TypedefSyntax<'a> { Record(RecordSyntax<'a>), Union(UnionSyntax<'a>), Variant(VariantSyntax<'a>), - Handle(HandleSyntax), + Handle(HandleSyntax<'a>), List(Box>), Pointer(Box>), ConstPointer(Box>), @@ -521,6 +553,19 @@ impl<'a> Parse<'a> for ConstSyntax<'a> { } } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ResourceSyntax<'a> { + pub ident: wast::Id<'a>, +} + +impl<'a> Parse<'a> for ResourceSyntax<'a> { + fn parse(parser: Parser<'a>) -> Result { + parser.parse::()?; + let ident = parser.parse()?; + Ok(ResourceSyntax { ident }) + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct FlagsSyntax<'a> { pub repr: Option, @@ -656,12 +701,15 @@ impl<'a> Parse<'a> for CaseSyntax<'a> { } #[derive(Debug, Clone, PartialEq, Eq)] -pub struct HandleSyntax {} +pub struct HandleSyntax<'a> { + pub resource: wast::Id<'a>, +} -impl<'a> Parse<'a> for HandleSyntax { +impl<'a> Parse<'a> for HandleSyntax<'a> { fn parse(parser: Parser<'a>) -> Result { parser.parse::()?; - Ok(HandleSyntax {}) + let resource = parser.parse()?; + Ok(HandleSyntax { resource }) } } diff --git a/tools/witx/src/toplevel.rs b/tools/witx/src/toplevel.rs index 9a5f10392..57d50755e 100644 --- a/tools/witx/src/toplevel.rs +++ b/tools/witx/src/toplevel.rs @@ -165,19 +165,4 @@ mod test { e => panic!("wrong error: {:?}", e), } } - - #[test] - fn use_invalid() { - match parse_witx_with("/a", &MockFs::new(&[("/a", "(use bbbbbbb)")])) - .err() - .unwrap() - { - WitxError::Parse(e) => { - let err = e.to_string(); - assert!(err.contains("expected an identifier"), "bad error: {}", err); - assert!(err.contains("/a:1:6")); - } - e => panic!("wrong error: {:?}", e), - } - } } diff --git a/tools/witx/src/validate.rs b/tools/witx/src/validate.rs index c8ad706cd..cc7b67a5a 100644 --- a/tools/witx/src/validate.rs +++ b/tools/witx/src/validate.rs @@ -6,7 +6,8 @@ use crate::{ VariantSyntax, }, Abi, BuiltinType, Case, Constant, Function, HandleDatatype, Id, IntRepr, Location, Module, - ModuleId, NamedType, Param, RecordDatatype, RecordKind, RecordMember, Type, TypeRef, Variant, + ModuleId, NamedType, Param, RecordDatatype, RecordKind, RecordMember, Resource, ResourceId, + Type, TypeRef, Variant, }; use std::collections::{HashMap, HashSet}; use std::path::Path; @@ -131,6 +132,7 @@ impl IdentValidation { pub struct ModuleValidation<'a> { module: Module, type_ns: IdentValidation, + resource_ns: IdentValidation, func_ns: IdentValidation, constant_ns: HashMap, bool_ty: TypeRef, @@ -145,6 +147,7 @@ impl<'a> ModuleValidation<'a> { Self { module: Module::new(name, module_id), type_ns: IdentValidation::new(), + resource_ns: IdentValidation::new(), func_ns: IdentValidation::new(), constant_ns: HashMap::new(), bool_ty: TypeRef::Value(Rc::new(Type::Variant(Variant { @@ -193,22 +196,59 @@ impl<'a> ModuleValidation<'a> { self.type_ns.introduce(ty.name.as_str(), loc)?; self.module.push_type(ty.clone()); } + for r in module.resources() { + let loc = self.location(span); + self.resource_ns.introduce(r.name.as_str(), loc)?; + self.module.push_resource(r.clone()); + } } UsedNames::List(names) => { for name in names { - let loc = self.location(name.span()); - let id = self.type_ns.introduce(name.name(), loc)?; - let ty = match module.typename(&id) { - Some(ty) => ty, - None => { - return Err(ValidationError::UnknownName { - name: name.name().to_string(), - location: self.location(name.span()), - } - .into()); + let mut used = false; + let id = Id::new(name.other_name.name()); + let other_loc = self.location(name.other_name.span()); + let our_loc = self.location(name.our_name.span()); + + if let Some(ty) = module.typename(&id) { + let id = self + .type_ns + .introduce(name.our_name.name(), our_loc.clone())?; + let ty = if name.other_name.name() == name.our_name.name() { + ty + } else { + Rc::new(NamedType { + name: id, + module: self.module.module_id().clone(), + tref: TypeRef::Name(ty), + docs: String::new(), + }) + }; + self.module.push_type(ty); + used = true; + } + + if let Some(r) = module.resource(&id) { + let id = self.resource_ns.introduce(name.our_name.name(), our_loc)?; + let r = if name.other_name.name() == name.our_name.name() { + r + } else { + Rc::new(Resource { + name: id, + resource_id: r.resource_id.clone(), + docs: String::new(), + }) + }; + self.module.push_resource(r); + used = true; + } + + if !used { + return Err(ValidationError::UnknownName { + name: name.other_name.name().to_string(), + location: other_loc, } - }; - self.module.push_type(ty); + .into()); + } } } } @@ -228,13 +268,28 @@ impl<'a> ModuleValidation<'a> { let tref = self.validate_datatype(&decl.def, true, decl.ident.span())?; self.module.push_type(Rc::new(NamedType { - name: name.clone(), + name, module: self.module.module_id().clone(), tref, docs, })); } + DeclSyntax::Resource(decl) => { + let loc = self.location(decl.ident.span()); + let name = self.resource_ns.introduce(decl.ident.name(), loc)?; + let docs = comments.docs(); + + self.module.push_resource(Rc::new(Resource { + name: name.clone(), + resource_id: ResourceId { + name, + module_id: self.module.module_id().clone(), + }, + docs, + })); + } + DeclSyntax::Const(syntax) => { let ty = Id::new(syntax.item.ty.name()); let loc = self.location(syntax.item.name.span()); @@ -631,10 +686,15 @@ impl<'a> ModuleValidation<'a> { fn validate_handle( &self, - _syntax: &HandleSyntax, + syntax: &HandleSyntax, _span: wast::Span, ) -> Result { - Ok(HandleDatatype {}) + let loc = self.location(syntax.resource.span()); + let name = self.resource_ns.get(syntax.resource.name(), loc)?; + let resource = self.module.resource(&name).unwrap(); + Ok(HandleDatatype { + resource_id: resource.resource_id.clone(), + }) } fn validate_int_repr( diff --git a/tools/witx/tests/witxt.rs b/tools/witx/tests/witxt.rs index 753cac444..c74e34679 100644 --- a/tools/witx/tests/witxt.rs +++ b/tools/witx/tests/witxt.rs @@ -530,19 +530,23 @@ impl Witx<'_> { let mut validator = witx::ModuleValidation::new(contents, file); for t in doc.decls.iter() { match &t.item { - TopLevelSyntax::Decl(d) => validator.validate_decl(&d, &t.comments)?, + TopLevelSyntax::Decl(d) => validator + .validate_decl(&d, &t.comments) + .map_err(|e| anyhow::anyhow!("{}", e.report()))?, TopLevelSyntax::Use(_) => unimplemented!(), } } for f in doc.functions.iter() { - validator.validate_function(&f.item, &f.comments)?; + validator + .validate_function(&f.item, &f.comments) + .map_err(|e| anyhow::anyhow!("{}", e.report()))?; } Ok(validator.into_module()) } WitxDef::Fs(path) => { let parent = file.parent().unwrap(); let path = parent.join(path); - Ok(witx::load(&path)?) + Ok(witx::load(&path).map_err(|e| anyhow::anyhow!("{}", e.report()))?) } } } diff --git a/tools/witx/tests/witxt/abi.witxt b/tools/witx/tests/witxt/abi.witxt index ccdad7bf4..913e6edb2 100644 --- a/tools/witx/tests/witxt/abi.witxt +++ b/tools/witx/tests/witxt/abi.witxt @@ -131,7 +131,8 @@ ;; handle parameter (assert_abi (witx - (typename $a (handle)) + (resource $a) + (typename $a (handle $a)) (module $x (@interface func (export "f") (param $p $a))) ) (wasm (param i32)) @@ -284,7 +285,8 @@ ;; handle return (assert_abi (witx - (typename $a (handle)) + (resource $a) + (typename $a (handle $a)) (module $x (@interface func (export "f") (result $p $a))) ) (wasm (result i32)) diff --git a/tools/witx/tests/witxt/anonymous.witxt b/tools/witx/tests/witxt/anonymous.witxt index 0ed3bc0d2..4ff479712 100644 --- a/tools/witx/tests/witxt/anonymous.witxt +++ b/tools/witx/tests/witxt/anonymous.witxt @@ -27,7 +27,8 @@ (assert_invalid (witx - (typename $a (@witx pointer (handle))) + (resource $x) + (typename $a (@witx pointer (handle $x))) ) "Anonymous structured types") diff --git a/tools/witx/tests/witxt/resources.witxt b/tools/witx/tests/witxt/resources.witxt new file mode 100644 index 000000000..fb991903a --- /dev/null +++ b/tools/witx/tests/witxt/resources.witxt @@ -0,0 +1,41 @@ +(assert_invalid + (witx (typename $x (handle $y))) + "Unknown name `y`") + +(assert_invalid + (witx + (typename $y u32) + (typename $x (handle $y))) + "Unknown name `y`") + +(assert_invalid + (witx + (resource $x) + (resource $x)) + "Redefinition of name `x`") + +(witx + (resource $x) + (typename $x (handle $x))) + +(witx $a + (resource $x) + (typename $x (handle $x)) + (typename $y (handle $x)) +) + +(assert_eq $a "x" $a "y") + +(witx $a + (resource $x) + (resource $y) + (typename $x (handle $x)) + (typename $y (handle $y)) +) + +(assert_ne $a "x" $a "y") + +(witx $a (load "resources/multi.witx")) + +(assert_eq $a "x1" $a "x2") +(assert_ne $a "y1" $a "y2") diff --git a/tools/witx/tests/witxt/resources/multi.witx b/tools/witx/tests/witxt/resources/multi.witx new file mode 100644 index 000000000..105fef364 --- /dev/null +++ b/tools/witx/tests/witxt/resources/multi.witx @@ -0,0 +1,11 @@ +(use ($x as $other_x) ($y as $other_y) from $other) + +;; this resource named `y` shouldn't be the same as another resource named `y` +;; defined elsewhere. +(resource $y) + +(typename $x1 (handle $other_x)) +(typename $x2 (handle $other_x)) + +(typename $y1 (handle $other_y)) +(typename $y2 (handle $y)) diff --git a/tools/witx/tests/witxt/resources/other.witx b/tools/witx/tests/witxt/resources/other.witx new file mode 100644 index 000000000..7de20bc3d --- /dev/null +++ b/tools/witx/tests/witxt/resources/other.witx @@ -0,0 +1,2 @@ +(resource $x) +(resource $y)