Fix path canonicalization in witx use statements

[cratelyn]

🦇 ✨ Hello!

This branch fixes a bug I have found in the witx crate, involving use statements.

🐛 The Bug

Paths in ..transitive(?) use statements (e.g. document A imports document B, which imports document C) are evaluated relative to the original root document, rather than the import-er document B.

This is better understood with an example (see below), or consulting the test case added here, multi_use_with_layered_dirs. Note that unlike the existing multi_use test, these documents do not all live within the same directory in the file system.

📜 Example

;; /root.witx
(use "subdir/child.witx")

;; /subdir/child.witx"
(use "sibling.witx")
(typename $b_float f64)

;; /subdir/sibling.witx
(typename $c_int u32)

Today, when root.witx is loaded, we'll run into an error that /sibling.witx does not exist.

☭ The Fix

The fix for this turns out to be a single line of code! That can be found in 963c5cf.

It turns out that when we were recursing in witx::toplevel::parse_file upon the discovery of a use statement, we'd pass the original root along, rather than providing the directory of the given document.

[cratelyn]

Gah. I had an old main branch, I'll have to fix this up a bit.

[cratelyn]

I have updated this branch so that it is based on #395, which I believe is the commit that corresponds with the 0.9 version of the witx crate on crates.io.

Unfortunately, this work conflicts with changes that introduced a new use syntax in #415. I spoke with @pchickey about this last week, and if I understand correctly, this work was experimental and is not likely to see public release.

So, in order for this work to make its way into a 0.9.1 release of witx, I believe I'd first need to submit a preliminary PR elsewhere that reverts #415. That PR was a substantive change however, so before I do so I'd like to confirm with other maintainers that this is the correct route forward. (cc: @alexcrichton)

I thank you in advance for your time and assistance 💌

[alexcrichton]

Thanks for the fix! I agree yeah that we should revert the main branch back to what it was at the time of the latest crates.io release. I had plans for more work but they got sidetracked and sidelined due to other priorities and other avenues, so yeah let's revert what's not published and then we can merge the bug fix.

Are you ok submitting the reverts? I can take a quick look and approve them/this.

[cratelyn]

I've just pushed #435, which should unblock this patch 🙂

I'll note that I've not updated the base branch here, because I'm not sure how well that plays across forks.

[alexcrichton]

I think this may need a rebase now with the recent merge, but tests may also need some updates?

[cratelyn]

I think this may need a rebase now with the recent merge, but tests may also need some updates?

Indeed! I've rebased this branch. Thank you for the quick response!

I'll fix tests up next 🙂

[cratelyn]

The tests have been satiated ☺️

[pchickey]

This looks great, suggest adding one more check to the tests (which it passes!)

diff --git a/tools/witx/src/toplevel.rs b/tools/witx/src/toplevel.rs
index e415065..94dbd13 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");

[cratelyn]

This looks great, suggest adding one more check to the tests (which it passes!)

Great idea! I've added that in 5148a1b. ✨

[alexcrichton]

It looks like the crate was last published at 7ec4b1a (diff from main), and if we'd like to make an 0.9.1 release (ideally just a bugfix release), I think there's another breaking change we merged in the meantime, notably the deletion of tools/witx/src/phases.rs, which I think happened as #398. I think we may need to revert that as well before publishing 0.9.1?

[cratelyn]

It looks like the crate was last published at 7ec4b1a (diff from main), and if we'd like to make an 0.9.1 release (ideally just a bugfix release), I think there's another breaking change we merged in the meantime, notably the deletion of tools/witx/src/phases.rs, which I think happened as #398. I think we may need to revert that as well before publishing 0.9.1?

Great catch, thank you. I've just opened #436, which should address this point.