Skip to content

Credit a cross-module field read through a bound name - #95

Open
mpecan wants to merge 1 commit into
juxt:mainfrom
mpecan:fix/field-unused-cross-module-binding
Open

Credit a cross-module field read through a bound name#95
mpecan wants to merge 1 commit into
juxt:mainfrom
mpecan:fix/field-unused-cross-module-binding

Conversation

@mpecan

@mpecan mpecan commented Aug 25, 2026

Copy link
Copy Markdown

Credit a cross-module field read through a bound name

Problem

allium.field.unused fires on a field whose only reader reads it across a use
edge through a bound name rather than through the qualified spelling. A surface
writes context shelf: alias/Shelf and then reads the relationship as
shelf.copies; alias/Shelf.copies never appears anywhere, so
collect_reverse_contributions credits nothing and the imported module reports a
field that its only reader iterates.

Because check exits 1 on warnings, this fails any gate keyed on the exit status,
on valid input. Found downstream in a repo that had to raise a ratcheting debt
baseline to accommodate two of these.

Root cause

The reverse channel credits one spelling of a field read.
collect_qualified_field_refs matches alias/Entity.field at the reference site.
That is the spelling an importer uses when it names the entity — and not the one
it uses once something has already bound it.

Two ordinary constructs bind it. A surface's context/facing clause, and a
rule's trigger parameter typed by the surface that provides that trigger. Both are
the normal way an importer reads an imported entity, and neither leaves a qualified
reference behind for the existing pass to see.

This is the field.unused analogue of #65: the collector was built for the
qualified form and never re-enumerated the ways a name can come to denote an
imported entity.

Fix

Two additions, both reusing machinery that is already here.

The read. collect_bound_field_refs credits bound.field for names typed to
an imported entity, walking with the existing walk_expr_children rather than
duplicating the expression match. Its _from_item sibling joins the seven that
already exist in this file.

The bindings. These come from qualified_context_binding and
collect_importer_command_param_types unchanged — passed a map of every
imported entity in place of status_by_entity. Both helpers read that map for
membership only; it carries just the status-bearing entities because its other
consumers are lifecycle checks, and a field read is worth crediting whatever the
entity's lifecycle. rule_trigger_bindings then resolves a rule's when:
positionally against that table — the same resolution collect_witnessed_transition
already performs for its own clause.

No existing collector is modified. collect_qualified_field_refs is byte-identical
to main.

Why this cannot over-credit

Crediting stays gated on a real use import edge and on the binding's type
resolving to a declared imported entity — never arbitrary co-supply.

Beyond that gate, the new path only ever fires on an Expr::MemberAccess /
OptionalAccess, so every name it yields is one the module-local collector would
yield on the same expression
, then intersected with the imported entity's declared
field names. It cannot credit anything the equivalent merged single file does not
already credit. bound_read_matches_the_merged_single_file_control asserts exactly
that, and is the test to read first.

The module-wide (rather than per-entity) imported_field_names set is unchanged
from the existing qualified path, deliberately: making the new path stricter would
break the merged-file oracle in the other direction.

Not covered, and deliberately

Both need field-type resolution across the boundary, which is a larger change than
this one, and both reproduce on main with the qualified spelling too — neither
is introduced here.

  • A read through a loop binding derived from a bound name:
    for c in shelf.copies: c.title credits copies, not title.
  • A chained read, message.group.members, whose object is itself a member
    access rather than a name.

In the downstream repo this means three false reports become one. Happy to file
these as an issue if you'd like them tracked.

Tests

Six parser unit tests and six integration tests in cross_module_lifecycle.rs,
placed with their siblings. The integration set asserts the three oracles this file
already holds everything else to:

  • the two-file pair reports exactly what the equivalent one-file spec reports;
  • the imported module analysed alone keeps its local warning;
  • without a use edge, a co-supplied file's bound read credits nothing.

Plus a precision guard: crediting one field of a bound entity leaves that entity's
unread fields still reported.

Each of the four new decisions — the facing keyword, the rule channel, the
entity-existence guard, and the type refinement — was mutated individually and is
killed by exactly one test.

Verification

  • cargo test --workspace: 658 passed, 0 failed.
  • cargo clippy --workspace --all-targets: warning multiset identical to main
    (diffed, not sampled).
  • allium check docs/project/specs/: coded diagnostics identical to main,
    per code. The behaviour spec gains two rules in the reverse-aggregation section.
  • Formatting follows the surrounding file. cargo fmt --check does not pass on this
    file on main either, and CI runs neither fmt nor clippy, so I have matched
    the neighbouring style rather than reformatted regions I did not otherwise touch.

Version bump left to land separately after merge, per the usual flow here.

`allium.field.unused` reported a field whose only reader reads it across a
`use` edge through a bound name rather than through the qualified spelling.
A surface writes `context shelf: alias/Shelf` and then reads the relationship
as `shelf.copies`; `alias/Shelf.copies` never appears, so
`collect_reverse_contributions` credited nothing and the imported module
reported a field its only reader iterates.

The root cause is that the reverse channel credited one spelling of a field
read. `collect_qualified_field_refs` matches `alias/Entity.field` at the
reference site, which is the spelling an importer uses when it names the
entity — and not the one it uses when something has already bound it. A
surface's `context`/`facing` binding and a rule's trigger parameter are both
such bindings, and both are the ordinary way an importer reads an imported
entity.

Two additions, both reusing what the file already has:

- `collect_bound_field_refs` walks with `walk_expr_children` and credits
  `bound.field` for names typed to an imported entity, with a
  `_from_item` sibling alongside the seven that already exist.
- The binding sets come from `qualified_context_binding` and
  `collect_importer_command_param_types` unchanged, passed a map of every
  imported entity instead of `status_by_entity`. Both read that map for
  membership only; it carries the status-bearing entities because its other
  consumers are lifecycle checks, and a field read is worth crediting
  whatever the entity's lifecycle. `rule_trigger_bindings` then resolves a
  rule's `when:` positionally against that table, the same resolution
  `collect_witnessed_transition` already does for its own clause.

Crediting stays gated on a real `use` edge and on the binding's type
resolving to a declared imported entity; it is not widened to arbitrary
co-supply. The new path only ever fires on a member access, so every name it
yields is one the module-local collector would yield on the same expression,
intersected with the imported entity's declared fields — it cannot credit
anything the equivalent merged single file does not.

Not covered, and deliberately: a read through a loop binding derived from a
bound name (`for c in shelf.copies: c.title` credits `copies` but not
`title`), and a chained read (`message.group.members`), whose object is
itself a member access rather than a name. Both need field-type resolution
across the boundary, which is a larger change than this one. Both reproduce
on `main` with the qualified spelling too, so neither is introduced here.

Six parser unit tests and six integration tests in
`cross_module_lifecycle.rs`, including the merged-single-file control, the
imported module analysed alone, the no-`use`-edge gate, and a precision guard
that an unread field of a bound entity is still reported. Each of the four
new decisions — the `facing` keyword, the rule channel, the entity-existence
guard and the type refinement — was mutated and is killed by exactly one
test. Workspace suite 658 passed, clippy warning set unchanged from `main`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant