feat: implement basic expressions, function calls and literals - #2
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe repository adds an Expressif Tree-sitter grammar with generated parser artifacts, C, Node.js, and Python bindings, build configurations, syntax highlighting, parser corpus tests, documentation, and CI workflows. ChangesExpressif parser implementation
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant TreeSitterCLI
participant GeneratedParser
participant LanguageBinding
participant Parser
Developer->>TreeSitterCLI: generate parser artifacts
TreeSitterCLI->>GeneratedParser: write grammar and parser outputs
LanguageBinding->>GeneratedParser: load tree_sitter_expressif
GeneratedParser->>Parser: provide TSLanguage
Parser-->>LanguageBinding: expose configured language
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (8)
queries/highlights.scm (1)
1-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider capturing
escape_sequenceand the punctuation tokens.The grammar defines a named
escape_sequencenode. The query does not capture it, so escapes insidedouble_quoted_literalrender with the plain string color. The delimiters|,(,), and,are also uncaptured.These additions are optional and only affect editor rendering.
♻️ Proposed additions
[ (double_quoted_literal) (backtick_quoted_literal) ] `@string` +(escape_sequence) `@string.escape` + +"|" `@operator` + +[ + "(" + ")" +] `@punctuation.bracket` + +"," `@punctuation.delimiter` + [ (date_literal) (date_time_literal) (time_literal) ] `@string.special`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@queries/highlights.scm` around lines 1 - 15, Update the highlight query to capture the named escape_sequence node with an appropriate escape-related capture, and add punctuation captures for the |, (, ), and , tokens. Keep the existing function, number, boolean, string, and date/time captures unchanged.setup.py (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
setup.pydepends on two deprecated packaging APIs. Both sites rely on packaging helpers that setuptools andwheelhave deprecated and plan to remove, so a future dependency upgrade breaks the Python build rather than warning.
setup.py#L8-L8: replace thewheel.bdist_wheelimport withsetuptools.command.bdist_wheel, and keep atry/except ImportErrorfallback to thewheellocation for older setuptools versions.setup.py#L13-L16: replace the distutils-providedself.copy_tree("queries", dest)call withshutil.copytree("queries", dest, dirs_exist_ok=True)and importcopytreeat the top of the file.Pin a minimum
setuptoolsversion in the build requirements oncesetuptools.command.bdist_wheelis used, so the fallback path is only exercised where it is needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.py` at line 8, The deprecated packaging APIs in setup.py must be replaced: at setup.py lines 8-8, import bdist_wheel from setuptools.command.bdist_wheel with an ImportError fallback to wheel.bdist_wheel; at lines 13-16, replace self.copy_tree("queries", dest) with shutil.copytree using dirs_exist_ok=True and add the required import. Also pin a minimum setuptools version in the build requirements to support the preferred import while retaining the fallback for older versions.test/corpus/invalid.txt (2)
165-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRejection coverage is missing for several constructs named in the objectives.
The objectives require rejection of variables, references, named arguments, spread arguments, operators, and unquoted textual arguments.
This file covers variables at Lines 88-103 and unquoted textual arguments at Lines 165-180. It does not cover named arguments, spread arguments, operators, or references.
Add cases for the remaining constructs, for example
foo(a: 1),foo(...a),1 + 2, and the reference syntax the language reserves.Do you want me to draft these corpus cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/corpus/invalid.txt` around lines 165 - 180, Add rejection cases to test/corpus/invalid.txt for the missing constructs: named arguments such as foo(a: 1), spread arguments such as foo(...a), operators such as 1 + 2, and the language’s reserved reference syntax. Match the existing invalid-corpus structure and expected ERROR parse trees, while preserving the current variable and unquoted-text cases.
9-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the exact Tree-sitter CLI version for corpus error trees.
test/corpus/invalid.txtrecords full error-recovery output, butpackage.jsononly declarestree-sitter-cliwith a caret bump policy andpackage-lock.jsonis uncommitted. Switch the dependency to an exact patch version or add the file-local CLI/recovery-source note so lockfile refreshes cannot change these expected trees.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/corpus/invalid.txt` around lines 9 - 17, Pin the tree-sitter-cli dependency in package.json to an exact patch version, and commit the resulting package-lock.json so corpus error trees remain reproducible. Use the dependency declaration as the change point; do not alter the expected tree in test/corpus/invalid.txt.test/corpus/expressions.txt (2)
190-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name claims preservation, but the assertion does not check it.
The case is named "Backtick text preserves punctuation and whitespace". The expected tree contains only
(backtick_quoted_content)with no source text. The test passes for any single content token, so it does not verify that the leading space, the comma, the parentheses, and the trailing space are inside the literal.Add a second case that shows the distinction, for example a backtick literal used as a positional argument next to an unquoted equivalent that is rejected. Also consider adding cases for a temporal literal and a backtick literal used as call arguments. No such case exists today.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/corpus/expressions.txt` around lines 190 - 204, Expand the backtick literal corpus tests so “Backtick text preserves punctuation and whitespace” verifies the preserved content rather than only the backtick_quoted_content node. Add a contrasting positional-argument case using a backtick literal beside an equivalent unquoted value that is rejected, and add coverage for temporal literals and backtick literals passed as call arguments.
1-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the empty parenthesized call test the parentheses.
lowerandlower()currently produce the same S-expression because Tree-sitter corpus tests hide anonymous tokens. Convert only thelower()case to:cst-style output, as supported bytree-sitter test; the expected tree will then include the"("and")"tokens and block removal of theoptional(seq("(", ..., ")"))branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/corpus/expressions.txt` around lines 1 - 29, Update only the “Empty parenthesized function call” corpus test to use :cst output, preserving the existing bare lower test. Include the anonymous "(" and ")" tokens in that test’s expected tree so it distinguishes lower() from lower() without parentheses and validates the optional parenthesized-call grammar branch.src/node-types.json (1)
146-164: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffUse supertypes for the choice-only wrapper rules.
value,quoted_literal,temporal_literal, andexpressionadd visible CST layers because they are plainchoicerules ingrammar.js. Adding them tosupertypesremoves the wrapper nodes while keeping them available as Tree-sitter query unions, which avoids downstream consumers needing to navigate every wrapper. This is an optional CST-shape decision, but if applied it requiresnpm run generateand corpus test updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/node-types.json` around lines 146 - 164, Add value, quoted_literal, temporal_literal, and expression to the grammar’s supertypes configuration so these choice-only wrappers are omitted from the CST while remaining available as query unions. Regenerate the node types with npm run generate and update any affected corpus expectations..github/workflows/ci.yml (1)
23-24: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence for both checkout steps.
Both
actions/checkoutsteps use the defaultpersist-credentials: true. No visible step needs authenticated Git operations. Setpersist-credentials: falsein both steps. The action documentstrueas the default. (github.com)Proposed fix
- name: Check out repository uses: actions/checkout@v6 + with: + persist-credentials: falseApply the same change to the checkout step in the
csharpjob.[enhancement]
Also applies to: 37-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 23 - 24, Update both actions/checkout steps in the CI workflow, including the checkout step in the csharp job, to set persist-credentials to false. Keep the existing checkout configuration unchanged otherwise.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/dependabot.yml:
- Around line 9-17: Align both binding references with the repository layout: in
.github/dependabot.yml lines 9-17, point the pip ecosystem at the directory
containing pyproject.toml and setup.py and remove or relocate the NuGet entry
until a NuGet manifest exists; in README.md lines 69-72, list bindings/c,
bindings/node, and bindings/python instead of the C# and TypeScript paths.
In @.github/workflows/ci.yml:
- Around line 49-53: Guard C# project discovery in the PowerShell workflow step
by checking that bindings/csharp exists before calling Get-ChildItem, while
preserving found=False and the skip message when it is absent. Apply the same
existence check to the Restore and Build C# jobs so they skip cleanly without
enumerating a missing path.
In `@bindings/node/binding_test.js`:
- Around line 5-10: Update the package test script in package.json (lines 17-20)
to include node --test bindings/node/binding_test.js, ensuring the can load
grammar test in bindings/node/binding_test.js (lines 5-10) runs as part of npm
test; no direct change is required in the test file.
In `@bindings/node/index.d.ts`:
- Around line 12-19: Update the NodeInfo type union to match the node-types.json
contract: model children as a single ChildNode object rather than an array, and
make super_types, fields, and children optional where required for supertype and
field-only variants. Preserve the existing BaseNode intersection while adjusting
the union branches so TypeScript consumers receive the published nodeTypeInfo
shape.
In `@CMakeLists.txt`:
- Around line 74-76: Ensure parser generation runs before native tests: update
the ts-test custom target in CMakeLists.txt lines 74-76 to depend on
src/parser.c, and change the test target declaration in Makefile lines 113-114
to test: $(PARSER), preserving the existing test commands.
In `@CONTRIBUTING.md`:
- Line 13: Correct the issue-search sentence in CONTRIBUTING.md by changing
“hasn't already be submitted” to “hasn't already been submitted.”
- Around line 44-46: Use the repository identity Expressif.Syntax consistently
in CONTRIBUTING.md: at lines 44-46, direct pull requests to
Seddryck/Expressif.Syntax; at line 3, replace Expressif-Syntax with
Expressif.Syntax. In SECURITY.md line 5, replace Expressif-Parser with
Expressif.Syntax.
- Around line 21-27: Update the topic-branch guidance in CONTRIBUTING.md to
consistently use the latest origin/main as the base, replacing references to
master and local main. Adjust the example git checkout command accordingly, and
state that pull requests must target main.
- Around line 37-40: Update the documentation commit guidance in the
“Documentation” section of CONTRIBUTING.md by replacing the “(doc)” commit
prefix with the Conventional Commits “docs:” prefix, while preserving the
existing exception for trivial comment and documentation changes.
- Around line 30-33: Update the contribution checklist around the test and
AppVeyor bullets to reflect the active GitHub Actions workflows: instruct
contributors to verify the C# binding when bindings/csharp exists and run the
parser tests, and remove the AppVeyor status check unless it is still a required
CI job.
In `@grammar.js`:
- Line 13: Update the grammar’s extras handling so newline characters are not
skipped while scanning quoted literals, while preserving whitespace skipping
elsewhere. Clarify the intended newline behavior in the grammar and add a corpus
test covering newlines inside both double-quoted and backtick literals,
verifying they are rejected or preserved as intended.
In `@pyproject.toml`:
- Around line 24-25: Update the core optional dependency version constraint for
tree-sitter to require a lower bound that supports parser ABI 15, while
retaining the existing compatible upper-bound behavior. Ensure the dependency
used by the Python binding tests and generated parser is not allowed to resolve
to 0.24.0.
In `@SECURITY.md`:
- Line 5: Correct the misspelled “vulnerabilty” link text in the
security-reporting sentence to “vulnerability,” leaving the link destination and
surrounding documentation unchanged.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 23-24: Update both actions/checkout steps in the CI workflow,
including the checkout step in the csharp job, to set persist-credentials to
false. Keep the existing checkout configuration unchanged otherwise.
In `@queries/highlights.scm`:
- Around line 1-15: Update the highlight query to capture the named
escape_sequence node with an appropriate escape-related capture, and add
punctuation captures for the |, (, ), and , tokens. Keep the existing function,
number, boolean, string, and date/time captures unchanged.
In `@setup.py`:
- Line 8: The deprecated packaging APIs in setup.py must be replaced: at
setup.py lines 8-8, import bdist_wheel from setuptools.command.bdist_wheel with
an ImportError fallback to wheel.bdist_wheel; at lines 13-16, replace
self.copy_tree("queries", dest) with shutil.copytree using dirs_exist_ok=True
and add the required import. Also pin a minimum setuptools version in the build
requirements to support the preferred import while retaining the fallback for
older versions.
In `@src/node-types.json`:
- Around line 146-164: Add value, quoted_literal, temporal_literal, and
expression to the grammar’s supertypes configuration so these choice-only
wrappers are omitted from the CST while remaining available as query unions.
Regenerate the node types with npm run generate and update any affected corpus
expectations.
In `@test/corpus/expressions.txt`:
- Around line 190-204: Expand the backtick literal corpus tests so “Backtick
text preserves punctuation and whitespace” verifies the preserved content rather
than only the backtick_quoted_content node. Add a contrasting
positional-argument case using a backtick literal beside an equivalent unquoted
value that is rejected, and add coverage for temporal literals and backtick
literals passed as call arguments.
- Around line 1-29: Update only the “Empty parenthesized function call” corpus
test to use :cst output, preserving the existing bare lower test. Include the
anonymous "(" and ")" tokens in that test’s expected tree so it distinguishes
lower() from lower() without parentheses and validates the optional
parenthesized-call grammar branch.
In `@test/corpus/invalid.txt`:
- Around line 165-180: Add rejection cases to test/corpus/invalid.txt for the
missing constructs: named arguments such as foo(a: 1), spread arguments such as
foo(...a), operators such as 1 + 2, and the language’s reserved reference
syntax. Match the existing invalid-corpus structure and expected ERROR parse
trees, while preserving the current variable and unquoted-text cases.
- Around line 9-17: Pin the tree-sitter-cli dependency in package.json to an
exact patch version, and commit the resulting package-lock.json so corpus error
trees remain reproducible. Use the dependency declaration as the change point;
do not alter the expected tree in test/corpus/invalid.txt.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e09e864-1207-4138-bb17-b41ca7629a9f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (39)
.editorconfig.gitattributes.github/conventional_commits_labels.json.github/dependabot.yml.github/release.yml.github/workflows/ci.yml.gitignoreAGENTS.mdCMakeLists.txtCONTRIBUTING.mdMakefileREADME.mdSECURITY.mdbinding.gypbindings/c/tree-sitter-expressif.pc.inbindings/c/tree_sitter/tree-sitter-expressif.hbindings/node/binding.ccbindings/node/binding_test.jsbindings/node/index.d.tsbindings/node/index.jsbindings/python/tests/test_binding.pybindings/python/tree_sitter_expressif/__init__.pybindings/python/tree_sitter_expressif/__init__.pyibindings/python/tree_sitter_expressif/binding.cbindings/python/tree_sitter_expressif/py.typedgrammar.jspackage.jsonpyproject.tomlqueries/highlights.scmsetup.pysrc/grammar.jsonsrc/node-types.jsonsrc/parser.csrc/tree_sitter/alloc.hsrc/tree_sitter/array.hsrc/tree_sitter/parser.htest/corpus/expressions.txttest/corpus/invalid.txttree-sitter.json
| - package-ecosystem: "nuget" | ||
| directory: "/bindings/csharp" | ||
| schedule: | ||
| interval: "weekly" | ||
|
|
||
| - package-ecosystem: "pip" | ||
| directory: "/bindings/python" | ||
| schedule: | ||
| interval: "weekly" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Align the binding layout across configuration and documentation.
The repository binding layout is inconsistent between .github/dependabot.yml and README.md. The supplied project exposes C, Node, and Python bindings, but these files reference C# and TypeScript directories and place Python packaging under the wrong directory. This can suppress automated dependency updates.
.github/dependabot.yml#L9-L17: point the pip update to the directory containingpyproject.tomlandsetup.py, and remove or relocate the NuGet block until an actual NuGet manifest exists.README.md#L69-L72: listbindings/c,bindings/node, andbindings/python.
[bug]
📍 Affects 2 files
.github/dependabot.yml#L9-L17(this comment)README.md#L69-L72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/dependabot.yml around lines 9 - 17, Align both binding references
with the repository layout: in .github/dependabot.yml lines 9-17, point the pip
ecosystem at the directory containing pyproject.toml and setup.py and remove or
relocate the NuGet entry until a NuGet manifest exists; in README.md lines
69-72, list bindings/c, bindings/node, and bindings/python instead of the C# and
TypeScript paths.
Summary
Impact
This establishes the initial concrete syntax tree required for future Expressif grammar features while keeping callable resolution and validation in the semantic binding layer.
Validation
npm run generatenpm test— 31/31 corpus tests passednpm pack --dry-rungit diff --checkClose #1
Summary by CodeRabbit