fix NAPI `cursor` types and expose `cursor.depth` by OmarTawfik · Pull Request #676 · NomicFoundation/slang

OmarTawfik · 2023-11-27T23:36:03Z

expose cursor.depth API.
add cst.Node union type.
type check NAPI index.d.ts.
replace Cursor::find_*() APIs with go_to_*() alternatives.

Fixes #636

changeset-bot · 2023-11-27T23:36:07Z

🦋 Changeset detected

Latest commit: 82b9683

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

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

Xanewok · 2023-12-01T14:33:37Z

crates/solidity/testing/utils/src/version_pragmas/mod.rs


-    let mut cursor = output.create_tree_cursor();
    let mut pragmas = vec![];
-    while let Some(rule_node) = cursor.find_rule_with_kind(&[RuleKind::VersionPragmaExpression]) {


Can't we adapt the old code to directly use the new while cursor.go_to_next_rule_with_kinds(..) followed by extract_pragma(cursor.node())?

Unfortunately this is recursive, which for something like X || Y would match all three of X || Y, X, and Y. We need to skip children when found.

Since this is DFS pre-order, I imagine the parent should be matched first and in the loop body we already call cursor.go_to_next_non_descendent(), which should skip the subtree elements (i.e. individual versions) in a subsequent while find_rule_with_kind loop call. Otherwise, how the previous code handled it? Or was it wrong/matched everything recursively?

This would miss two consecutive versions like X Y which is also valid syntax. go_to_next_non_descendent() would go to Y, and then find_rule_with_kind() would skip it before we get to it.

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

Xanewok

Left a question but not blocking; thanks!

Xanewok · 2023-12-02T13:13:03Z

crates/solidity/testing/utils/src/version_pragmas/mod.rs


-    let mut cursor = output.create_tree_cursor();
    let mut pragmas = vec![];
-    while let Some(rule_node) = cursor.find_rule_with_kind(&[RuleKind::VersionPragmaExpression]) {


Since this is DFS pre-order, I imagine the parent should be matched first and in the loop body we already call cursor.go_to_next_non_descendent(), which should skip the subtree elements (i.e. individual versions) in a subsequent while find_rule_with_kind loop call. Otherwise, how the previous code handled it? Or was it wrong/matched everything recursively?

Xanewok · 2023-12-02T13:15:23Z

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

        if self.current.child_number > 0 {
            if let Some(parent_path_element) = self.path.last() {
-                let new_child_number = self.current.child_number + 1;
+                let new_child_number = self.current.child_number - 1;


Ooh, good catch! It'd be great to add some simple tests to catch stuff like this at some point.

Xanewok · 2023-12-02T13:16:03Z

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

-        }
-
-        true
+        self.go_to_first_child() || self.go_to_next_non_descendent()


I was gonna mumble about it but on the second thought it really is a lot clearer now, good thinking!

Xanewok · 2023-12-02T14:36:58Z

It looks like CI needs fixing.

Xanewok · 2023-12-02T18:24:57Z

@OmarTawfik I've edited the OP to close the #636 when merged, hope you don't mind!

I think that further improvements not strictly related to exposing the existing Rust logic to TS (like tracking the line/column from #683) are probably better tracked in #334.

@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.12.0 ### Minor Changes - [#699](#699) [`ddfebfe9`](ddfebfe) Thanks [@Xanewok](https://github.com/Xanewok)! - Remove `ProductionKind` in favor of `RuleKind` - [#699](#699) [`ddfebfe9`](ddfebfe) Thanks [@Xanewok](https://github.com/Xanewok)! - Allow parsing individual precedence expressions, like `ShiftExpression` - [#665](#665) [`4b5f8b46`](4b5f8b4) Thanks [@Xanewok](https://github.com/Xanewok)! - Remove the CST Visitor API in favor of the Cursor API - [#666](#666) [`0434b68c`](0434b68) Thanks [@Xanewok](https://github.com/Xanewok)! - Add `Node::unparse()` that allows to reconstruct the source code from the CST node - [#675](#675) [`daea4b7f`](daea4b7) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - rename `Cursor`'s `pathRuleNodes()` to `ancestors()` in the NodeJS API. - [#676](#676) [`b496d361`](b496d36) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Fix NAPI `cursor` types and expose `cursor.depth`. ### Patch Changes - [#685](#685) [`b5fca94a`](b5fca94) Thanks [@Xanewok](https://github.com/Xanewok)! - `bytes` is now properly recognized as a reserved word - [#660](#660) [`97028991`](9702899) Thanks [@Xanewok](https://github.com/Xanewok)! - Drop List suffix from collection grammar rule names Co-authored-by: github-actions[bot] <github-actions[bot]@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.12.0 ### Minor Changes - [#699](NomicFoundation/slang#699) [`ddfebfe9`](NomicFoundation/slang@ddfebfe) Thanks [@Xanewok](https://github.com/Xanewok)! - Remove `ProductionKind` in favor of `RuleKind` - [#699](NomicFoundation/slang#699) [`ddfebfe9`](NomicFoundation/slang@ddfebfe) Thanks [@Xanewok](https://github.com/Xanewok)! - Allow parsing individual precedence expressions, like `ShiftExpression` - [#665](NomicFoundation/slang#665) [`4b5f8b46`](NomicFoundation/slang@4b5f8b4) Thanks [@Xanewok](https://github.com/Xanewok)! - Remove the CST Visitor API in favor of the Cursor API - [#666](NomicFoundation/slang#666) [`0434b68c`](NomicFoundation/slang@0434b68) Thanks [@Xanewok](https://github.com/Xanewok)! - Add `Node::unparse()` that allows to reconstruct the source code from the CST node - [#675](NomicFoundation/slang#675) [`daea4b7f`](NomicFoundation/slang@daea4b7) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - rename `Cursor`'s `pathRuleNodes()` to `ancestors()` in the NodeJS API. - [#676](NomicFoundation/slang#676) [`b496d361`](NomicFoundation/slang@b496d36) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Fix NAPI `cursor` types and expose `cursor.depth`. ### Patch Changes - [#685](NomicFoundation/slang#685) [`b5fca94a`](NomicFoundation/slang@b5fca94) Thanks [@Xanewok](https://github.com/Xanewok)! - `bytes` is now properly recognized as a reserved word - [#660](NomicFoundation/slang#660) [`97028991`](NomicFoundation/slang@9702899) Thanks [@Xanewok](https://github.com/Xanewok)! - Drop List suffix from collection grammar rule names Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

OmarTawfik requested a review from a team as a code owner November 27, 2023 23:36

OmarTawfik enabled auto-merge November 27, 2023 23:36

Xanewok suggested changes Nov 28, 2023

View reviewed changes

crates/codegen/parser/runtime/src/cursor.rs Show resolved Hide resolved

crates/codegen/parser/runtime/src/cursor.rs Outdated Show resolved Hide resolved

crates/codegen/parser/runtime/src/cursor.rs Show resolved Hide resolved

OmarTawfik mentioned this pull request Nov 28, 2023

Expose Cursor in TypeScript #636

Closed

OmarTawfik force-pushed the fix-cursor-matching-api branch 2 times, most recently from 3d679ce to 428e9ce Compare December 1, 2023 12:01

OmarTawfik changed the title ~~fix Cursor::find_matching() API~~ fix NAPI cursor types and expose cursor.depth Dec 1, 2023

OmarTawfik requested a review from Xanewok December 1, 2023 12:14

Xanewok reviewed Dec 1, 2023

View reviewed changes

crates/codegen/parser/runtime/src/cursor.rs Outdated Show resolved Hide resolved

OmarTawfik force-pushed the fix-cursor-matching-api branch from 65dfb5c to 18164d5 Compare December 2, 2023 05:50

OmarTawfik requested a review from Xanewok December 2, 2023 05:50

Xanewok approved these changes Dec 2, 2023

View reviewed changes

OmarTawfik added 7 commits December 3, 2023 05:29

replace Cursor::find_matching() API

8dda5ec

type check NAPI index.d.ts

dadf453

add cst.Node union type

8f8a6e4

expose cursor.depth NAPI API

782d161

add changeset

0b83953

spell check

dc62c2d

clean up

82b9683

OmarTawfik force-pushed the fix-cursor-matching-api branch from 18164d5 to 82b9683 Compare December 3, 2023 13:31

OmarTawfik added this pull request to the merge queue Dec 3, 2023

Merged via the queue into NomicFoundation:main with commit b496d36 Dec 3, 2023

OmarTawfik deleted the fix-cursor-matching-api branch December 3, 2023 13:59

github-actions bot mentioned this pull request Dec 3, 2023

Bump Slang Version #669

Merged

Conversation

OmarTawfik commented Nov 27, 2023 • edited by Xanewok Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

🦋 Changeset detected

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xanewok Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Dec 2, 2023

Choose a reason for hiding this comment

Uh oh!

Xanewok Dec 2, 2023

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Dec 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Xanewok left a comment

Choose a reason for hiding this comment

Uh oh!

Xanewok Dec 2, 2023

Choose a reason for hiding this comment

Uh oh!

Xanewok Dec 2, 2023

Choose a reason for hiding this comment

Uh oh!

Xanewok Dec 2, 2023

Choose a reason for hiding this comment

Uh oh!

Xanewok commented Dec 2, 2023

Uh oh!

Xanewok commented Dec 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

OmarTawfik commented Nov 27, 2023 •

edited by Xanewok

Loading

changeset-bot bot commented Nov 27, 2023 •

edited

Loading