Implement Iterator for Cursor by Xanewok · Pull Request #625 · NomicFoundation/slang

Xanewok · 2023-10-29T22:04:51Z

Closes #609

Still needs tweaking to adapt to the current Cursor's behavior and needs more documentation/tests for the Visitor trait.

...since we can freely reset and/or re-spawn it at another location

changeset-bot · 2023-10-29T22:04:55Z

🦋 Changeset detected

Latest commit: 81e772f

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	Minor

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

AntonyBlakey · 2023-10-30T02:19:49Z

crates/codegen/parser/runtime/src/visitor.rs

-        node: &Rc<RuleNode>,
-        cursor: &Cursor,
-    ) -> Result<VisitorEntryResponse, E> {
+    fn rule_enter(&mut self, _: &Rc<RuleNode>, _: &Cursor) -> Result<VisitorEntryResponse, E> {


I think _node etc is preferable because it documents the role of the arg.

The type pretty much documents what it is in this case, but I can also use the prefixed name, if preferred

The type documents it only with some reasoning applied, whereas the name is immediate.

Done in d5e851d

AntonyBlakey · 2023-10-30T02:22:49Z

crates/codegen/parser/runtime/src/cursor.rs

+/// A [`NodePtr`] that points to a [`RuleNode`].
 #[derive(Clone, Debug, PartialEq, Eq)]
-struct CursorPathElement {
+struct RuleNodePtr {


I prefer the original name because it documents the purpose of the struct, rather than merely re-describing what it contains.

In general I agree that naming should help document the purpose, but in this case I believe that separating where it's used versus how it's used makes the code below slightly less obvious.

Due to the way that the cursor is implemented, the Cursor path element has to contain what the pointed to node has (child number and text offset) and the only way these path elements are actually used in the code is to convert between the NodePtr <-> this struct.

Because of this, I think it's better to emphasise that that the conversion is almost 1-1, with the rule node being more specific type. Otherwise reading NodePtr::to_path_element or PathElement::into_node_ptr might imply a less direct conversion.

All in all, this simplifies the model to "Cursor iteration always needs (node, child_id, offset)". The fact that the path can only contain rule nodes is external to this module and also used in the public fn path_rule_nodes.

Maybe I can change it to PathRuleNode{Ptr,Pointer}?

happy with PathRuleNode

Done in 03d055a

AntonyBlakey · 2023-10-30T02:23:15Z

crates/codegen/parser/runtime/src/cursor.rs

+
+/// A pointer to a [`Node`] in a CST, used by the [`Cursor`] to implement the traversal.
 #[derive(Clone, Debug, PartialEq, Eq)]
-struct CursorLeaf {


The cursor is also meant to iterate on the rule nodes, so I'm missing the leaf naming aspect. That's why I renamed it and went with the fact that the cursor points to a node, hence pointer. Was thinking about CursorNodePtr to specify that it's Cursor specific, but since everything in this module is Cursor-specific already, I didn't find repeating that valuable, so I was left with NodePtr.

It's the leaf of the path. So in alignment with the other name, this should be LeafNode

Are we sure about that the naming here? In the context of traversing the tree, referring to the (current) final path element as leaf is very confusing to me, since leaf nodes (almost?) exclusively refer to the (terminal) nodes without children and the currently pointed to node is almost as probable to be an internal node as it is to be an outer one during the traversal.

PathLeafNode? or PathEndNode or Last or Final. It's the principle I care about.

Done in 03d055a

To retain the existing behaviour. This should be mostly cleanup/org pass more than anything.

This is already in std and designed for such use cases.

Xanewok · 2023-10-30T17:15:08Z

This retains the same API behaviour but I understand we'll want to adapt it soon anyway.

Also, not feeling particularly strongly about 6c374f4, just included it to see if that's something we might be interested in. I also tried to go further in the 6fc2aa5 and there I flipped the ControlFlow with Result which makes the code a bit easier to read but the error propagation is slightly less convenient out of the box.

AntonyBlakey

IMO, when a function contains any return statements, then it should use a return at the end as well. But not a blocker here.

@Xanewok

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @nomicfoundation/slang@0.11.0 ### Minor Changes - [#625](#625) [`7bb650b`](7bb650b) Thanks [@Xanewok](https://github.com/Xanewok)! - The CST Cursor now implements the Iterator trait as part of the Rust API - [#647](#647) [`b1dced3`](b1dced3) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Require specifying an initial offset when creating a CST cursor. ### Patch Changes - [#648](#648) [`2327bf5`](2327bf5) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Support Solidity v0.8.22. - [#623](#623) [`80114a8`](80114a8) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Correct the types in the TS api by adding the correct namespaces to type references Co-authored-by: OmarTawfik <15987992+OmarTawfik@users.noreply.github.com>

@Xanewok

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @nomicfoundation/slang@0.11.0 ### Minor Changes - [#625](NomicFoundation/slang#625) [`7bb650b`](NomicFoundation/slang@7bb650b) Thanks [@Xanewok](https://github.com/Xanewok)! - The CST Cursor now implements the Iterator trait as part of the Rust API - [#647](NomicFoundation/slang#647) [`b1dced3`](NomicFoundation/slang@b1dced3) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Require specifying an initial offset when creating a CST cursor. ### Patch Changes - [#648](NomicFoundation/slang#648) [`2327bf5`](NomicFoundation/slang@2327bf5) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Support Solidity v0.8.22. - [#623](NomicFoundation/slang#623) [`80114a8`](NomicFoundation/slang@80114a8) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Correct the types in the TS api by adding the correct namespaces to type references Co-authored-by: OmarTawfik <15987992+OmarTawfik@users.noreply.github.com>

Xanewok added 7 commits October 28, 2023 19:17

chore: Remove unused allow directives around the Cursor

b19f7a6

refactor: Document and slightly oxidize the Cursor internals

c6f8b77

feat: Implement (Fused)Iterator for Cursor

93fe84c

refactor: Use Cursor directly instead of helper DescendentsUnordered

6ff92c2

WIP: Try to simplify the existing Cursor helpers

8156cec

wip: document the discrepancy between iterator and cursor impls

91b3b22

fix: Cursor is not a FusedIterator

8e43cbd

...since we can freely reset and/or re-spawn it at another location

Xanewok changed the title ~~implement Iterator for Cursor~~ Implement Iterator for Cursor Oct 29, 2023

AntonyBlakey reviewed Oct 30, 2023

View reviewed changes

Xanewok added 10 commits October 30, 2023 12:00

Add a test for the Cursor iterator impl

dc68fb4

refactor: Simplify summing up the children text length in Cursor

9ebf9dd

fixup: Simplify also the cursor sage in parser_function.rs

67f69ab

Settle on non-consuming Cursor::find_* APIs for now

0492133

To retain the existing behaviour. This should be mostly cleanup/org pass more than anything.

Simplify the cursor_as_iter test

3754823

Rename Cursor helper structs

03d055a

Bring back arg names in Visitor functions

d5e851d

Document slightly the Visitor trait

8268619

Prefer ControlFlow over Visitor{Entry,Exit}Response

6c374f4

This is already in std and designed for such use cases.

Add a changeset file

81e772f

Xanewok marked this pull request as ready for review October 30, 2023 17:09

Xanewok requested a review from a team as a code owner October 30, 2023 17:09

AntonyBlakey approved these changes Oct 31, 2023

View reviewed changes

Xanewok added this pull request to the merge queue Oct 31, 2023

Merged via the queue into NomicFoundation:main with commit 7bb650b Oct 31, 2023

Xanewok deleted the feat-cursor-iterator branch October 31, 2023 11:08

github-actions bot mentioned this pull request Oct 31, 2023

Bump Slang Version #624

Merged

Conversation

Xanewok commented Oct 29, 2023

Uh oh!

changeset-bot bot commented Oct 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xanewok commented Oct 30, 2023

Uh oh!

AntonyBlakey left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot bot commented Oct 29, 2023 •

edited

Loading