Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change Edge::label from optional to required #1205

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Change Edge::label from optional to required #1205

wants to merge 19 commits into from

Conversation

mjoerussell
Copy link
Contributor

@mjoerussell mjoerussell commented Dec 30, 2024 •
edited
Loading

This involves a couple of changes outside of just changing the field type:

  1. CursorIterator must be able to produce a label for every node now. This wasn't possible before because the root nodes of a cursor had no parent, and so could not derive a label. I've changed Cursor in the following ways to fix this:
    • Cursor::parent is a new type, Parent<T>, instead of Option<Rc<PathAncestor<T>>>. Parent<T> is an enum with three variants - None, Open(Rc<PathAncestor<T>>), and Closed(Rc<PathAncestor<T>>). A Closed parent is used when a cursor is spawned from an arbitrary non-root node, and means that the parent can be read but not traversed to. None is only used for the true root node.
  2. KindTypes::EdgeLabel is given a Default implementation, which for now just returns the first PredefinedLabel variant. This is needed in order to be able to set a "concrete" EdgeLabel value from the metaslang_cst crate.
  3. Added a Root variant to PredefinedLabel. This will be used when a node is the true root node. For now this will be the default value for KindTypes::EdgeLabel.

Closes #815

@mjoerussell
Make Edge::label required instead of optional.
b018180 
This involves a couple of changes outside of just changing the field type:
1. `CursorIterator` must be able to produce a label for every node now. This wasn't possible before because the root nodes of a cursor had no parent, and so could not derive a label. I've changed `Cursor` in the following ways to fix this:
    * `Cursor::parent` is a new type, `Parent<T>`, instead of `Option<Rc<PathAncestor<T>>>`. `Parent<T>` is an enum with three variants - `None`, `Open(Rc<PathAncestor<T>>)`, and `Closed(Rc<PathAncestor<T>>)`. A `Closed` parent is used when a cursor is spawned from an arbitrary non-root node, and means that the parent can be read but not traversed to. `None` is only used for the true root node.
2. `KindTypes::EdgeLabel` is given a `Default` implementation, which for now just returns the first `PredefinedLabel` variant. This is needed in order to be able to set a "concrete" `EdgeLabel` value from the `metaslang_cst` crate.
3. Added a `Root` variant to `PredefinedLabel`. This will be used when a node is the true root node. For now this will be the default value for `KindTypes::EdgeLabel`.
@mjoerussell
Update generated files
b12b4c1
@mjoerussell
Remove unused import in cst_output/renderer
bba6f4a
@mjoerussell mjoerussell self-assigned this Dec 30, 2024
@mjoerussell mjoerussell requested review from a team as code owners December 30, 2024 21:26
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 30, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 8515650

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@nomicfoundation/slang Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

OmarTawfik
OmarTawfik requested changes Dec 31, 2024
View reviewed changes
Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

OmarTawfik
OmarTawfik reviewed Dec 31, 2024
View reviewed changes
...ots/cst_output/Statements/recovery_ignore_multiple_empty_matches/generated/0.6.0-failure.yml Outdated
@@ -27,7 +27,7 @@ Tree:
- (for_keyword꞉ ForKeyword): "for" # (0..3)
- (open_paren꞉ OpenParen): "(" # (3..4)
- (initialization꞉ ForStatementInitialization) ► (variant꞉ VariableDeclarationStatement) ► (variable_type꞉ VariableDeclarationType) ► (variant꞉ TypeName) ► (variant꞉ ElementaryType) ► (variant꞉ IntKeyword): "int" # (4..7)
- (MISSING): "" # (7..7)
- (root꞉ MISSING): "" # (7..7)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we are missing labels for MISSING nodes. I wonder if we are able to use the original label the parser was trying to parse, to indicate what is missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an Unrecognized label for UNRECOGNIZED nodes. However, getting the correct label for MISSING nodes was not that straightforward, at least for how well I currently understand the parser. For now, I've left those as having the label Root. @OmarTawfik if you have any thoughts about how to get those labels, I'd be happy to go back and make those updates.

I see. Thanks for the update. This looks like it needs a bigger refactoring the parser codegen, and I don't think it should block this PR/fix.
So, for the time being, WDYT of adding a similar EdgeLabel::MISSING for now, so that we don't mislabel them as root in the meantime?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #1215 to track this, and added it to the backlog for now.

mjoerussell and others added 10 commits January 2, 2025 08:33
@mjoerussell @OmarTawfik
Update crates/codegen/runtime/cargo/wasm/src/runtime/interface/genera…
da6265a 
…ted/cst.wit

Co-authored-by: Omar Tawfik <[email protected]>
@mjoerussell @OmarTawfik
Update crates/codegen/runtime/cargo/crate/src/runtime/cst/edge_label.…
35bd99c 
…rs.jinja2

Co-authored-by: Omar Tawfik <[email protected]>
@mjoerussell @OmarTawfik
Update crates/codegen/runtime/cargo/wasm/src/runtime/interface/genera…
ecb20e3 
…ted/cst.wit

Co-authored-by: Omar Tawfik <[email protected]>
@mjoerussell
Clean up usage of IntoFFI::_into_ffi()
5ef9fd4
@mjoerussell
Return Option<Edge> from Helper::current() instead of `Option<(Ed…
049dd40 
…geLabel, Node)>`
@mjoerussell
Don't skip root nodes in Helper::current()
8d1e175
@mjoerussell
* Change Parent::Open to Parent::Connected and Parent::Closed t…
26729fa 
…o `Parent::Disconnected`

* Change the inner type of `Parent::Disconnected` from another PathAncestor to `T::EdgeLabel` so that an ancestor can't be misused
@mjoerussell
Rename Edge::anonymous() to Edge::root()
790f5d5
@mjoerussell
Print all edge labels in snapshot tests
4b5c352
@mjoerussell
Add an Unrecognized label for Unrecognized nodes
819da29
@mjoerussell
Copy link
Contributor Author

looks like we are missing labels for UNRECOGNIZED nodes. Should we add a matching EdgeLabel::UNRECOGNIZED for these? thoughts?

looks like we are missing labels for MISSING nodes. I wonder if we are able to use the original label the parser was trying to parse, to indicate what is missing?

I've added an Unrecognized label for UNRECOGNIZED nodes. However, getting the correct label for MISSING nodes was not that straightforward, at least for how well I currently understand the parser. For now, I've left those as having the label Root. @OmarTawfik if you have any thoughts about how to get those labels, I'd be happy to go back and make those updates.

All the other comments should be addressed now so this PR is ready for another review.

@mjoerussell mjoerussell requested a review from OmarTawfik January 3, 2025 21:59
@OmarTawfik OmarTawfik mentioned this pull request Jan 7, 2025
Open
OmarTawfik
OmarTawfik requested changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made another pass on all unresoved comments.
Thanks!

OmarTawfik
OmarTawfik requested changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@OmarTawfik OmarTawfik left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is one unresolved comment still: #1205 (comment)

So, for the time being, WDYT of adding a similar EdgeLabel::MISSING for now, so that we don't mislabel them as root in the meantime?

Additionally, I suggest adding a changeset to describe the changes here.

Thanks!

@mjoerussell
Copy link
Contributor Author

I think there is one unresolved comment still: #1205 (comment)

Wasn't that added to the backlog as #1215? I thought that that was going to be resolved in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmarTawfik OmarTawfik OmarTawfik requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@mjoerussell mjoerussell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make EdgeLabel required in the CST
2 participants
@mjoerussell @OmarTawfik