fix(parser): omit calls from dead guards in C/C++, Go, TS and JS#666
Open
HouMinXi wants to merge 3 commits into
Open
fix(parser): omit calls from dead guards in C/C++, Go, TS and JS#666HouMinXi wants to merge 3 commits into
HouMinXi wants to merge 3 commits into
Conversation
The ast-based reachability pass only sees Python, so a call inside
`#if 0`, `if false { ... }` or `if (0) { ... }` still produced a CALLS
edge in every other language. Callers, impact radius and dead-code
queries then traversed code the compiler or preprocessor drops.
Detect those guards on the tree-sitter side, next to the Python pass:
- Go / TypeScript / JavaScript: an `if` whose condition is the literal
`false`, or `0` for TS/JS. The condition arrives wrapped in a
`parenthesized_expression` in TS/JS and is unwrapped first.
- C / C++: `#if 0` and `#elif 0` preprocessor blocks. That walk ignores
function and class scope, because the preprocessor removes the text
before the compiler ever sees it.
Only the consequence is dead; `else`, `#else` and `#elif` branches keep
their edges, as does `if (true)` and any non-literal condition. A
declaration nested in a dead branch is dead as well, matching how the
Python pass already treats a `def` or `class` under `if False:` --
JS/TS class declarations are not hoisted, so nothing reachable is lost.
Numeric detection is limited to the literal `0`. `0x0` and `0.0` are
equally falsy but stay live: a missed guard only drops a suppression,
while evaluating arbitrary numeric literals risks suppressing a live
call.
Tests cover each language plus the negative cases, and assert at the
store level that a dead target never reaches a graph consumer.
Signed-off-by: Minxi Hou <houminxi@gmail.com>
code-review-graph reviewOverall risk: 0.65 (MEDIUM) — 68 changed function(s)/class(es), 0 affected flow(s), 31 test gap(s) Risk-scored changes
Test gaps
Token savings: this graph-backed report used ~154,018 fewer tokens (~94%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
The automated review flagged _node_is_in_child, _is_statically_false_condition, _is_in_static_dead_guard and _extract_calls as lacking direct tests. The behaviour-level tests exercise them through parse_file(), but these tests call them directly with tree-sitter nodes so every branch is provably hit. 24 tests covering: - _node_is_in_child: direct child, nested, sibling, self, root - _is_statically_false_condition: false, 0, parens, true, 1, variable - _is_in_static_dead_guard: Go/TS/C dead + live + else + no-guard - _extract_calls: integration verifying dead calls omitted per language
rajpratham1
approved these changes
Jul 19, 2026
rajpratham1
left a comment
rajpratham1
left a comment
There was a problem hiding this comment.
This pull request introduces a well-designed enhancement for detecting and excluding calls located in statically unreachable code across C/C++, Go, TypeScript, and JavaScript. The parser changes are focused on dead-guard detection without impacting live execution paths, and the implementation is supported by comprehensive fixture-based, integration, and helper-level unit tests covering multiple edge cases. The changes improve the accuracy of generated call graphs while maintaining consistency across supported languages. No blocking issues were identified during review. LGTM and approved.
Contributor
Author
|
@tirth8205 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
f84b7d1was the right call, and usingastfor Python gets things asyntactic walk cannot: alias resolution, shadowing, and boolean
combinations. This PR does not touch that path, and does not try to
replace it. It only covers the languages
astcannot reach.Every non-Python language still emits dead edges:
ast.parse()raisesSyntaxErroron C, Go, TypeScript and Rust, andtree-sitter-language-packis the only multi-language parser in thedependency set, so the non-Python path has to be a tree-sitter walk.
This PR adds one, alongside the Python pass rather than replacing it.
_python_unreachable_call_positionskeeps ownership of Python.What it detects
if false { ... }if (false) { ... },if (0) { ... }#if 0,#elif 0The TS/JS condition arrives wrapped in a
parenthesized_expressionandis unwrapped before matching;
0is node typenumberthere, notPython's
integer. The C/C++ walk deliberately ignores function andclass scope, because the preprocessor deletes the text before the
compiler sees it, so a call inside a whole function wrapped in
#if 0is dead too.
What stays live
Only the consequence is dead. These keep their edges, and each has a
test:
else,#else,#elifbranches of a dead guardif (true),if (1), and any non-literal conditionif (a === false),if (foo(false)), which are not literal conditionsDirection matters here: the risk worth guarding against is suppressing a
live call, so every ambiguous case is left live.
Semantics for nested declarations
A function or class declared inside a dead branch is never evaluated, so
calls in its body are dead. That matches the Python pass, which already
drops calls inside a
deforclassunderif False:. JS/TS classdeclarations are not hoisted, so no reachable symbol is lost by treating
them this way.
Scope
Numeric detection is the literal
0only.0x0and0.0are equallyfalsy but stay live: a missed guard just drops a suppression, whereas
evaluating arbitrary numeric literals risks the opposite error. Noted in
a comment at the check.
Not included:
#ifdef/#ifndef(needs macro state, not available toa syntactic walk), and Rust, which is ready but held back to keep this
diff reviewable.
Tests
tests/test_parser.py, one per language plus:test_dead_guard_covers_declarations_nested_in_dead_branch:nested-declaration semantics above.
test_dead_guard_calls_absent_from_graph_store: builds a realGraphStorefrom each fixture and asserts the dead target neverappears in
get_all_call_targets(), mirroring the consumer-levelcheck in
test_python_reachability.py.Each test was verified by injecting the bug it targets, confirming the
failure, then reverting; same drill for the 24 direct helper tests added
in bc1936e (gate disabled: the integration tests fail; helper broken:
the literal and fixture tests fail; restored: all green).
Full suite at bc1936e: 1989 passed, 5 skipped, 2 xpassed, plus one
failure in test_embedding_initialization that is pre-existing: it fails
identically at the v2.3.7 base in a clean worktree, and only on Python
3.14 Linux, which the CI matrix does not run. The 1966 figure I first
posted was from a run on a different branch; corrected.
Python behavior is unchanged; the ast path is untouched.
Questions for you
I would rather match your design than land my own, so please push back
on any of this:
_python_unreachable_call_positionsso the two reachability passessit together, and added a single gate in
_extract_calls. If youwould prefer them in their own module, or the gate somewhere else,
say the word and I will move them.
would you rather have one small table of "statically false condition"
node types per language that this and future languages read from? I
kept it inline because there are only three shapes today, but I am
happy to build the table if you expect more languages.
if false, andcfg!(any())distinguished from
cfg!(any(feature = "x"))), held back only tokeep this diff small. Want it here, as a follow-up, or not at all?
#ifdef/#ifndef. Left out because it needs macro staterather than syntax. If you think an approximation is worth having
for the common
#ifdef UNDEFINED_MACROcase, I will look at it.a direction in mind for non-Python reachability, I am glad to drop
this and implement yours instead.
Happy to split, rebase, or rework this however suits the project.