Skip to content

fix(getter/groovy): a var the grammar reads as a type_identifier bills as an operand #1706

Closed

Description

@dekobon

Summary

#1704 made Groovy's var bill as a declaration keyword where the dekobon grammar emits it as the keyword leaf ({var:60} under local_variable_declaration). The grammar spells the same keyword as a type_identifier in two other positions, and there it still bills as an operand named var — a type name that names no type:

source var node bills as
def f() { var x = 1 } var:60 operator var (#1704)
class C { var field = 1 } type_identifier:308 operand var
class C { static var s = 2 } type_identifier:308 operand var
def f() { final var q = 5 } type_identifier:308 operand var

Measured with bca ops --no-config on fix/batch-2026-10-23 after #1704. So one construct has two readings depending on whether a modifier or a class body precedes it, and the operand reading is the one Java had before #1560's text gate (Java's get_op_type_with_code returns Unknown for a type_identifier spelled var; var y; bills nothing — checked on the same build).

What the fix needs

The right answer is the #1704 one: operator var, keyed as the keyword. Two ways, neither free:

  1. Key it by text. Getter::get_operator_spelling(node, ancestors) is byte-less, so it cannot say "this type_identifier spells var". Widening it to take code is a big-code-analysis-ast trait change (outside STABILITY.md's shape contract, so allowed in a minor), after which the Groovy getter can answer Some("var") for a type_identifier whose bytes are var and get_op_type_with_code can return Operator. Groovy's Primitive row already does the text-gated half (spells_primitive_type) and the walk keys primitives by text through Checker::is_primitive, so the plumbing for "operator keyed by its spelling" exists for that one role and would need to grow one more case.
  2. Java parity, cheaper: return Unknown for the type_identifier var as Java does, so it bills nothing. Stops the bogus operand but leaves var x (operator) and var field (nothing) disagreeing.

Option 1 is the one that makes the table above read one line. Either way it moves Groovy's n2 on every field or modified local declared with var.

Related

#1704 (the keyword leaf), #1560 (Java's var gate), #1568 (Groovy's text-gated primitives), #1693 (the DeclKeyword row).

Status after #1599 (PR #1717)

Option 1 is unblocked: Checker::keys_operator_by_text(node, code, ancestors) is the byte-aware hook Halstead reads, and Groovy already overrides it for the Primitive row's text gate — see the comment of that date.


Resolution

Status: Fixed (pending merge)
Commit: 89ad67e on branch fix/issue-1706 (integration branch fix/batch-2026-10-25)
Root cause: the DeclKeyword row owned only the keyword leaf. Fixed with a text-gated TypeIdentifier owner — gated on the parent too, since var is a legal package segment and import alias — and keys_operator_by_text keying both spellings by text so Halstead counts one var. The var m() {} row in the table above is invalid Groovy and is not pinned; the parameter spelling m(var x) is covered.

Activity

  1. dekobon commented on Oct 26, 2026

    @dekobon
    OwnerAuthor

    The byte-aware hook now exists (#1696, PR #1717)

    Option 1's blocker — "Getter::get_operator_spelling(node, ancestors) is byte-less, so it cannot say this type_identifier spells var" — is gone. #1599 landed Checker::keys_operator_by_text(node, code, ancestors), which Halstead reads for the operator key, and Groovy already overrides it to read the Primitive row's text gate (spells_primitive_type on identifier / type_identifier).

    So the fix is the shape the row table already has: give the DeclKeyword row of roles/groovy.rs a TypeIdentifier(text_gated_by spells_var) owner beside the keyword leaf, with the gate reading the bytes and (as the FIXME(#1706) at roles/groovy.rs:249 notes) declining the var get_op_type_with_code rejects for its own reasons; get_keyword_role then bills it as an operator and keys_operator_by_text keys it by text, so the table in the summary reads one line. The n2 move on every var field or modified local is the expected zero-drift exception and wants an ops fixture per row of that table.

  2. dekobon commented on Oct 26, 2026

    @dekobon
    OwnerAuthor

    Fix Summary

    Root cause: the DeclKeyword row of roles/groovy.rs owned var as the keyword leaf only; the type_identifier spelling fell to the getter's operand arm.

    Changes:

    • big-code-analysis-ast/src/roles/groovy.rs: TypeIdentifier(text_gated_by spells_var) on the DeclKeyword row. The gate reads the bytes and the parent — field_declaration, formal_parameter, or the ERROR that final var q = 5 recovers into. The FIXME(fix(getter/groovy): a var the grammar reads as a type_identifier bills as an operand #1706) is gone.
    • big-code-analysis-ast/src/checker/groovy.rs: keys_operator_by_text keys var by text in both spellings, so Halstead's u_operators (kind-keyed map + text-keyed map) counts one var, not two.
    • big-code-analysis-ast/src/getter/groovy.rs: the operand arm's role-bypass marker names DeclKeyword.
    • big-code-analysis-ast/src/roles/mod.rs: DeclKeyword joins the TEXT_GATED census.
    • src/metrics/halstead.rs: groovy_type_identifier_var_is_the_keyword_1706 plus the 1704 row move and the 1599 keyed-set scan widened to every node; CHANGELOG.md fix(getter/groovy): bill var as a declaration keyword #1704 entry corrected.

    Two corrections to the issue's table:

    • var m() { } is not valid Groovy (the compiler rejects var on a method declaration; fail/Var_02x.groovy upstream), so it is deliberately not pinned (grammar-dispatch §6).
    • The review found valid Groovy where a type_identifier spelled var must stay an operand — a package segment a.var.B, an import alias import java.lang.String as var used in a cast, instanceof, type argument or array type — which a bytes-only gate (option 1 as sketched) gets wrong. Hence the parent gate. A parameter's def m(var x) (valid; type_identifier under formal_parameter) was also missing from the table and is covered.

    Languages affected: Groovy only (Java gates its var separately, #1560; Kotlin's is a keyword leaf).

    Tests: operator rows for field / static field / private field / two fields / final var / parameter / constructor parameter / mixed spellings ([7, 10], the one row pinning the single text key — kind-keyed it reads 8) / var var = 6; operand rows for the same plus qualified, cast, instanceof, type-argument, array and variant prefix controls. Nine perturbations each fail exactly the tests that claim them (~/bca-evidence/1706/perturb.txt): dropping the owner, keying by kind, text-keying every keyword, admitting every type_identifier, bytes-only gate, dropping each parent, prefix match. Patch coverage 100% (29/29; project 98.78%). No integration-snapshot movement (the corpora carry no Groovy var fields).

    Known gap: a script-level final var s = 9 followed by another statement recovers as a method_declaration head rather than an ERROR, so its var still bills nothing — documented at the gate, not pinned.

    Evidence: ~/bca-evidence/1706/ (context dumps, fixtures, perturbation driver and log, coverage-llvm.json).

    Commit: 89ad67e on branch fix/issue-1706 (integration branch fix/batch-2026-10-25).

  3. sharmis-tp commented on Oct 27, 2026

    @sharmis-tp

    Option 1 seems right to me, and the FIXME(#1706) already sits beside the DeclKeyword row in roles/groovy.rs. I'd follow the Primitive row: add TypeIdentifier(text_gated_by ...) to DeclKeyword, with a gate checking that the bytes spell var in a declaration's type position. Then I'd widen Getter::get_operator_spelling to take code, so the Groovy getter answers Some("var") and all four rows of your table key as one operator. Tests would pin those four, plus def var = 3 and [var: 1] staying operands as identifiers.

    What gives me pause is #1690: R05 rewrote every language's rows on refactor/1694-1695-paying-roles, which isn't pushed yet. Should this PR target main now, or wait and rebase onto that branch once it lands?

    If the plan suits you, please assign me; I'd have a PR up within 1-2 days of knowing the base.

  4. dekobon commented on Oct 31, 2026

    @dekobon
    OwnerAuthor

    Evidence supplement (2026-10-31)

    The fix comment above cites a local evidence directory, which is a path on one machine. The one thing in the tree that rested on it is the rule in .claude/rules/grammar-dispatch.md ("Every parent in the dump gets a row or a stated exclusion"), which cited the first investigation fixture by path.

    That fixture packed every var spelling into one 22-line Groovy class and method. Nothing in it is needed any more: each spelling it held is now a single-construct row in the #1706 tests in src/metrics/halstead.rs, including the one the rule is about, where the keyword sits inside a catch clause and the gate first shipped without that parent. The packing is itself the rule's second point: in that file the clause recovered as an error node, so the real parent appeared only once the construct was dumped alone.

    The perturbation driver and coverage capture were retained locally but their anchors are tied to the pre-merge text; the nine perturbations and their results are in the fix comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions