Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .claude/rules/detection.md

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- **`unused-class-members` no longer false-flags a method dispatched through an interface-typed property (ports-and-adapters / hexagonal DI).** A method reached through a property whose declared type is an interface, on a class that `implements` that interface (`useIt(deps: Deps) { deps.greeter.greet() }` where `Deps.greeter: GreeterPort` and `class GreeterAdapter implements GreeterPort`), now credits `GreeterAdapter.greet`. The interface dispatch already worked through a direct parameter or variable (`useIt(g: GreeterPort) { g.greet() }`); this closes the remaining gap where the receiver is reached via an interface property hop, which dominates hexagonal-architecture findings. A genuinely-unused method on the implementing class still reports. Thanks [@lukeramsden](https://github.com/lukeramsden) for the clean minimal reduction. (Closes [#1863](https://github.com/fallow-rs/fallow/issues/1863))
- **`unused-class-members` no longer false-flags a method reached through a factory that returns an object literal.** A factory function returning an inferred object literal whose property values are class instances (`export function createUi() { const factory = new InvokerFactory(); return { orders: factory.ordersPage } }`), consumed cross-module as `const ui = createUi(); ui.orders.placeOrder()`, now credits `OrdersPage.placeOrder`. Every property-value shape resolves: a direct `new Class()`, a local `const` alias to one, and a member read of a separately-constructed instance (`factory.ordersPage`, whether a typed field or a getter). Nested object literals (`ui.invoke.dashboard.method()`), the assigned-then-returned form (`const ui = {...}; return ui`), and same-file consumption are all covered. A genuinely-unused method on the returned class still reports. This is the general root cause behind the Playwright page-object-factory pattern; it is not Playwright-specific. Thanks [@committedpazz](https://github.com/committedpazz) for the precise bisection. (Closes [#1858](https://github.com/fallow-rs/fallow/issues/1858))

- **Star re-exports no longer make a source module's default export appear
Expand Down
3 changes: 3 additions & 0 deletions crates/core/src/analyze/members/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1157,6 +1157,7 @@ fn collect_propagated_member_accesses(
propagate_common_member_accesses(
input,
indexes,
&heritage_context.interface_to_implementers,
&mut accessed_members,
&mut whole_object_used_exports,
);
Expand Down Expand Up @@ -1191,6 +1192,7 @@ fn collect_propagated_member_accesses(
fn propagate_common_member_accesses(
input: UnusedMemberScanInput<'_>,
indexes: &MemberPassIndexes<'_>,
interface_to_implementers: &FxHashMap<ExportKey, Vec<ExportKey>>,
accessed_members: &mut FxHashMap<ExportKey, FxHashSet<String>>,
whole_object_used_exports: &mut FxHashSet<ExportKey>,
) {
Expand Down Expand Up @@ -1222,6 +1224,7 @@ fn propagate_common_member_accesses(
input.graph,
input.resolved_modules,
indexes,
interface_to_implementers,
accessed_members,
);
propagate_fluent_chain_accesses(
Expand Down
55 changes: 53 additions & 2 deletions crates/core/src/analyze/members/typed_property.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,17 @@ pub(super) fn typed_property_declaring_sites<'a>(
/// 3. the terminal property type resolves through the last declaring
/// module's own imports/exports and must be a class with members
/// (`export_is_class_with_members`, reused via
/// `credit_factory_return_class_member`).
/// `credit_factory_return_class_member`), OR (issue #1863) an INTERFACE
/// with implementers, in which case the member is credited on the
/// interface's export key so the later `propagate_interface_member_accesses`
/// pass carries it to every implementing class.
///
/// See issue #1785.
/// See issues #1785 and #1863.
pub(super) fn propagate_typed_property_accesses(
graph: &ModuleGraph,
resolved_modules: &[ResolvedModule],
indexes: &MemberPassIndexes<'_>,
interface_to_implementers: &FxHashMap<ExportKey, Vec<ExportKey>>,
accessed_members: &mut FxHashMap<ExportKey, FxHashSet<String>>,
) {
// Phase 1: walk every fact's property path to its terminal
Expand Down Expand Up @@ -143,5 +147,52 @@ pub(super) fn propagate_typed_property_accesses(
terminal_name.as_str(),
member.as_str(),
);
credit_typed_property_interface_terminal(
&mut credit_context,
interface_to_implementers,
declaring_file_id,
terminal_name.as_str(),
member.as_str(),
);
}
}

/// Credit a typed-property-hop TERMINAL whose type is an INTERFACE (issue #1863).
///
/// `credit_factory_return_class_member` above resolves only class-with-members
/// terminals; an interface terminal (`deps.greeter: GreeterPort` where
/// `GreeterPort` is an interface) is filtered out by `export_is_class_with_members`
/// and would credit nothing. Resolving the interface to its canonical export key
/// (the same `local_keys` + `export_key_with_origins` resolution the
/// `interface_to_implementers` map is keyed by) and inserting the member there
/// makes the property-hop terminal behave exactly like the direct-parameter case:
/// the interface access is carried to every implementing class by the later
/// `propagate_interface_member_accesses` pass, which runs after this one (both
/// sit inside `collect_propagated_member_accesses`, the typed-property pass in
/// `propagate_common_member_accesses` first, then the interface pass). Purely
/// additive; a terminal that is not an interface with implementers credits
/// nothing, so it can only suppress a false positive, never create one.
fn credit_typed_property_interface_terminal(
context: &mut FactoryReturnCreditContext<'_, '_>,
interface_to_implementers: &FxHashMap<ExportKey, Vec<ExportKey>>,
declaring_file_id: FileId,
terminal_name: &str,
member: &str,
) {
let graph = context.graph;
let indexes = context.indexes;
let Some(seed_keys) = indexes.local_keys(declaring_file_id).get(terminal_name) else {
return;
};
for seed_key in seed_keys {
for interface_origin in export_key_with_origins(graph, seed_key) {
if interface_to_implementers.contains_key(&interface_origin) {
context
.accessed_members
.entry(interface_origin)
.or_default()
.insert(member.to_string());
}
}
}
}
2 changes: 2 additions & 0 deletions crates/core/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,8 @@ mod issue_1821_per_class_this_scoping;
mod issue_1821_private_field_di;
#[path = "integration_test/issue_1858_object_literal_factory_return.rs"]
mod issue_1858_object_literal_factory_return;
#[path = "integration_test/issue_1863_interface_property_dispatch.rs"]
mod issue_1863_interface_property_dispatch;
#[path = "integration_test/issue_346_static_factory_method.rs"]
mod issue_346_static_factory_method;
#[path = "integration_test/issue_604_vite_rollup_path_helpers.rs"]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
use super::common::{create_config, fixture_path};

#[test]
fn interface_typed_property_dispatch_credits_implementer_member() {
// Issue #1863: a member reached through a PROPERTY whose declared type is an
// interface (`deps.greeter.greet()` where `deps: Deps`, `Deps.greeter:
// GreeterPort`) must credit the member on every class that `implements`
// GreeterPort. The #1785 typed-property hop resolves the terminal to the
// interface, but the terminal credit only handled classes; routing an
// interface terminal through the existing interface->implementer propagation
// carries the member to `GreeterAdapter`, exactly as the direct-parameter
// case already works. A genuinely-dead method on the same class stays
// flagged (non-vacuous control).
let root = fixture_path("issue-1863-interface-property-dispatch");
let mut config = create_config(root);
config.rules.unused_class_members = fallow_config::Severity::Error;
let results = fallow_core::analyze(&config).expect("analysis should succeed");

let unused: Vec<String> = results
.unused_class_members
.iter()
.map(|m| format!("{}.{}", m.member.parent_name, m.member.member_name))
.collect();

assert!(
!unused.contains(&"GreeterAdapter.greet".to_string()),
"GreeterAdapter.greet is reached via deps.greeter.greet() through the GreeterPort \
interface property and must be credited (issue #1863), found: {unused:?}"
);
assert!(
unused.contains(&"GreeterAdapter.deadOnAdapter".to_string()),
"GreeterAdapter.deadOnAdapter has no call site and must stay flagged, found: {unused:?}"
);
}

#[test]
fn interface_property_hop_credits_every_implementer_cross_file() {
// Issue #1863 (fan-out + cross-file): the port interface (`GreeterPort`), the
// interface holding the typed property (`Deps` in a separate file), and each
// implementing adapter all live in different files, the realistic hexagonal
// layout. `deps.greeter.greet()` must credit `greet` on EVERY class that
// implements `GreeterPort` (the documented interface->implementer over-credit
// direction), while a differently-named dead method on each adapter stays
// flagged.
let root = fixture_path("issue-1863-multi-implementer");
let mut config = create_config(root);
config.rules.unused_class_members = fallow_config::Severity::Error;
let results = fallow_core::analyze(&config).expect("analysis should succeed");

let unused: Vec<String> = results
.unused_class_members
.iter()
.map(|m| format!("{}.{}", m.member.parent_name, m.member.member_name))
.collect();

assert!(
!unused.contains(&"AdapterA.greet".to_string())
&& !unused.contains(&"AdapterB.greet".to_string()),
"greet must be credited on both implementers of GreeterPort via the interface \
property hop (issue #1863), found: {unused:?}"
);
assert!(
unused.contains(&"AdapterA.deadOnA".to_string())
&& unused.contains(&"AdapterB.deadOnB".to_string()),
"the uncalled methods on each adapter must stay flagged (non-vacuous control), \
found: {unused:?}"
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "issue-1863-interface-property-dispatch",
"private": true,
"main": "src/index.ts"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import type { GreeterPort } from './port';

export class GreeterAdapter implements GreeterPort {
greet(name: string): string {
return `hi ${name}`;
}
deadOnAdapter(): void {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import type { Deps } from './port';

export function useIt(deps: Deps): string {
return deps.greeter.greet('x');
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { GreeterAdapter } from './adapter';
import { useIt } from './consumer';

export function run(): string {
return useIt({ greeter: new GreeterAdapter() });
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export interface GreeterPort {
greet(name: string): string;
}
export interface Deps {
greeter: GreeterPort;
}
5 changes: 5 additions & 0 deletions tests/fixtures/issue-1863-multi-implementer/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "issue-1863-multi-implementer",
"private": true,
"main": "src/index.ts"
}
8 changes: 8 additions & 0 deletions tests/fixtures/issue-1863-multi-implementer/src/adapter-a.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import type { GreeterPort } from './port';

export class AdapterA implements GreeterPort {
greet(name: string): string {
return `A ${name}`;
}
deadOnA(): void {}
}
8 changes: 8 additions & 0 deletions tests/fixtures/issue-1863-multi-implementer/src/adapter-b.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import type { GreeterPort } from './port';

export class AdapterB implements GreeterPort {
greet(name: string): string {
return `B ${name}`;
}
deadOnB(): void {}
}
5 changes: 5 additions & 0 deletions tests/fixtures/issue-1863-multi-implementer/src/consumer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import type { Deps } from './deps';

export function useIt(deps: Deps): string {
return deps.greeter.greet('x');
}
5 changes: 5 additions & 0 deletions tests/fixtures/issue-1863-multi-implementer/src/deps.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import type { GreeterPort } from './port';

export interface Deps {
greeter: GreeterPort;
}
7 changes: 7 additions & 0 deletions tests/fixtures/issue-1863-multi-implementer/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { AdapterA } from './adapter-a';
import { AdapterB } from './adapter-b';
import { useIt } from './consumer';

export function run(): string {
return useIt({ greeter: new AdapterA() }) + useIt({ greeter: new AdapterB() });
}
3 changes: 3 additions & 0 deletions tests/fixtures/issue-1863-multi-implementer/src/port.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export interface GreeterPort {
greet(name: string): string;
}
Loading