[sanskar717]

Summary

Adds Solidity modifier_invocation support to the existing Solidity language specification so modifier usages such as onlyOwner are tracked as decorators in the code graph.

Changes

• Added solidity_decorator_types with modifier_invocation
• Wired it into CBM_LANG_SOLIDITY
• Enables modifier definitions and invocations such as onlyOwner to be tracked correctly

Verification

Verified locally with search_code using onlyOwner:

• Found the modifier definition
• Found the onlyOwner invocation on withdraw

Relation to #1754

The issue proposes adding Solidity parser support. Since Solidity grammar is already vendored through Tree-sitter, this change extends the existing parsing path without introducing a Node.js runtime or JavaScript dependency.

This PR specifically fixes the missing modifier/decorator tracking in the current Solidity support.

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[sanskar717]

The issue specifically requested @solidity-parser/parser (JS). Since this project is pure C and uses a single-binary setup, I completed the existing tree-sitter-solidity path instead, focusing on modifier/decorator tracking.

Verified using search_code with onlyOwner, covering both the modifier definition and its invocations.

[DeusData]

Thank you for working within the existing C/tree-sitter Solidity path and clarifying the modifier-invocation scope. We need more time to review the extraction semantics and their regression coverage before giving a decision.

The review queue is currently full, so detailed feedback may take a little time. We are working through it carefully and appreciate the work you have put into supporting the project. Thank you for your patience.

[DeusData]

Here is the review this was waiting on since 2026-09-09. Eleven days for a two-line change is too long, and I am sorry.

First, the judgement call you made was the right one and I want to endorse it explicitly. The issue asked for @solidity-parser/parser, a JavaScript dependency. This project is a single C binary with vendored tree-sitter grammars, and adding a JS parser to it would have been a bigger architectural change than the feature justifies. Completing the existing tree-sitter-solidity path instead is exactly the call I would have made, and you explained the reasoning up front rather than quietly substituting one thing for another. That saved the review a round trip.

The change itself reads correctly: solidity_decorator_types = {"modifier_invocation"} puts Solidity on the same footing as java_decorator_types, rust_decorator_types and the rest, and feeds the existing extract_decorators path rather than inventing a parallel one.

What I need before merging is a regression test, and it is the one thing the maintainer note from 2026-09-09 was circling.

Right now nothing pins this behaviour. I checked: tests/test_grammar_labels.c:107 asserts a Solidity census of Class:1,Method:1,Module:1, and your CI is fully green — which tells us the change adds no nodes to that fixture, and therefore that no existing assertion would notice if this line were deleted tomorrow. A two-line table entry with nothing holding it in place tends to get reverted by accident during the next refactor of that table.

The shape to follow already exists next door — tests/test_grammar_probe_c.c:314 (solidity_contract_node) and :334 (solidity_interface_node) are the pattern. Something like a contract with a modifier:

pragma solidity ^0.8.0;
contract Vault {
    modifier onlyOwner() { _; }
    function withdraw() public onlyOwner { }
}

asserting that withdraw carries onlyOwner as a decorator. The test to aim for is one that fails with your two lines reverted — if it passes either way, it is not pinning anything, and this repository has shipped a few tests like that, so I check for it now rather than assume.

No rush and no pressure on timing. Push the test and I will review promptly — the table change itself is already settled as far as I am concerned.

[sanskar717]

Pushed the regression test (7036c4d) — solidity_modifier_decorator in test_extraction.c, following the pattern from extract_java_method_annotations_issue382.

Verified: reverting the table entry (solidity_decorator_types → empty_types) makes it fail with ASSERT(decorators_contain(m, "onlyOwner")); with the fix in place it passes.

[DeusData]

Thank you — the regression test is exactly what was asked for, and saying how you proved it (table entry reverted, ASSERT(decorators_contain(m, "onlyOwner")) fails; restored, it passes) is what makes it a test rather than a decoration.

The twelve red legs on this head are ours, not yours. Every one of them fails on the same test, which has nothing to do with Solidity:

tool_check_index_coverage_freshness_uses_indexer_mtime_source_issue1714
tests/test_mcp.c:5806: strstr(response, ""freshness":"metadata_match"") is NULL

That test asserted a JSON field without asking the tool for JSON; it was fixed on main on 20 September (96a2fc35), and your CI run was built against a main from before that fix. Your branch is 226 commits behind, so the verdict you are looking at is stale rather than wrong.

Next steps are mine: a local build of the merge result with the extraction suites, then I update the branch with main for a fresh run. I am holding that update for an hour or two only because the CI runner pool is saturated tonight. After a green matrix this merges. Nothing further is needed from you.

[sanskar717]

Side note: VS Code flags CLOCK_MONOTONIC as undefined at line 6049 (extract_wide_flat_ms) — time.h is included, so this may be Windows-specific (matches the windows-CLANG64 comment just above it). Not something I touched, flagging in case it's useful.

[DeusData]

Thanks for the regression and for flagging the CLOCK_MONOTONIC editor diagnostic. The earlier review says no further contributor work is needed; the promised merge-result verification and branch refresh are still maintainer-side steps. I have not established the cause of that editor diagnostic in this sweep.

[DeusData]

Thank you, @sanskar717, and sorry the branch refresh we promised on 21 September took this long. Your part was done then: the one-line table entry puts Solidity on the same decorator path as Java and Rust, and your regression test pins it (we re-checked that it fails with the entry removed, as you said).

What we changed on your branch, so you know what landed:

1. Merged current main in (one merge commit, signed off). main had added two Python route tests at the same spot in tests/test_extraction.c where you added solidity_modifier_decorator, so the branch no longer merged cleanly. We kept all three tests; your lang_specs.c change merged cleanly and is untouched.

That is the only change. Before pushing it we ran the merged tree on a real Solidity codebase, OpenZeppelin Contracts: with your entry the index gains 141 DECORATES edges from functions to the modifiers they use (onlyOwner, onlyRole, nonReentrant, initializer and so on), against none without it, and nothing else in the graph moves apart from one related-symbol edge that comes from the new decorator data.

The red CI on your old head was the stale-base test we mentioned before; the new run is against current main. We will merge once it is green. Thanks again for choosing the tree-sitter path and for explaining why up front.

[DeusData]

Thank you again for the Solidity modifier regression. The October updates record the branch refresh and decorator verification as completed on our side, so no repeat contributor work is requested. Sorry for the delay in the remaining maintainer follow-up—I’ll follow up soon.

[DeusData]

Thank you again, @sanskar717. One more housekeeping step on our side, and nothing is needed from you: we merged current main into your branch again (one signed-off merge commit, no conflicts). Your lang_specs.c entry and the solidity_modifier_decorator test are unchanged. The last CI run failed only on a repository-wide documentation count check that was fixed on main after your branch was last refreshed, so the run on the new head is the one that counts. If you push to this branch again, please pull first so the merge commit is kept. We will merge as soon as it is green.

[sanskar717]

Thanks a lot, @DeusData, and no need to apologize at all. I really appreciate the time and care you put into reviewing this, including the OpenZeppelin check and the branch refreshes. I won't push anything to the branch, so your merge commit stays intact. Happy to adjust anything if needed once CI finishes.

[DeusData]

Thanks again, @sanskar717. Several other pull requests landed on main this evening, including changes in files this one also touches, so we merged current main into your branch once more (one signed-off merge commit, 2382876; your changes are untouched). That keeps the required checks honest against today's tree; we will merge once they are green. Please pull before pushing again.

[DeusData]

Thank you for your patience, @sanskar717. CI went green on this branch, but tonight's merges on main touched tests/test_extraction.c, which this pull request also changes, so that green no longer covered today's main. We merged current main into the branch once more (one merge commit, 6217b49; your changes are untouched) and will merge once the fresh checks are green. Please pull before pushing again.

[DeusData]

Thank you for your patience, @sanskar717. CI went green on this branch, but this evening's merges on main touched files this pull request also changes, so that green no longer covered today's main. We merged current main into the branch once more (one merge commit, e2c9de1; your changes are untouched) and will merge once the fresh checks are green. Please pull before pushing again.

[DeusData]

Merged - thank you, @sanskar717! This puts Solidity modifiers such as onlyOwner and nonReentrant on the same decorator path as Java annotations and Rust attributes, so functions now link to the modifiers that guard them in the code graph. Two things you did made this easy to land: you chose the existing tree-sitter path over adding a JavaScript parser and explained why up front, and you proved your regression test fails with the table entry removed. Thank you for your patience with how long the review queue took.