fix(nargs): count a macro-obscured declarator's own arguments - #1215
Merged
Conversation
An annotation macro carrying an argument -- `T *f() TF_LOCKS_EXCLUDED(mu_)`, the TensorFlow / Abseil spelling -- is a `function_declarator` with a `parameters` field, so the walk names the space after the macro rather than after the function. C parses it cleanly and gets `f`; C++, mozcpp and Objective-C recover into an `ERROR` and get the macro. That split is not the two-two of the parameterless spelling, so a fixture in either alone misreports the other. Recovery trees are outside the walk's contract, so this pins the behaviour rather than claiming it is right. Also corrects #1208's measured corpus counts, which the changelog and the walk's doc both understated: 46 spaces named, 2 un-named, 4 renamed, for a net 44 fewer nameless spaces.
An unexpanded function-like macro standing where the declared name belongs -- `RUN_STATS_METHOD(allocate)(JNIEnv *env, jclass clazz)`, the JNI shim idiom -- nests one `function_declarator` directly inside another. Since #1200 `nargs` read the innermost, so the macro's `(allocate)` was the answer and the function's own arguments were discarded: TensorFlow's four `run_stats_jni.cc` shims all reported 1 while declaring 2, 3, 4 and 3, and `bca check --threshold nargs=1` found one violation in that file where five belong. The rule is structural, not a guess about macros. Neither language lets a function return a function type (C11 6.7.6.3p1, C++ [dcl.fct]), so the direct nesting cannot be a declarator chain -- a legitimate function returning a function pointer interposes a `parenthesized_declarator`. Both links have to be tested: a pointer return puts the outer declarator in a `pointer_declarator`'s `declarator` field, and stopping there reports 0. The space keeps the macro's name, so the 44 names #1208 recovered are unaffected. After `##` pasting the real symbol is not in the source at all, and the macro is the token a reader greps for. Arity now comes off the outer declarator and the name off the invocation it wraps, which retires #1208's same-node pairing in favour of one function, one walk. 46 corpus spaces change, none of them snapshotted. 26 are macro shims, fixed; the other 20 are `TF_ASSIGN_OR_RETURN(...); if (...)` statements that tree-sitter recovers into the same shape, which was never inside the walk's contract and was not reporting an arity before either. Fixes #1213
…tack Replaces `then(..).flatten()` in a `successors` closure with a plain `while`: four lines shorter, no combinator stack, and the `?` returns `None` rather than a `function_declarator` every caller rejects on kind. Also trims the module doc's closing paragraph, which restated what both function docs say with more precision, and corrects two test-row comments that overclaimed -- the `char *MACRO(n)(..)` row does not exclusively guard the gate's `current.kind()` half (#1200's own pointer rows also fail without it), and the `operator()` measurement belongs on the C++ row rather than on `int (*g(void))[4]`.
The clean/recovery split of the 46 affected corpus spaces was 26/20; it is 27/19. The classifier behind the first figure flagged an outer declarator as recovery whenever an `ERROR` appeared anywhere in its subtree, which misfiled `run_stats_jni.cc:49` -- `RUN_STATS_METHOD(delete)` puts the `ERROR` inside the *inner* list while its own chain is clean, so it is a shim the fix corrects rather than a recovery tree. The `operator()` near-miss count of 179 was carried over unverified and is wrong: `bca functions` reports 1,546 such spaces across the corpora. The claim that matters is unaffected -- not one of the 46 direct nestings is an operator.
The row expected `(0, 0)` for `int (*g(void))[4]`, which is what the table reports when `innermost_declarator` returns `None` for every input -- so it passed with the whole walk dead while 28 sibling rows failed. Gives it two arguments instead. Also corrects the `operator()` row's comment, which implied it guards the gate against being widened. No widening can fail it: the chain already stops at that declarator. What it does guard is a future grammar bump spelling `operator()` as a nested `function_declarator`, which would halve the arity of 1,546 corpus spaces. Renames `the_innermost_declarator_names_the_function_space`: three of its rows now take the name from inside the innermost declarator rather than from it, so the old name read as a specification the body does not meet.
The `while` loop introduced in 30f53dc needed a `?` for "this `function_declarator` has no `declarator` field", a state all four grammars declare impossible and only an `ERROR` could produce. Coverage counts those arms, and no test can reach them: `c_declarator.rs` went from 6 uncovered regions to 8. Back to a `successors` chain, but with a plain `if` in the closure rather than the `then(..).flatten()` stack that prompted the rewrite. Same six uncovered regions as before the branch, all three of them pre-existing test-failure paths.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1215 +/- ##
=======================================
Coverage 98.31% 98.32%
=======================================
Files 277 277
Lines 71904 71991 +87
Branches 71474 71561 +87
=======================================
+ Hits 70696 70783 +87
Misses 796 796
Partials 412 412
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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
1 participant
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.
Fixes #1213
An unexpanded function-like macro standing where the declared name
belongs —
RUN_STATS_METHOD(allocate)(JNIEnv *env, jclass clazz), theJNI shim idiom — nests one
function_declaratordirectly insideanother. Since #1200
nargsread the innermost, so the macro's(allocate)was the answer and the function's own parameters werediscarded.
TensorFlow's four
run_stats_jni.ccshims each reported 1 whiledeclaring 2, 3, 4 and 3, so
bca check --threshold nargs=1found oneviolation in that file where five belong.
The rule is structural, not a guess about macros
Neither language lets a function return a function type (C11 6.7.6.3p1,
C++ [dcl.fct]), so a
function_declaratornested directly insideanother cannot be a declarator chain — a legitimate function returning a
function pointer interposes a
parenthesized_declarator, which isexactly the #1208 shape. Both links have to be tested: a pointer return
puts the outer declarator inside a
pointer_declarator'sdeclaratorfield, and stopping at the first link reports 0.
What this changes about #1208
#1208 landed on "the name and the arity come off the same node." That
was too strong. The invariant is one function, one walk — not one
node: for a macro-obscured declarator the arity belongs to the outer
link and the name to the invocation it wraps. The 44 names #1208
recovered are unaffected, and the space keeps the macro's name
deliberately — after
##pasting the real symbol is not in the sourceat all, and the macro is the token a reader greps for.
Measurements
46 corpus spaces change arity, none of them snapshotted:
TF_ASSIGN_OR_RETURN(...); if (...)statements that tree-sitter recovers into the same shape, which was never inside a function's contractNot one of the 46 is an
operator()— the near-miss worth ruling out,since
bca functionsreports 1,546 such spaces across the corpora and arule that caught them would halve their arity.
Also in this branch
Four corrections that came out of reviewing the above, each its own
commit:
test(c_declarator)— pinsT *f() TF_LOCKS_EXCLUDED(mu_), theannotated-macro spelling. It carries an argument, so it is a
function_declaratorwithparametersand the walk names the spaceafter the macro. It splits the grammars differently from the
parameterless spelling (C gets
f; C++, mozcpp and Objective-Crecover into an
ERRORand get the macro), so a fixture in eitheralone misreports the other. Recovery trees are outside the walk's
contract, so this pins the behaviour rather than claiming it is right.
docs— fix(getter): C-family function returning a function pointer resolves its space name to null #1208's changelog entry and the walk's doc both quotedthe net corpus figure. Re-measured: 46 named, 2 un-named, 4
renamed, for a net 44. One of those renames (
resource→TF_LOCKS_EXCLUDED) is a name regression against the pre-fix(getter): C-family function returning a function pointer resolves its space name to null #1208leftmost search, now named in the changelog with its baseline-key
consequence.
test(nargs)— theint (*g(void))[4]row expected(0, 0),which is also what the table reports when
innermost_declaratorreturns
Nonefor every input: it passed with the whole walk deadwhile 28 sibling rows failed. It now takes two arguments.
refactor+ follow-up — the name walk was rewritten as awhileloop, which needed a
?for a state all four grammars declareimpossible; coverage counts arms no test can reach, so
c_declarator.rswent from 6 uncovered regions to 8. Reverted to asuccessorschain with a plainif, back to the original six.make pre-commit→BCA_GATE: pass. No.snap.newunder the outputsubmodule; the submodule pointer is unchanged.