add AST strong types by OmarTawfik · Pull Request #719 · NomicFoundation/slang

OmarTawfik · 2023-12-20T15:26:24Z

Adds TypeScript AST types, and some initial tests. Additionally:

changed the DSL Item to be an enum of Rc<> variants, instead of an Rc<> to an enum, to make it easier to clone/store individual variants separately.
unified some inconsistent Rust/TypeScript node helpers: *_with_kind(), *_with_kinds(), *_is_kind()), ...

changeset-bot · 2023-12-20T15:26:27Z

🦋 Changeset detected

Latest commit: a6d9180

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/solidity/outputs/npm/package/src/ast/generated/frontend.ts

Xanewok

Initial pass, I'll come back to this later. Overall, it's great to see that land soon; good job!

bin/hermit.hcl

crates/codegen/parser/generator/src/ast.rs

crates/codegen/parser/runtime/src/napi/napi_ast_helpers.rs

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

crates/codegen/parser/generator/src/trie.rs

crates/codegen/parser/generator/src/lib.rs

crates/codegen/parser/generator/src/code_generator.rs

Xanewok · 2023-12-21T10:33:32Z

I like the proposed naming!

…

On Thu, 21 Dec 2023 at 11:32, Omar Tawfik ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/codegen/parser/generator/src/code_generator.rs <#719 (comment)> : > @@ -62,6 +63,18 @@ impl CodeGenerator { CargoWorkspace::locate_source_crate("codegen_parser_runtime")?.join("src"); let mut codegen = Codegen::read_write(&runtime_dir)?; + { + #[derive(Serialize)] + pub struct Template<'a> { + pub ast_types: &'a AstTypes, + } + codegen.render( + Template { ast_types }, + runtime_dir.join("napi/templates/ast_backend.rs.jinja2"), + output_dir.join("napi/napi_ast_backend.rs"), I can change to: - ast_model.rs for the codegen model builder - ast_selectors instead of the backend - ast_types for the front end WDYT? — Reply to this email directly, view it on GitHub <#719 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXTFXNABXX2OU4PBFPGEADYKQF3NAVCNFSM6AAAAABA5AFGQCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJSGY4TMNRSHE> . You are receiving this because your review was requested.Message ID: ***@***.***>

Xanewok · 2023-12-21T10:34:55Z

Agreed on Q1 and module split; this will only get unnecessarily complicated down the line if we don’t restructure it 👍

…

On Thu, 21 Dec 2023 at 11:29, Omar Tawfik ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/codegen/parser/generator/src/ast.rs <#719 (comment)> : > @@ -0,0 +1,298 @@ +use codegen_language_definition::model; +use indexmap::{IndexMap, IndexSet}; +use itertools::Itertools; +use serde::Serialize; + +#[derive(Default, Serialize)] +pub struct AstTypes { Sure. Happy to rename. Re: module organization: I think this is related to the bigger issue, of having a monolith codegen step that is tied to both rust/typescript. I suggest we dedicate some time in Q1 to figure out a better way to organize this pipeline. — Reply to this email directly, view it on GitHub <#719 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXTFXM2K4JYKI4RYGPEWM3YKQFPRAVCNFSM6AAAAABA5AFGQCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJSGY4TCNZWGY> . You are receiving this because your review was requested.Message ID: ***@***.***>

crates/infra/utils/src/codegen/common/tera.rs

crates/infra/utils/src/codegen/common/file_system.rs

crates/infra/utils/src/codegen/common/formatting.rs

crates/infra/utils/src/codegen/common/tera.rs

crates/codegen/parser/generator/src/code_generator.rs

crates/codegen/language/definition/src/model/item.rs

Xanewok

I'm excited to finally have the AST types, great job 🎉 I left some comments for the doc example and for the frontend/backend distinction. Also, it'd be great to clean up a bit the codegen pipeline since it's getting messy but we decided to punt that to Q1.

I understand we have a holiday season now and so there's some time pressure, so I don't want to block this now; we can address further feedback separately/later in Q1.

Quite a holiday gift; happy holidays and again, great job 🎉

Xanewok · 2023-12-23T10:33:23Z

crates/solidity/outputs/npm/build/src/main.rs

+        )?;

-        CodeGenerator::write_source(&crate_dir.join("src/generated"), &grammar)?;
+        CodeGenerator::write_frontend(


I'd like to change the backend/frontend naming here as well since it's slightly confusing, but let's do that in a follow-up.

Xanewok · 2023-12-23T10:34:50Z

crates/codegen/parser/runtime/src/napi/templates/ast_selectors.rs.jinja2

+
+#[derive(Debug, thiserror::Error)]
+enum Error {
+    // Should not theoritically happen, since we're only called from our own generated AST types.


Suggested change

// Should not theoritically happen, since we're only called from our own generated AST types.

// Should not theoretically happen, since we're only called from our own generated AST types.

Xanewok · 2023-12-23T10:34:56Z

crates/codegen/parser/runtime/src/napi/templates/ast_selectors.rs.jinja2

+    #[error("Unexpected parent node with RuleKind '{0}'.")]
+    UnexpectedParent(RuleKind),
+
+    // Should not theoritically happen, since we're only called from our own generated AST types.


Suggested change

// Should not theoritically happen, since we're only called from our own generated AST types.

// Should not theoretically happen, since we're only called from our own generated AST types.

Xanewok · 2023-12-23T10:35:04Z

crates/codegen/parser/runtime/src/napi/templates/ast_selectors.rs.jinja2

+    #[error("Unexpected trailing children at index '{0}'.")]
+    UnexpectedTrailing(usize),
+
+    // Should not theoritically happen, unless AST error recovery was changed.


Suggested change

// Should not theoritically happen, unless AST error recovery was changed.

// Should not theoretically happen, unless AST error recovery was changed.

Xanewok · 2023-12-23T10:37:40Z

crates/codegen/parser/runtime/src/napi/templates/ast_selectors.rs.jinja2

+        Ok(None)
+    }
+
+    fn finalize(&mut self) -> Result<()> {


nit: If I understand the intent correctly, this should probably consume the type with self to better express intent/disallow calling twice

Xanewok · 2023-12-23T10:41:13Z

documentation/public/user-guide/npm-package/how-to-parse-a-file/index.md

 ```

-### Example 2: List the top-level contracts and their names
+### Example 2: Reading Contracts using Cursors


Saying we "read" them in this example is, I feel like, a bit too generous - we're only fetching the name, which is a direct token child of the encompassing rule. It'd be better if we had some more examples on how to practically read the contracts (listing public/external functions, errors etc.) and use that wording there

also I feel that "contracts" should be lower case since it refers to contracts generally; Cursor is okay since it's the Cursor API/structs we expose

Xanewok · 2023-12-23T10:43:47Z

documentation/public/user-guide/npm-package/how-to-parse-a-file/index.md

 The `Cursor` type provides procedural-style functions that allow you to navigate the source in a step-by-step manner. In addition to `goToNext`, we can go to the parent, first child, next sibling, etc., as well as nodes with a given kind.

-To list the top-level contracts and their names, we need to visit the `ContractDefinition` rule nodes and then their `Identifier` children.
+Let's start with this piece of solidity source code:


Suggested change

Let's start with this piece of solidity source code:

Let's start with this piece of Solidity source code:

Xanewok · 2023-12-23T10:48:00Z

documentation/public/user-guide/npm-package/how-to-parse-a-file/index.md

+
+AST types are a set of TypeScript classes that provide a typed view of the untyped CST nodes.
+You can convert any untyped CST node to its corresponding AST type using their constructors.
+AST types are immutable, and are constructed on the fly, as they are used/accessed for the first time.


Suggested change

AST types are immutable, and are constructed on the fly, as they are used/accessed for the first time.

AST types are immutable and are constructed on the fly as they are used/accessed for the first time.

small suggestion: Maybe "lazily" would fit better instead of "on the fly"? Seems like it would be more intuitive for the (programming) reader.

- address later feedback on NomicFoundation#719 - fix syntax highlighting for TS templates - use `ReadonlyArray<T>` in return types instead of `Array<T>`. - use `undefined` instead of `null` for optional fields, as it is more idiomatic.

- address later feedback on #719 - fix syntax highlighting for TS templates - use `ReadonlyArray<T>` in return types instead of `Array<T>`. - use `undefined` instead of `null` for optional fields, as it is more idiomatic.

@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.13.0 ### Minor Changes - [#710](#710) [`2025b6cb`](2025b6c) Thanks [@Xanewok](https://github.com/Xanewok)! - CST children nodes are now named - [#723](#723) [`b3dc6bcd`](b3dc6bc) Thanks [@Xanewok](https://github.com/Xanewok)! - Properly parse unreserved keywords in an identifier position, i.e. `from`, `emit`, `global` etc. - [#728](#728) [`662a672c`](662a672) Thanks [@Xanewok](https://github.com/Xanewok)! - Remove Language#scan API; use the parser API instead - [#719](#719) [`1ad6bb37`](1ad6bb3) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - introduce strong types for all Solidity non terminals in the TypeScript API. ### Patch Changes - [#719](#719) [`1ad6bb37`](1ad6bb3) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - unify Rust/TypeScript node helpers: `*_with_kind()`, `*_with_kinds()`, `*_is_kind()`), ... - [#731](#731) [`3deaea2e`](3deaea2) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `RuleNode.unparse()` to the TypeScript API 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.13.0 ### Minor Changes - [#710](NomicFoundation/slang#710) [`2025b6cb`](NomicFoundation/slang@2025b6c) Thanks [@Xanewok](https://github.com/Xanewok)! - CST children nodes are now named - [#723](NomicFoundation/slang#723) [`b3dc6bcd`](NomicFoundation/slang@b3dc6bc) Thanks [@Xanewok](https://github.com/Xanewok)! - Properly parse unreserved keywords in an identifier position, i.e. `from`, `emit`, `global` etc. - [#728](NomicFoundation/slang#728) [`662a672c`](NomicFoundation/slang@662a672) Thanks [@Xanewok](https://github.com/Xanewok)! - Remove Language#scan API; use the parser API instead - [#719](NomicFoundation/slang#719) [`1ad6bb37`](NomicFoundation/slang@1ad6bb3) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - introduce strong types for all Solidity non terminals in the TypeScript API. ### Patch Changes - [#719](NomicFoundation/slang#719) [`1ad6bb37`](NomicFoundation/slang@1ad6bb3) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - unify Rust/TypeScript node helpers: `*_with_kind()`, `*_with_kinds()`, `*_is_kind()`), ... - [#731](NomicFoundation/slang#731) [`3deaea2e`](NomicFoundation/slang@3deaea2) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `RuleNode.unparse()` to the TypeScript API Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

OmarTawfik marked this pull request as ready for review December 20, 2023 15:36

OmarTawfik requested a review from a team as a code owner December 20, 2023 15:36

OmarTawfik enabled auto-merge December 20, 2023 15:52

OmarTawfik linked an issue Dec 21, 2023 that may be closed by this pull request

Implement a Typed AST #634

Closed

AntonyBlakey reviewed Dec 21, 2023

View reviewed changes

crates/solidity/outputs/npm/package/src/ast/generated/frontend.ts Outdated Show resolved Hide resolved

crates/solidity/outputs/npm/package/src/ast/generated/frontend.ts Outdated Show resolved Hide resolved

Xanewok reviewed Dec 21, 2023

View reviewed changes

generate NAPI AST types

2521648

OmarTawfik force-pushed the ast-types branch from d654844 to f053b06 Compare December 22, 2023 13:02

address feedback

7357fb5

OmarTawfik force-pushed the ast-types branch from f053b06 to 57a9930 Compare December 22, 2023 14:32

doc example

a6d9180

OmarTawfik force-pushed the ast-types branch from 57a9930 to a6d9180 Compare December 22, 2023 14:44

Xanewok approved these changes Dec 23, 2023

View reviewed changes

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

Merged via the queue into NomicFoundation:main with commit 1ad6bb3 Dec 23, 2023

OmarTawfik deleted the ast-types branch December 23, 2023 11:02

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

Bump Slang Version #718

Merged

OmarTawfik mentioned this pull request Jan 3, 2024

clean up AST types #727

Merged

	// Should not theoritically happen, since we're only called from our own generated AST types.
	// Should not theoretically happen, since we're only called from our own generated AST types.

	// Should not theoritically happen, unless AST error recovery was changed.
	// Should not theoretically happen, unless AST error recovery was changed.

	Let's start with this piece of solidity source code:
	Let's start with this piece of Solidity source code:

	AST types are immutable, and are constructed on the fly, as they are used/accessed for the first time.
	AST types are immutable and are constructed on the fly as they are used/accessed for the first time.

Conversation

OmarTawfik commented Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Dec 20, 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!

Xanewok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xanewok commented Dec 21, 2023 via email

Uh oh!

Xanewok commented Dec 21, 2023 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xanewok left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

OmarTawfik commented Dec 20, 2023 •

edited

Loading

changeset-bot bot commented Dec 20, 2023 •

edited

Loading