Skip to content

feat!: Indexes for nodes in the AST - #414

Merged
anakrish merged 1 commit into
microsoft:mainfrom
anakrish:node-index
Jun 20, 2025
Merged

feat!: Indexes for nodes in the AST#414
anakrish merged 1 commit into
microsoft:mainfrom
anakrish:node-index

Conversation

@anakrish

@anakrish anakrish commented May 27, 2025

Copy link
Copy Markdown
Collaborator

Indexes allow associating extra data with nodes in the AST
using an array and then quickly looking up the array to fetch
the extra data.

  • Index eidx for expressions
  • Index sidx for statements
  • Index qidx for queries.

AST nodes are not cloneable. Therefore once a module is created,
it is not possible to accidentally create two nodes with the same
index inadvertently via clone.

Also added IndexChecker in debug builds. When a module is parsed,
it will assert that indexes have been constructed correctly.

AST Cleanup

  • Make literal expressions (null, val, number, string etc) also structs
    to match all other expressions
  • Merge True and False nodes into a single Bool node.

Also update dependencies.

@anakrish
anakrish force-pushed the node-index branch 2 times, most recently from 0371a43 to be98e95 Compare May 27, 2025 16:23
Comment thread src/scheduler.rs Fixed
@anakrish
anakrish force-pushed the node-index branch 2 times, most recently from ede5e4c to 46b9f92 Compare May 27, 2025 16:39
@anakrish
anakrish marked this pull request as ready for review May 27, 2025 16:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces new indexing fields for AST nodes (eidx, sidx, and qidx) and updates AST node representations from tuple variants to struct variants for consistency, along with minor AST cleanup such as merging True/False into Bool.

  • Updated AST node variants to include explicit index fields.
  • Refactored pattern matching throughout the parser, interpreter, and traversal functions to use struct variants.
  • Adjusted test support and utilities to align with the new AST design.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/parser/mod.rs Updated pattern matching for literals and expressions to use struct-style AST nodes.
src/utils.rs Modified extraction routines to reflect the new struct variant for variable and string nodes.
src/scheduler.rs Revised match arms and pattern matching to use the new AST node formats including Bool nodes.
src/parser.rs Added new index incrementation methods and updated AST construction for number, string, call, etc.
src/interpreter.rs Adapted evaluation routines to the updated AST; ensuring calls, var lookups, and refs are matched correctly.
src/ast.rs Reworked AST definitions to replace tuple variants with struct variants; introduced new index fields.
Comments suppressed due to low confidence (1)

src/ast.rs:106

  • [nitpick] Consider renaming 'eidx' to 'expr_index' (and similarly 'sidx' and 'qidx') for improved clarity, unless the abbreviated names are a documented and intentional style in the codebase.
pub enum Expr {

Comment thread src/ast.rs Outdated
Comment thread src/ast.rs
@cemheren

Copy link
Copy Markdown

pub struct Interpreter {

unrelated to this PR, are there some good ways to split this struct up? 3000 line files are a bit hard to follow logically


Refers to: src/interpreter.rs:40 in 46b9f92. [](commit_id = 46b9f92, deletion_comment = False)

Comment thread src/parser.rs
@anakrish

Copy link
Copy Markdown
Collaborator Author

pub struct Interpreter {

unrelated to this PR, are there some good ways to split this struct up? 3000 line files are a bit hard to follow logically

Refers to: src/interpreter.rs:40 in 46b9f92. [](commit_id = 46b9f92, deletion_comment = False)

One kind of split I've been thinking of doing is to move expression evaluation into a separate entity, followed by query evaluations and then rules. That should give a clearer logical separation of the interpreter components. I haven't yet gotten a chance to take this on. I will likely squeeze this in along with other tasks to see how it works out.

Comment thread tests/parser/mod.rs
dekomissMSFT
dekomissMSFT previously approved these changes May 29, 2025
Comment thread src/lib.rs Fixed
Comment thread src/indexchecker.rs Fixed
@anakrish
anakrish force-pushed the node-index branch 6 times, most recently from e06b9fb to cc501b3 Compare June 2, 2025 17:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces indexing for AST nodes to enable associating and verifying auxiliary data via continuous integer IDs. It instruments expressions, statements, and queries with eidx, sidx, and qidx, updates the parser and interpreter to emit these indexes, and adds an IndexChecker to debug builds to assert uniqueness and continuity. Test fixtures are updated with expected index annotations and module-level counts.

  • Added eidx to all Expr variants, sidx to LiteralStmt, and qidx to Query.
  • Extended Parser to assign and propagate indexes via next_eidx, next_sidx, and next_qidx, and integrated IndexChecker in Module construction.
  • Created src/indexchecker.rs to validate that indexes are unique, continuous, and match module count fields; updated tests with index annotations and count metadata.

Reviewed Changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/ast.rs Added eidx, sidx, qidx fields on AST types.
src/parser.rs Instrumented parser to assign and carry indexes.
src/indexchecker.rs New debug-only module to assert index invariants.
src/utils.rs, src/scheduler.rs, Updated pattern matches for new struct variants.
src/interpreter.rs, src/lib.rs Propagated index changes through interpreter paths.
tests/parser/cases/**/*.yaml Annotated expected eidx, sidx, qidx and counts.
build.rs Ensured rebuild on build.rs changes.
Comments suppressed due to low confidence (3)

tests/parser/cases/expressions/membership.yaml:15

  • This test only specifies num_expressions; please also add num_statements: 0 and num_queries: 0 so the IndexChecker can validate statement and query counts.
num_expressions: 17

tests/parser/cases/expressions/in.yaml:16

  • Add num_statements: 0 and num_queries: 0 alongside num_expressions to ensure the index checker verifies all counts for this module.
num_expressions: 21

tests/parser/cases/expressions/array-compr.yaml:122

  • Duplicate eidx: 15 appears twice in this comprehension; expression indexes must be unique and should be incremented (e.g., the second occurrence should be eidx: 16).
eidx: 15

@cemheren

Copy link
Copy Markdown

Approved

Comment thread src/builtins/opa.rs Outdated
Comment thread tests/parser/mod.rs Outdated
dekomissMSFT
dekomissMSFT previously approved these changes Jun 20, 2025
@dekomissMSFT

Copy link
Copy Markdown
Collaborator

nit for future PRs: IMO it is much easier to review new iterations of the PR if the new changes are pushed as separate commits. Is there a benefit to force pushing instead?

@anakrish

Copy link
Copy Markdown
Collaborator Author

nit for future PRs: IMO it is much easier to review new iterations of the PR if the new changes are pushed as separate commits. Is there a benefit to force pushing instead?

Good point. I have been force-pushing out of habit. I am not really sure anymore why that pattern was following in previous projects. I agree that it is easier to review if commits aren't force pushed.

dekomissMSFT
dekomissMSFT previously approved these changes Jun 20, 2025
Indexes allow associating extra data with nodes in the AST
using an array and then quickly looking up the array to fetch
the extra data.

- Index eidx for expressions
- Index sidx for statements
- Index qidx for queries.

AST nodes are not cloneable. Therefore once a module is created,
it is not possible to accidentally create two nodes with the same
index inadvertently via clone.

Also added IndexChecker in debug builds. When a module is parsed,
it will assert that indexes have been constructed correctly.

AST Cleanup
- Make literal expressions (null, val, number, string etc) also structs
  to match all other expressions
- Merge True and False nodes into a single Bool node.

Also update dependencies.

Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>
@anakrish
anakrish merged commit 444b297 into microsoft:main Jun 20, 2025
33 checks passed
@anakrish
anakrish deleted the node-index branch June 20, 2025 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants