Reimplement wac-parser on wac-graph.
#82
Merged
Conversation
This file contains 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
This commit is a complete reimplementation of `wac-parser`'s resolution to be based on `wac-graph`. It also contains an overhaul of the `wac-graph` API to facilitate its use from `wac-parser`: * Reimplements `CompositionGraph` on top of `petgraph::StableDiGraph`, which eliminates needing to keep track of nodes internally to `CompositionGraph`. * Packages now populate their type information into the graph's types collection rather than having their own collections. * Splits the `Error` enum into discrete error types for each graph operation; this allows the AST resolver to properly attach span information to errors. * Add type dependency edges for defined types, ensuring topological ordering. * Fix encoding of defined type aliases so that each encoded alias refers to the export index of the previously encoded type. * Added a debug formatter for `CompositionGraph` that outputs a DOT representation (used in resolution tests as well). * Graph encoding now includes an optional producers custom section. * Fixed toposort to be in ascending node index order for independent nodes. The subtype checker implementation was reverted to the previous implementation, where `invert` and `revert` are called at specific points to change the type of check being performed; the newer implementation did not give the correct "expected, found" error messages for AST resolution. `wac resolve` now outputs a DOT representation of the resolved composition graph instead of JSON. Most of the deleted code comes from code that has already moved into `wac-types` and `wac-graph`. Closes bytecodealliance#79. Fixes bytecodealliance#78. Fixes bytecodealliance#76.
|
When this and a fix for #81 lands, I plan on adding a publish script to CI and creating a |
peterhuene
force-pushed
the
update-parser
branch
from
fcf76f4
to
0970c6d
Compare
rylev
requested changes
Apr 15, 2024
There was a problem hiding this comment.
I'm seeing a stack overflow when running this against my own project. It seems that for some reason, a particular interface that we're merging into the types collection has an export (a resource) that depends on the interface itself - specifically the resource alias owner is the interface being merged. This causes an infinite loop since we've not yet registered this interface as being remapped.
rylev
reviewed
Apr 15, 2024
rylev
reviewed
Apr 15, 2024
Co-authored-by: Ryan Levick <[email protected]>
Co-authored-by: Ryan Levick <[email protected]>
Co-authored-by: Ryan Levick <[email protected]>
Co-authored-by: Ryan Levick <[email protected]>
Co-authored-by: Ryan Levick <[email protected]>
Co-authored-by: Ryan Levick <[email protected]>
Co-authored-by: Ryan Levick <[email protected]>
Co-authored-by: Ryan Levick <[email protected]>
This commit fixes an existing bug exposed by the refactoring (likely due to the changes in the order in which nodes are processed for implicit arguments). Previously, an interface might contain a type alias for another resource it owns; this lead to the alias information of the resource being self-referential for the interface containing the aliased resource. It would then lead to infinite recursion in an attempt to remap such an interface when populating implicit instantiation arguments. The fix is to clear the self-referencing information while converting an interface during package parsing.
rylev
reviewed
Apr 15, 2024
Co-authored-by: Ryan Levick <[email protected]>
rylev
approved these changes
Apr 15, 2024
peterhuene
added a commit
to peterhuene/wac
that referenced
this pull request
Apr 16, 2024
The `NodeInfo` struct should have been removed with the changes in bytecodealliance#82 as it is no longer needed.
Merged
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.
This PR is a complete reimplementation of
wac-parser's resolution to be based onwac-graph.It also contains an overhaul of the
wac-graphAPI to facilitate its use fromwac-parser:CompositionGraphon top ofpetgraph::StableDiGraph, which eliminates needing to keep track of nodes internally toCompositionGraph.Errorenum into discrete error types for each graph operation; this allows the AST resolver to properly attach span information to errors.CompositionGraphthat outputs a DOT representation (used in resolution tests as well).The subtype checker implementation was reverted to the previous implementation, where
invertandrevertare called at specific points to change the type of check being performed; the newer implementation did not give the correct "expected, found" error messages for AST resolution.wac resolvenow outputs a DOT representation of the resolved composition graph instead of JSON.A names custom section is now included in the output of an encoded WAC document.
Most of the deleted code comes from code that has already moved into
wac-typesandwac-graph.Closes #79.
Fixes #78.
Fixes #76.
Closes #37.