Skip to content

Commit 931c62a

Browse files
katelyn martinPat Hickey
katelyn martin
and
Pat Hickey
authored
Fix path canonicalization in witx use statements (#434)
* 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. * Add some temporary printlns 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 🐛 🔍😃 * Fix the bug * Remove temproary printlns This reverts commit 07c1ae9. * oh no, windows paths * add review suggestion Co-authored-by: Pat Hickey <[email protected]> Co-authored-by: Pat Hickey <[email protected]>
1 parent 2c84c7b commit 931c62a

File tree

2 files changed

+36
-4
lines changed

2 files changed

+36
-4
lines changed

tools/witx/src/io.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,23 +52,23 @@ impl WitxIo for Filesystem {
5252
}
5353

5454
pub struct MockFs {
55-
map: HashMap<String, String>,
55+
map: HashMap<PathBuf, String>,
5656
}
5757

5858
impl MockFs {
5959
pub fn new(strings: &[(&str, &str)]) -> Self {
6060
MockFs {
6161
map: strings
6262
.iter()
63-
.map(|(k, v)| (k.to_string(), v.to_string()))
63+
.map(|(k, v)| (PathBuf::from(k), v.to_string()))
6464
.collect(),
6565
}
6666
}
6767
}
6868

6969
impl WitxIo for MockFs {
7070
fn fgets(&self, path: &Path) -> Result<String, WitxError> {
71-
if let Some(entry) = self.map.get(path.to_str().unwrap()) {
71+
if let Some(entry) = self.map.get(path) {
7272
Ok(entry.to_string())
7373
} else {
7474
Err(WitxError::Io(
@@ -78,7 +78,7 @@ impl WitxIo for MockFs {
7878
}
7979
}
8080
fn fget_line(&self, path: &Path, line: usize) -> Result<String, WitxError> {
81-
if let Some(entry) = self.map.get(path.to_str().unwrap()) {
81+
if let Some(entry) = self.map.get(path) {
8282
entry
8383
.lines()
8484
.skip(line - 1)

tools/witx/src/toplevel.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ fn parse_file(
6666
.map_err(WitxError::Validation)?;
6767
}
6868
TopLevelSyntax::Use(u) => {
69+
let root = path.parent().unwrap_or(root);
6970
parse_file(u.as_ref(), io, root, validator, definitions, parsed)?;
7071
}
7172
}
@@ -138,6 +139,37 @@ mod test {
138139
);
139140
}
140141

142+
#[test]
143+
fn multi_use_with_layered_dirs() {
144+
let doc = parse_witx_with(
145+
&[Path::new("/root.witx")],
146+
&MockFs::new(&[
147+
("/root.witx", "(use \"subdir/child.witx\")"),
148+
(
149+
"/subdir/child.witx",
150+
"(use \"sibling.witx\")\n(typename $b_float f64)",
151+
),
152+
("/subdir/sibling.witx", "(typename $c_int u32)"),
153+
// This definition looks just like subdir/sibling.witx but
154+
// defines c_int differently - this test shows it does Not get
155+
// included by subdir/child.witx's use.
156+
("/sibling.witx", "(typename $c_int u64)"),
157+
]),
158+
)
159+
.expect("parse");
160+
161+
let b_float = doc.typename(&Id::new("b_float")).unwrap();
162+
assert_eq!(**b_float.type_(), Type::Builtin(BuiltinType::F64));
163+
164+
let c_int = doc.typename(&Id::new("c_int")).unwrap();
165+
assert_eq!(
166+
**c_int.type_(),
167+
Type::Builtin(BuiltinType::U32 {
168+
lang_ptr_size: false
169+
})
170+
);
171+
}
172+
141173
#[test]
142174
fn use_not_found() {
143175
match parse_witx_with(&[Path::new("/a")], &MockFs::new(&[("/a", "(use \"b\")")]))

0 commit comments

Comments
 (0)