Refactor how liveness of imports is calculated #2115
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In `wit-component` there's a pass when creating a component to prune types/functions/etc from imported interfaces if they're not actually needed by the final component. This logic has not stood well against the test of time and the loop nowadays looks like: for each import if it's found within the imports then mark it live. This logic had some special handling of resource intrinsics but did not take into account any async/stream/future-related intrinsics. To fix this issue I've replaced the outdated loop with "for all module imports add them, as necessary, to the set of live types". This starts with the source of truth, module imports, and works backwards to WIT types as opposed to the other way around, starting with WIT types and testing to see if it's included in the intrinsics list. That fixes the various cases with async intrinsics and should ensure that all future intrinsics are also accounted for. Closes bytecodealliance/wasip3-prototyping#82
alexcrichton
commented
Mar 27, 2025
Comment on lines
+52
to
+64
| if u.arbitrary()? { | ||
| let mut dst = Module::default(); | ||
| let mut reencode = RemoveImports { | ||
| u, | ||
| removed_funcs: 0, | ||
| }; | ||
| if reencode | ||
| .parse_core_module(&mut dst, Default::default(), &dummy) | ||
| .is_ok() | ||
| { | ||
| dummy = dst.finish(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'll note that this update to the fuzzer did not actually turn up this bug after fuzzing for ~1 hour on 30 cores. My best guess is that the shape of this bug is pretty specific so it's relatively rare to come up, but nonetheless I'm hoping that this stresses the optional import logic of wit-component.
dicej
approved these changes
Mar 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In
wit-componentthere's a pass when creating a component to prune types/functions/etc from imported interfaces if they're not actually needed by the final component. This logic has not stood well against the test of time and the loop nowadays looks like: for each import if it's found within the imports then mark it live. This logic had some special handling of resource intrinsics but did not take into account any async/stream/future-related intrinsics.To fix this issue I've replaced the outdated loop with "for all module imports add them, as necessary, to the set of live types". This starts with the source of truth, module imports, and works backwards to WIT types as opposed to the other way around, starting with WIT types and testing to see if it's included in the intrinsics list. That fixes the various cases with async intrinsics and should ensure that all future intrinsics are also accounted for.
Closes bytecodealliance/wasip3-prototyping#82