WebAssembly / WASI

WebAssembly System Interface
Other
4.81k stars 249 forks source link

Fix path canonicalization in witx `use` statements #434

Closed cratelyn closed 3 years ago

cratelyn commented 3 years ago

:bat: :sparkles: Hello!

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

:bug: 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.

:scroll: 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 commented 3 years ago

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

cratelyn commented 3 years ago

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 :love_letter:

alexcrichton commented 3 years ago

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 commented 3 years ago

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

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

alexcrichton commented 3 years ago

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

cratelyn commented 3 years ago

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 :slightly_smiling_face:

cratelyn commented 3 years ago

The tests have been satiated :relaxed:

cratelyn commented 3 years ago

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

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

alexcrichton commented 3 years ago

It looks like the crate was last published at 7ec4b1a65621ed3dd98fb67ab3746b4f29b4a62b (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 https://github.com/WebAssembly/WASI/pull/398. I think we may need to revert that as well before publishing 0.9.1?

cratelyn commented 3 years ago

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.