From 03a7246b9620aa5351b2f6656b26f44ab577e0b9 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Fri, 18 Jun 2021 13:01:16 -0400 Subject: [PATCH 1/6] Add a (failing) test case This test case demonstrates a bug identified with the semantics of `use` statements in witx documents. No fix is provided in this commit, so `cargo test` will fail at this commit. --- tools/witx/src/toplevel.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tools/witx/src/toplevel.rs b/tools/witx/src/toplevel.rs index 257e6edd..ed3b44c7 100644 --- a/tools/witx/src/toplevel.rs +++ b/tools/witx/src/toplevel.rs @@ -138,6 +138,33 @@ mod test { ); } + #[test] + fn multi_use_with_layered_dirs() { + let doc = parse_witx_with( + &[Path::new("/root.witx")], + &MockFs::new(&[ + ("/root.witx", "(use \"subdir/child.witx\")"), + ( + "/subdir/child.witx", + "(use \"sibling.witx\")\n(typename $b_float f64)", + ), + ("/subdir/sibling.witx", "(typename $c_int u32)"), + ]), + ) + .expect("parse"); + + let b_float = doc.typename(&Id::new("b_float")).unwrap(); + assert_eq!(**b_float.type_(), Type::Builtin(BuiltinType::F64)); + + let c_int = doc.typename(&Id::new("c_int")).unwrap(); + assert_eq!( + **c_int.type_(), + Type::Builtin(BuiltinType::U32 { + lang_ptr_size: false + }) + ); + } + #[test] fn use_not_found() { match parse_witx_with(&[Path::new("/a")], &MockFs::new(&[("/a", "(use \"b\")")])) From 8c1d9c56a0e5c177404f01155774b0cbb0a089d9 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Fri, 18 Jun 2021 16:07:37 -0400 Subject: [PATCH 2/6] Add some temporary printlns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because who doesn't love println debugging? Now, running the (failing) test case added in the previous commit, we'll see: ``` ---- toplevel::test::multi_use_with_layered_dirs stdout ---- parsing file at path='"root.witx"' root='"/"' canonical-path='"/root.witx"' found a use statement, parsing file at "subdir/child.witx" parsing file at path='"subdir/child.witx"' root='"/"' canonical-path='"/subdir/child.witx"' found a use statement, parsing file at "sibling.witx" parsing file at path='"sibling.witx"' root='"/"' canonical-path='"/sibling.witx"' thread 'toplevel::test::multi_use_with_layered_dirs' panicked at 'parse: Io("/sibling.witx", Custom { kind: Other, error: "mock fs: file not found" })', src/toplevel.rs:151:10 ``` Note that root is always `/` even when we're evaluating the paths found in a document that lives in a subdirectory! See how the canonical path in the last block resolves to '/sibling.witx'? We found you, Mr. Bug 🐛 🔍😃 --- tools/witx/src/toplevel.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/witx/src/toplevel.rs b/tools/witx/src/toplevel.rs index ed3b44c7..423986da 100644 --- a/tools/witx/src/toplevel.rs +++ b/tools/witx/src/toplevel.rs @@ -43,11 +43,14 @@ fn parse_file( definitions: &mut Vec, parsed: &mut HashSet, ) -> Result<(), WitxError> { + eprintln!("parsing file at path='{:?}' root='{:?}'", &path, &root); let path = io.canonicalize(&root.join(path))?; + eprintln!("canonical-path='{:?}'", &path); if !parsed.insert(path.clone()) { return Ok(()); } let input = io.fgets(&path)?; + eprintln!(""); // this one is just for space let adjust_err = |mut error: wast::Error| { error.set_path(&path); @@ -66,6 +69,7 @@ fn parse_file( .map_err(WitxError::Validation)?; } TopLevelSyntax::Use(u) => { + eprintln!("found a use statement, parsing file at {:?}", &u); parse_file(u.as_ref(), io, root, validator, definitions, parsed)?; } } From 9931cf74fff166743de7f30461c2282dffaf9703 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Fri, 18 Jun 2021 16:18:07 -0400 Subject: [PATCH 3/6] Fix the bug --- tools/witx/src/toplevel.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/witx/src/toplevel.rs b/tools/witx/src/toplevel.rs index 423986da..95368121 100644 --- a/tools/witx/src/toplevel.rs +++ b/tools/witx/src/toplevel.rs @@ -70,6 +70,7 @@ fn parse_file( } TopLevelSyntax::Use(u) => { eprintln!("found a use statement, parsing file at {:?}", &u); + let root = path.parent().unwrap_or(root); parse_file(u.as_ref(), io, root, validator, definitions, parsed)?; } } From acb808e84978f3a85ae1cdb1e367c3dccb507944 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Fri, 18 Jun 2021 16:18:55 -0400 Subject: [PATCH 4/6] Remove temproary printlns This reverts commit 07c1ae9dc3f59cd1994786e0aaadda0ccb0bdcb3. --- tools/witx/src/toplevel.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tools/witx/src/toplevel.rs b/tools/witx/src/toplevel.rs index 95368121..e4150656 100644 --- a/tools/witx/src/toplevel.rs +++ b/tools/witx/src/toplevel.rs @@ -43,14 +43,11 @@ fn parse_file( definitions: &mut Vec, parsed: &mut HashSet, ) -> Result<(), WitxError> { - eprintln!("parsing file at path='{:?}' root='{:?}'", &path, &root); let path = io.canonicalize(&root.join(path))?; - eprintln!("canonical-path='{:?}'", &path); if !parsed.insert(path.clone()) { return Ok(()); } let input = io.fgets(&path)?; - eprintln!(""); // this one is just for space let adjust_err = |mut error: wast::Error| { error.set_path(&path); @@ -69,7 +66,6 @@ fn parse_file( .map_err(WitxError::Validation)?; } TopLevelSyntax::Use(u) => { - eprintln!("found a use statement, parsing file at {:?}", &u); let root = path.parent().unwrap_or(root); parse_file(u.as_ref(), io, root, validator, definitions, parsed)?; } From d2fc21f274cd8ccf158ded2c9c7ad05990c1e732 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Mon, 21 Jun 2021 17:43:57 -0400 Subject: [PATCH 5/6] oh no, windows paths --- tools/witx/src/io.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/witx/src/io.rs b/tools/witx/src/io.rs index f46ab176..a38a22d8 100644 --- a/tools/witx/src/io.rs +++ b/tools/witx/src/io.rs @@ -52,7 +52,7 @@ impl WitxIo for Filesystem { } pub struct MockFs { - map: HashMap, + map: HashMap, } impl MockFs { @@ -60,7 +60,7 @@ impl MockFs { MockFs { map: strings .iter() - .map(|(k, v)| (k.to_string(), v.to_string())) + .map(|(k, v)| (PathBuf::from(k), v.to_string())) .collect(), } } @@ -68,7 +68,7 @@ impl MockFs { impl WitxIo for MockFs { fn fgets(&self, path: &Path) -> Result { - if let Some(entry) = self.map.get(path.to_str().unwrap()) { + if let Some(entry) = self.map.get(path) { Ok(entry.to_string()) } else { Err(WitxError::Io( @@ -78,7 +78,7 @@ impl WitxIo for MockFs { } } fn fget_line(&self, path: &Path, line: usize) -> Result { - if let Some(entry) = self.map.get(path.to_str().unwrap()) { + if let Some(entry) = self.map.get(path) { entry .lines() .skip(line - 1) From 5148a1bbf2dc3d741b0e97d1a03da68e4c2087be Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Tue, 22 Jun 2021 10:37:45 -0400 Subject: [PATCH 6/6] add review suggestion Co-authored-by: Pat Hickey --- tools/witx/src/toplevel.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/witx/src/toplevel.rs b/tools/witx/src/toplevel.rs index e4150656..94dbd132 100644 --- a/tools/witx/src/toplevel.rs +++ b/tools/witx/src/toplevel.rs @@ -150,6 +150,10 @@ mod test { "(use \"sibling.witx\")\n(typename $b_float f64)", ), ("/subdir/sibling.witx", "(typename $c_int u32)"), + // This definition looks just like subdir/sibling.witx but + // defines c_int differently - this test shows it does Not get + // included by subdir/child.witx's use. + ("/sibling.witx", "(typename $c_int u64)"), ]), ) .expect("parse");