feat(parsers): extract access modifiers and decorators via highlights.scm#566
feat(parsers): extract access modifiers and decorators via highlights.scm#566ChetanyaRathi wants to merge 12 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors decorator and modifier extraction across multiple languages by replacing language-specific handler methods with a unified utility that leverages tree-sitter highlights queries. It also updates the schema to store modifiers for classes, functions, and methods. The review feedback highlights three key issues: a bug in query loading where a module import failure incorrectly skips fallback queries, noisy modifier extraction that captures definition keywords like def or class, and a parsing failure in _decorator_tail_names when handling decorators with arguments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| module_name = f"{cs.TREE_SITTER_MODULE_PREFIX}{lang_name.replace('-', '_')}" | ||
| module = importlib.import_module(module_name) | ||
|
|
||
| query_str = "" | ||
| if hasattr(module, "HIGHLIGHTS_QUERY"): | ||
| query_str = module.HIGHLIGHTS_QUERY | ||
|
|
||
| fallback_path = ( | ||
| Path(__file__).parent / "queries" / "highlights" / f"{lang_name}.scm" | ||
| ) | ||
| if fallback_path.exists(): | ||
| custom_queries = fallback_path.read_text(encoding="utf-8") | ||
| query_str = ( | ||
| query_str + "\n" + custom_queries if query_str else custom_queries | ||
| ) | ||
|
|
||
| if query_str: | ||
| return Query(language, query_str) | ||
| except Exception as e: | ||
| logger.debug(f"Failed to load highlights query for {lang_name}: {e}") | ||
| return None |
There was a problem hiding this comment.
If the tree-sitter module import fails (e.g., ModuleNotFoundError or ImportError), the entire try block is aborted, which completely skips loading the fallback highlights query from fallback_path. This is a major bug because fallback queries should still be loaded even if the module import fails (or if the module is loaded from a submodule where the bindings path is no longer in sys.path). Separating the module import/query extraction from the fallback path loading using separate try-except blocks is much more robust and correct.
query_str = ""
try:
module_name = f"{cs.TREE_SITTER_MODULE_PREFIX}{lang_name.replace('-', '_')}"
module = importlib.import_module(module_name)
if hasattr(module, "HIGHLIGHTS_QUERY"):
query_str = module.HIGHLIGHTS_QUERY
except Exception as e:
logger.debug(f"Failed to import tree-sitter module for {lang_name}: {e}")
try:
fallback_path = (
Path(__file__).parent / "queries" / "highlights" / f"{lang_name}.scm"
)
if fallback_path.exists():
custom_queries = fallback_path.read_text(encoding="utf-8")
query_str = (
query_str + "\n" + custom_queries if query_str else custom_queries
)
if query_str:
return Query(language, query_str)
except Exception as e:
logger.debug(f"Failed to load highlights query for {lang_name}: {e}")
return None| def _decorator_tail_names(decorators: list[str]) -> set[str]: | ||
| return { | ||
| decorator.lstrip(cs.DECORATOR_AT).split(cs.SEPARATOR_DOT)[-1] | ||
| decorator.lstrip("@#[]() ").split(cs.SEPARATOR_DOT)[-1].rstrip(")]") | ||
| for decorator in decorators | ||
| } |
There was a problem hiding this comment.
The code decorator.lstrip("@#[]() ").split(cs.SEPARATOR_DOT)[-1].rstrip(")]") fails to correctly extract the bare name of decorators with arguments (e.g., @cached_property(ttl=3600) or @Component({selector: 'app-root'})). For @cached_property(ttl=3600), it returns cached_property(ttl=3600, which fails to match cached_property in cs.PROPERTY_DECORATORS. We should split on ( first to strip any arguments before extracting the tail name.
| def _decorator_tail_names(decorators: list[str]) -> set[str]: | |
| return { | |
| decorator.lstrip(cs.DECORATOR_AT).split(cs.SEPARATOR_DOT)[-1] | |
| decorator.lstrip("@#[]() ").split(cs.SEPARATOR_DOT)[-1].rstrip(")]") | |
| for decorator in decorators | |
| } | |
| def _decorator_tail_names(decorators: list[str]) -> set[str]: | |
| return { | |
| decorator.lstrip("@#[]() ").split("(")[0].split(cs.SEPARATOR_DOT)[-1].rstrip(")] ") | |
| for decorator in decorators | |
| } |
References
- When parsing decorators, annotations, or attributes, extract the full text including arguments, not just the name. This preserves crucial semantic information (e.g., arguments in
@RequestMapping(value="/api")or#[derive(Debug)]) for RAG queries and ensures consistency across all supported languages (Python, Java, Rust, TypeScript).
Greptile SummaryThis PR generalizes access-modifier and decorator extraction from a per-language
Confidence Score: 4/5The core extraction logic and the TSX fix are sound, but the non-combined parse path in
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AST node] --> B{parent type?}
B -->|decorated_definition / export_statement| C[target_node = parent]
B -->|other| D[target_node = node]
C --> E[sibling walk]
D --> E
E -->|prev sibling is attribute_item Rust| F[prepend sibling to query_nodes]
E -->|prev sibling is decorator on method_definition TS| F
F --> E
E -->|no more matching siblings| G[for each query_node]
G --> H{query_node == target_node?}
H -->|yes| I[set_byte_range: start to body_start]
H -->|no - sibling node| J[set_byte_range: full sibling]
I --> K[run highlights query]
J --> K
K -->|keyword.modifier / keyword| L[modifiers list]
K -->|function.decorator / attribute.*| M[decorators list]
L --> N[return modifiers, decorators]
M --> N
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[AST node] --> B{parent type?}
B -->|decorated_definition / export_statement| C[target_node = parent]
B -->|other| D[target_node = node]
C --> E[sibling walk]
D --> E
E -->|prev sibling is attribute_item Rust| F[prepend sibling to query_nodes]
E -->|prev sibling is decorator on method_definition TS| F
F --> E
E -->|no more matching siblings| G[for each query_node]
G --> H{query_node == target_node?}
H -->|yes| I[set_byte_range: start to body_start]
H -->|no - sibling node| J[set_byte_range: full sibling]
I --> K[run highlights query]
J --> K
K -->|keyword.modifier / keyword| L[modifiers list]
K -->|function.decorator / attribute.*| M[decorators list]
L --> N[return modifiers, decorators]
M --> N
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ChetanyaRathi
changed the title
|
Heads up: CI didn't run here — all jobs show "The job was not started because your account is locked due to a billing issue," which looks like a repo-level Actions billing problem rather than anything in this PR. Locally the full unit suite passes (only Windows-specific path/symlink/encoding tests fail, which pass on Linux CI), ruff check + format are clean, and ty check codebase_rag is clean. Happy to re-run once Actions is available. |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
vitali87
left a comment
vitali87
left a comment
There was a problem hiding this comment.
A few blockers before this can merge:
- Rebase — 248 commits behind
main, which has since reworked the parser surface this touches. Currently conflicting; needs a fresh diff. - Hardcoded strings —
extract_modifiers_and_decoratorsinparsers/utils.pyhardcodes strings that already have constants:
"decorated_definition"→cs.TS_PY_DECORATED_DEFINITION,
"export_statement"→cs.TS_EXPORT_STATEMENT,"attribute_item"→cs.TS_RS_ATTRIBUTE_ITEM, plus
"body"/"highlights"/the capture names/_EXCLUDED_KEYWORDS. Move these toconstants.py. - Scope — #525 wants 12 languages; this covers 6 (java, js, ts, php, python, rust). Change
ClosestoAdvances #525, or add C/C++/Go/Lua/Scala/C#. - CI — Type Check fails on
clang.cindex(stale-base artifact, should clear on rebase); Windows tests red (green on currentmain— confirm after rebase).
Re-review once rebased and constants are pulled out.
ChetanyaRathi
force-pushed
the
feat/525-highlights-modifiers-decorators
branch
from
9da44d5 to
7bdd479
Compare
|
Rebased onto main (v0.0.237), conflicts resolved against the reworked parser surface. Moved the hardcoded tree-sitter strings (decorated_definition, export_statement, attribute_item, body, highlights key, capture names, _EXCLUDED_KEYWORDS) into constants.py. Changed "Closes" to "Advances #525" since this covers 6 of the 12 languages. Local: 4315 passed, ruff check + format clean, ty check codebase_rag clean; only the 2 Windows symlink-privilege tests fail locally (pass on Linux CI). |
|
Post-rebase CI update: Lint and Type Check are green (the clang.cindex failure cleared on rebase, as expected). The only remaining red is windows-latest on two broken-symlink tests (test_hash_with_bytes_returns_none_for_broken_symlink and test_broken_symlink_does_not_crash_indexing), failing with WinError 1314 "A required privilege is not held" — that's Windows needing elevated privileges to create symlinks, not related to this PR. My diff doesn't touch test_graph_updater_incremental.py, and both pass on ubuntu and macOS. Happy to skip/guard them on unprivileged Windows if you'd like, but that seems out of scope here. |
|
@ChetanyaRathi confirmed one live correctness bug still on the rebased head (7bdd479): TypeScript method decorators are not populated. Greptile's outstanding P1 reproduces. Ran the shared extractor directly against the loaded parsers: Cause: in the TypeScript grammar the Fix: extend the extractor to pick up a preceding Separately, the modifier tests are all negative assertions ( |
ChetanyaRathi
force-pushed
the
feat/525-highlights-modifiers-decorators
branch
from
6589ab7 to
e1865be
Compare
|
Fixed all three:
|
All sorted already. |
vitali87
left a comment
vitali87
left a comment
There was a problem hiding this comment.
Almost there, can you please check what greptile tells about potential failure point? @ChetanyaRathi
…ghlights - Append custom fallback decorator highlights for languages missing them in upstream tree-sitter packages. - Expand modifier extraction to check wrapper nodes (like decorated_definition). - Remove obsolete decorator extraction tests.
…guage extraction tests
…lers and update ingest tests
…nition keywords, strip decorator args, capture Rust sibling attributes
…fier captures to fallback highlights; add positive modifier/decorator tests
ChetanyaRathi
force-pushed
the
feat/525-highlights-modifiers-decorators
branch
from
77a6b85 to
215d7b9
Compare
|
@ChetanyaRathi is this a big deal? Can you check please? The change is focused and covered by tests, but the shared highlights path introduces an observable extraction regression for PHP attributes. |
|
Good catch, checked it. It wasn't a crash, but it was a real bug — the PHP query was capturing the attribute twice, so #[Route('/x')] showed up as two decorators instead of one. Fixed it to capture only once, and added tests to confirm one attribute = one decorator (plus a test for multiple attributes). Pushed in a1d45a4. Only red check left is the flaky windows-py3.13 symlink test, which passes everywhere else. |
Yeah, I know about the windows flaky test. |
|
The only thing left is the uv.lock conflict, and it keeps re-conflicting on the version line because main is releasing several times a day (245 → 249 while I was rebasing). The actual code merges cleanly — GitHub shows "changes can be cleanly merged." Rather than chase another rebase that'll be stale again in an hour, could you take main's uv.lock when you merge? It regenerates deterministically from pyproject.toml, so there's no real content to resolve. All the review items are addressed (TS method decorators, fallback modifier captures, positive tests, function_ingest fix, PHP attribute dedup, constants, scope), and everything's green except the flaky windows-py3.13 symlink test. |
Don't worry about uv.lock conflict, I will fix it when all is correct. |
|
@ChetanyaRathi I tend ot merge PRs when they clear all greptile (5/5) and gemini comments. When you push a change, please invoke greptile with @greptile-apps to get your updates evaluated again in-place. This means that the greptile is adjusting its score on its first message. Right now it is stable 3/5 with potential issues. It used to be 4/5 at some point but since then we went down. The resolution of the issues is two way:
|
|
@greptile-apps Fixed the TSX highlights gap — TSX (.tsx) now reuses the TypeScript highlight query, so decorated TSX classes/functions get modifiers/decorators instead of None. Added a TSX regression test (test_tsx_decorated_method). Please re-review. |
3 participants
Advances #525 and #521.
Generalizes access-modifier and decorator extraction from Java-only to a single
shared, highlights.scm-driven path for all languages.
parser_loader.py; populates modifiers: list[str] and decorators: list[str] on
Function/Method/Class nodes (empty list when absent).
shared path, removing the bespoke logic; kept the handler Protocol consistent.
Testing: full unit suite green on CI targets; ruff check + ruff format clean;
ty check codebase_rag (--exclude tests) clean. Local Windows shows only
OS-specific failures (path separators, symlink privileges, cp1252 encoding,
libclang) that pass on Linux CI.