fix(members): three false positives in unused-class-members#1811
Open
Jerc92 wants to merge 4 commits into
Open
fix(members): three false positives in unused-class-members#1811Jerc92 wants to merge 4 commits into
Jerc92 wants to merge 4 commits into
Conversation
`class Sub extends Base {}` without an `export` never becomes an export, so the
analyze layer cannot resolve the object in `Sub.someStatic`: its import/export
map holds only imports and exports, and the heritage parent-to-children map is
built from exports alone. Every member reached only through the subclass was
reported unused. Exporting the subclass makes the identical code resolve, which
is the tell.
The fix belongs in extract, where the data already exists. Non-exported
top-level classes are recorded with their superclass, so walk that `extends`
chain to the first name that is not locally declared -- the imported or exported
base -- and re-emit the access against it. The existing import-to-export
resolution then credits the base. No synthetic export keys, which would be both
semantically wrong for a non-export and prone to collide across files.
Cycle detection, not a depth cap, terminates the walk. A cap would silently
abstain on a legitimately deep chain and leave its members falsely reported,
which is the failure this rule must never make.
The pass runs after `resolve_bound_member_accesses`, because that is what
materializes the class-qualified access for `const s = new Sub(); s.member`.
Running earlier saw only the statics spelled `Sub.member` in source and left
every instance member reached through a local subclass reported.
A mixin superclass records no name and abstains: `class Sub extends mixin(Base)`
may redefine what the subclass exposes, so crediting through it would be a
guess. That pre-existing false positive is pinned by a test rather than fixed by
accident.
Over-crediting is safe here. A subclass that shadows a base member credits the
base too, which is a false negative -- the direction this rule must err in, since
a false positive is what made it untrustworthy.
The regression fixture is cross-file. A same-file fixture passes for the wrong
reason: a class whose instance escapes through a same-file factory is skipped
wholesale by the whole-object gate, so it reports nothing either way.
Cross-module factory-return propagation already credited `const s = f(); s.m`.
It never fired for the two shapes real composables and singletons actually use:
useApi().member // chained call
const { member } = useApi() // destructure
The extractor recorded a factory-return candidate only for a plain binding
identifier, so an object pattern was dropped, and a chained call is not a
variable declarator at all, so nothing recorded it. Both fell through to "no
access", and every member of the returned class was reported unused -- 81% false
positives on the codebase that prompted this.
Both shapes name a callee and a member at capture time, so record that pair
directly. A same-file factory binds the class immediately; an imported callee
emits the typed fact the analyze layer resolves through the strict, value-proven
`exported_factory_returns` gate. Any other callee resolves to no proven factory
export and credits nothing, so recording every `identifier().member` is safe.
Routing through a stand-in local instead would have made each such call a
candidate, and candidate resolution rescans every member access -- quadratic on a
file full of helper calls.
Only the first level of the result belongs to the factory's class. `f().a.b`
credits `a`; `b` is read off whatever type `a` has. `{ a: { b } }` credits `a`
for the same reason. A member-expression callee (`obj.f().a`) and a call of a
call (`f()().a`) resolve to no proven factory export, so crediting through them
would be a guess and they stay uncredited.
`const { a, ...rest } = f()` and a computed key can read ANY property, so no set
of visible keys describes what is used. Crediting just `a` would leave every
other live member reported -- the same false positive, narrowed. The returned
class is marked wholly used instead, via a new `FactoryFnWholeObject` fact
resolved through the same gate as the member facts. Reporting nothing for that
class is a deliberate false negative: under-reporting is safe here,
over-reporting is what made the rule untrustworthy. The variant is appended to
`SemanticFact`, never inserted: bitcode encodes an enum by ordinal, so moving an
existing variant would make an old cache decode one fact as another.
Fixtures are cross-file. A same-file factory is skipped wholesale by the
whole-object gate, because the returned instance escapes the module, so a
same-file fixture reports nothing and passes for the wrong reason. That is why
the fixture added with the previous fix for this bug went green while the
reported bug stayed live.
Follow-ups from an adversarial review of the two preceding commits.
`resolve_factory_inline_accesses` scanned every import for each `f().member` it
recorded. A file that reads many helper results was O(accesses x imports).
Index the imported local names once instead.
The callee is matched by name and not by scope, so a local binding that shadows
an imported factory is treated as that factory. It can only add credit, so the
worst case is a member that stays unreported. Say so where it happens.
`class Sub extends ns.Base {}` walks to the dotted name `ns.Base`, which the
analyze layer cannot resolve: its import/export map keys bare local names. The
base's members stay reported. That is a pre-existing gap rather than a
regression, and it is wider than subclassing -- a direct `ns.Base.someStatic()`
is equally uncredited on main, verified by running both binaries against the
same fixture. The doc comment claimed the dotted form was handled; it is not.
Pin it with a test, as with the mixin superclass, so a future change to
namespace resolution has to decide the case deliberately rather than flip it by
accident.
Pure refactor from a simplification review. Behavior is unchanged: every fixture across the branch produces byte-identical output, and no extraction fact changes meaning, so `CACHE_VERSION` stays where it is. `propagate_factory_fn_accesses` and the new whole-object pass each walked the callee -> re-export origins -> `exported_factory_returns` -> class-with-members chain themselves, and `credit_factory_return_class_member` re-implemented the inner half of it a third time. Each link of that chain is an over-credit gate, so three copies are three places for the gates to drift apart. They now share `factory_return_classes_for_callee` and `factory_return_class_origins`. `credit_factory_return_class_member` keeps its own name because `typed_property.rs` calls it too. `record_factory_return_candidate` matched a bare `identifier(...)` callee by hand next to a helper doing exactly that. It now calls the helper, renamed `bare_call_callee_name` since it recognizes any bare call, not only a factory. `factory_inline_accesses` became `factory_unnamed_result_accesses`: it also holds destructured keys, so "inline" claimed less than the code does. Two comments were left describing a synthetic-local mechanism that no longer exists, and one doc claimed a `None` return from a function returning a `Vec`. `extract_destructured_names` and `destructured_factory_keys` look like the same function and are not: the former drops a computed key and keeps the rest, the latter makes a single unnameable key opaque the whole pattern, because a caller crediting class members off it must abstain rather than credit what it can see. Collapsing them would have silently dropped destructured names from taint-source tracking and two other callers. Both now say so.
6106b9f to
6153949
Compare
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
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.
What
Three confirmed false positives in
unused-class-members, all reproduced against a freshly builtmain(3.3.0) before any code was written, each pinned by a cross-file regression fixture with a positive control.1. A member reached through a local (non-exported) subclass.
Base.someStaticwas reported unused.Subnever becomes an export, so the analyze layer's import/export map cannot resolve the objectSub, andbuild_parent_to_childrenis built from exports alone. ExportingSubmakes the identical code resolve — that is the tell. Fixed in extract, by walking the localextendschain to the first name that is not locally declared and re-emitting the access against it. No synthetic export keys: anExportKeyfor a non-export is semantically wrong and collides across files.2 and 3. A factory result read without a named binding.
Cross-module factory-return propagation already worked for
const s = f(); s.m. It never fired for these two, because the extractor recorded a candidate only for a plainBindingIdentifierdeclarator, and a chained call is not a declarator at all. Those two shapes are what real composables and singletons use, so every member of the returned class was reported unused.Both now record
(callee, member)directly. A same-file factory binds the class; an imported callee emits the typed fact the analyze layer resolves through the strict, value-provenexported_factory_returnsgate. Any other callee resolves to no proven factory export and credits nothing, so recording everyidentifier().memberis safe.Only the first level of the result belongs to the factory's class.
f().a.bcreditsa;bis read off whatever typeahas.{ a: { b } }creditsafor the same reason.The opaque-destructure trap.
const { a, ...rest } = f()and a computed key can read any property. Crediting only the visible keys would leave every other live member reported — the same false positive, narrowed. The returned class is marked wholly used instead, via a newFactoryFnWholeObjectfact resolved through the same gate. Reporting nothing for that class is a deliberate false negative.The final commit is a pure refactor: the
callee -> re-export origins -> exported_factory_returns -> class-with-memberschain existed in three copies, and every link of it is an over-credit gate, so three copies are three places for the gates to drift apart.Why
Every fix here is additive credit: it can turn a reported member into an unreported one, never the reverse. Neither can introduce a new false positive. That is the direction this rule must err in — a false positive is what makes it untrustworthy, and it was disabled downstream because 81% of its findings were false positives.
Corrections to the original report
Two of the three diagnoses in the bug report were wrong, and following them would have produced the wrong fix:
getState(): Statereturn type looked ignored. It never was.static arrowViaSub = (v) => vis flagged identically. The discriminator is that the subclass is not exported.Deliberately not fixed
Each is pinned by a test, so a future change has to decide the case rather than flip it by accident:
class Sub extends mixin(Base)— no superclass name is recorded; a mixin may redefine what the subclass exposes, so crediting through it would be a guess.class Sub extends ns.Base— the dotted name resolves to no bare local. Pre-existing, and wider than subclassing: a directns.Base.someStatic()is equally uncredited onmain, verified by running both binaries against the same fixture. Closing it means resolving namespace aliases in the analyze layer.computed(() => new C()).value.m, or a plain{ value: new C() }wrapper). Annotated hops already work via the typed-property path; inferred ones and generic type arguments (Ref<T>/ComputedRef<T>) do not. Not Vue-specific; Vue just makes it ubiquitous.Happy to take any of these on in a follow-up.
Test plan
deepandinner(second-level keys) never become members of the factory's class.cargo test --workspace --lib --bins --tests— 58 test binaries, 0 failures.cargo clippy --workspace --all-targets -- -D warningsandcargo fmt --all -- --checkclean.generate:contracts:checkand a--features schema-emitbuild both pass.CACHE_VERSION(227 -> 228 -> 229); stale cached modules would otherwise carry pre-fix facts. The newSemanticFactvariant is appended, never inserted: bitcode encodes an enum by ordinal, so moving an existing variant would make an old cache decode one fact as another.The change was reviewed by four independent adversarial passes — on the plan, on the first commit, on the branch, and for redundancy. Findings acted on include: replacing a depth cap that was itself a false-positive source; moving the subclass pass after
resolve_bound_member_accesses, which is what materializes the access forconst s = new Sub(); s.member; appending rather than inserting the enum variant; and removing a stand-in-local mechanism that made everyhelper().xa resolution candidate.Note: the commits are unsigned — no GPG key on the machine they were authored on. Happy to re-sign if that blocks merge.