Add `Node::unparse` and tweak the cursor creation API #666

Xanewok · 2023-11-20T16:17:25Z

Closes #583
Ref #628 for the modified cursor/offset API

Changes are best reviewed commit-by-commit.

...than `Default::default()`, which doesn't say much at the call-site.

changeset-bot · 2023-11-20T16:17:28Z

🦋 Changeset detected

Latest commit: 1007589

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

OmarTawfik

Left a few questions.

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

OmarTawfik · 2023-11-20T22:33:25Z

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

        }
-        return None;
    }

    pub fn as_rule_with_kind(&self, kinds: &[RuleKind]) -> Option<&Rc<RuleNode>> {


Possibly unrelated to this PR, but asking since it is addressing API feedback:

For as_rule_matching(...) .. I wonder how is that different from .as_rule().filter(...), given it is the same characters/effort to type?

Are we duplicating Rust std:: methods? what new scenarios does it enable?

Same question for as_token_matching().

You're right that this is merely a convenience/"combined" method.

This API is from when, as I understand it, we wanted to have parity in the Rust/TS API. Since we decided against that recently, we can start thinking whether we want it or not on Rust side (AIUI it's more performant to keep that on TS side).

My first reaction was the same as yours, i.e as_{rule,token}_matching is probably redundant on the Rust side, now that we have introduced convenience as_{rule,token} at some point.

I do see some point in the convenience _with_kind functions, as as_{rule,token}_with_kind reads and formats better than the as_{rule,token}().filter(|node| &[some,kinds].contains(node.kind)) chain. WDYT?

I like these semantically rich apis that match common use cases. I'm also happy to have them in a separate trait, such as RichXXXAPI or EnhancedXXX etc, simply for the sake of having a 'core' minimal API and then a layered convenience API.

Specifically because it reads well in the code to express clearly the higher-level intent. And I like descriptive precise names, with no concern at all about the length of names.

For as_rule_matching(...) .. I wonder how is that different from .as_rule().filter(...), given it is the same characters/effort to type?

Same to type, but less to understand.

I do see some point in the convenience _with_kind functions, as as_{rule,token}_with_kind reads and formats better than the as_{rule,token}().filter(|node| &[some,kinds].contains(node.kind)) chain. WDYT?

Same to type, but less to understand.

@Xanewok @AntonyBlakey

with_kind() makes sense. The alternative doesn't have a good Rust standard library alternative, and I imagine it is a common use case, enough to have a helper for.

But on the other hand, as_rule_matching() here is longer than the Rust standard library alternative (.as_rule().filter()), which most Rust users are already familiar with, and is used extensively in all projects, so I feel we are only replacing the language built-in API with a longer/less idiomatic one, that is not really easier to use (both accept the same kind of callback).

Agree, I'm not so sure about the _matching variant, especially since as(...).filter(...) does the same and filter is bread-and-butter idiomatic Rust.

I can understand the readability context a bit but then again, if it's not using the std impls then that might imply something non-standard, actually.

I'm fine either way, to be honest!

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

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

crates/codegen/parser/runtime/src/support/parser_result.rs

crates/solidity/outputs/cargo/tests/src/doc_examples/simple_contract.rs

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

OmarTawfik

Left one suggestion about removing as_XXX_matching(), but otherwise, LGTM!

This is more vanilla and we didn't really use those internally ourselves, so I'm not sure if there's much benefit for those. We can reintroduce a `NodeHelpers` trait in the future that has that convenience API.

Xanewok · 2023-11-22T11:33:11Z

I think it makes sense to drop the individual as_*matching() for the tokens/rules, as as_*() followed by filter() should be equally readable for a Rust reader and this means there are less surprises (we're using std combinators) comparing to, let's say, Cursor's find_*_matching that does not consume the first item, unlike std::iter::Iterator::find.

@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/[email protected] ### 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 added 6 commits November 20, 2023 16:23

Rename create_cursor to cursor_with_offset

6867291

Implement convenience From for RuleNode/TokenNode

7b0f5ea

Simplify as_{token,rule}_{matching,with_kind}

9e5df5c

Add convenience RuleNode::cursor_with_offset

639547b

Add (Rule)Node::reconstruct that recreates the source code

db14452

Prefer cursor_with_offset(TextIndex::ZERO)

34a8034

...than `Default::default()`, which doesn't say much at the call-site.

Xanewok requested a review from a team as a code owner November 20, 2023 16:17

Add a changeset file

c392a5a

OmarTawfik requested changes Nov 20, 2023

View reviewed changes

Xanewok added 5 commits November 21, 2023 13:33

Rename Node::reconstruct to Node::unparse

e6421cc

Use TextIndex::ZERO in more places

2431458

Tweak the simple_contract test

ad3264d

Merge remote-tracking branch 'upstream/main' into cursor-tweaks

15ffdc5

Ignore unparse in cspell

4c446cb

Xanewok changed the title ~~Add Node::reconstruct and tweak the cursor creation API~~ Add Node::unparse and tweak the cursor creation API Nov 21, 2023

OmarTawfik reviewed Nov 21, 2023

View reviewed changes

crates/solidity/outputs/cargo/tests/src/doc_examples/simple_contract.rs Show resolved Hide resolved

OmarTawfik reviewed Nov 21, 2023

View reviewed changes

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

OmarTawfik approved these changes Nov 21, 2023

View reviewed changes

Xanewok added 2 commits November 22, 2023 11:00

Fix a typo

f478a03

Remove as_{rule,token}_matching in favor of as_*(..).filter(..)

1007589

This is more vanilla and we didn't really use those internally ourselves, so I'm not sure if there's much benefit for those. We can reintroduce a `NodeHelpers` trait in the future that has that convenience API.

Xanewok enabled auto-merge November 22, 2023 11:33

Xanewok added this pull request to the merge queue Nov 22, 2023

Merged via the queue into NomicFoundation:main with commit 0434b68 Nov 22, 2023
1 check passed

Xanewok deleted the cursor-tweaks branch November 22, 2023 11:47

github-actions bot mentioned this pull request Nov 22, 2023

Bump Slang Version #669

Merged

Xanewok mentioned this pull request Dec 2, 2023

clarify cursor API initial offsets #628

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Node::unparse` and tweak the cursor creation API #666

Add `Node::unparse` and tweak the cursor creation API #666

Xanewok commented Nov 20, 2023

changeset-bot bot commented Nov 20, 2023 •

edited

Loading

OmarTawfik left a comment

OmarTawfik Nov 20, 2023

Xanewok Nov 21, 2023

AntonyBlakey Nov 21, 2023

AntonyBlakey Nov 21, 2023

AntonyBlakey Nov 21, 2023

OmarTawfik Nov 21, 2023

Xanewok Nov 21, 2023

OmarTawfik left a comment

Xanewok commented Nov 22, 2023

Add Node::unparse and tweak the cursor creation API #666

Add Node::unparse and tweak the cursor creation API #666

Conversation

Xanewok commented Nov 20, 2023

changeset-bot bot commented Nov 20, 2023 • edited Loading

🦋 Changeset detected

OmarTawfik left a comment

Choose a reason for hiding this comment

OmarTawfik Nov 20, 2023

Choose a reason for hiding this comment

Xanewok Nov 21, 2023

Choose a reason for hiding this comment

AntonyBlakey Nov 21, 2023

Choose a reason for hiding this comment

AntonyBlakey Nov 21, 2023

Choose a reason for hiding this comment

AntonyBlakey Nov 21, 2023

Choose a reason for hiding this comment

OmarTawfik Nov 21, 2023

Choose a reason for hiding this comment

Xanewok Nov 21, 2023

Choose a reason for hiding this comment

OmarTawfik left a comment

Choose a reason for hiding this comment

Xanewok commented Nov 22, 2023

Add `Node::unparse` and tweak the cursor creation API #666

Add `Node::unparse` and tweak the cursor creation API #666

changeset-bot bot commented Nov 20, 2023 •

edited

Loading