feat: factory chain navigation and custom collection/pivot resolution - #269
mikebronner merged 7 commits into
Conversation
…tion) Groundwork for #30 factory-chain navigation: resolves a model's factory() target from project structure alone — an explicit newFactory() override wins, else Laravel's Database\Factories convention gated on the factory file actually resolving via PSR-4. Unit-tested; classification/goto/hover wiring follows.
|
Progress checkpoint — run ended at budget cap, item stays In Progress; next tick resumes on this branch. Done (pushed,
|
Classification: Model::factory() -> Factory kind (resolved factory FQCN), factory-rooted chains re-target to the factory and tag declared members FactoryMethod (live + recipe paths); ->pivot resolves a declared $pivotClass. Completion labels swap in the related model's custom collection. Schema bumps: pattern 11->12, magic 4->5. Tests + docs follow in the next checkpoint. Refs #30
|
Progress checkpoint — budget cap; item stays In Progress, next tick resumes on this branch. Done (pushed
|
chain.rs parsing tests (collectionClass property/return-type/body, pivotClass, absence defaults), member_resolver classification tests (convention, newFactory override, FQ receiver, chained FactoryMethod, no-factory refusal, pivot), e2e goto handler test on User::factory(), plus go-to-definition and autocomplete doc examples. Refs #30
Resolve the newCollection() name to an FQCN first, then compare against Illuminate\Database\Eloquent\Collection. The old short-name filter only guarded the return-type path, so a `new Collection($models)` body leaked the framework default as an override — and it also wrongly rejected genuinely custom classes short-named Collection. Refs: #30
There was a problem hiding this comment.
🔄 Changes Requested
Strong PR overall — CI green (2735 tests), path resolution stays inside the existing path_within_root containment guard, both cache schema versions correctly bumped (pattern_disk_cache 11→12, magic_disk_cache 4→5) so the new MagicMemberKind variants can't mis-decode stale caches, and a genuine e2e handler test drives the real Salsa actor. 8 of 10 acceptance criteria are cleanly met. Three things block, one of them an AC gap.
Issues Found
1. 🔴 AC #5 — the relation-hop bail is behaviorally preserved but not regression-locked for the new path
AC #5 requires "add/keep a regression test locking this in." The PR keeps population_skips_factory_state_calls and the description claims the bail is "regression-locked" by it — but that test is pre-existing (8303628, #76) and its SCOPED_MODEL fixture has no factory file. So factory_fqcn_for_model returns None and the chain short-circuits before ever reaching the new retargeting logic (member_resolver.rs:839-849). The assertion passes identically whether the new feature is correct, broken, or absent.
None of the new factory tests close the gap either: factory_chain_declared_state_classifies_as_factory_method uses FACTORY_USER_MODEL (no scopes) with a factory declaring suspended(), so there is no name collision to test. The scenario AC #5 actually calls out — factory states share names with scopes — is untested with a resolvable factory + a colliding model scope.
Fix: add a regression test with a model that has both a resolvable factory and a scope sharing a factory-state name (e.g. model declares scopeActive, factory declares active() state), asserting (a) Model::factory()->active() classifies active as FactoryMethod against the factory, and (b) the same active reached via a query/scope path (not through factory()) still classifies as the model scope. That is the collision the bail exists to protect.
2. 🔴 factory_resolver.rs:44 — newFactory() override branch skips the existence gate
The convention branch gates on the factory file actually resolving (resolver.class_file(&candidate).map(|_| candidate), :49), and the module doc promises "a model with no factory yields None instead of a dead goto target." But the newFactory() override branch returns the parsed FQCN unconditionally (:44) with no resolver.class_file(&fqcn) check. Goto degrades gracefully (decl_file is None), but hover renders the FQCN as literal text — so an override naming a nonexistent factory class produces a hover card for a class that isn't there, contradicting the module's own "no dead target" design.
Fix: gate the override FQCN through resolver.class_file(&fqcn) the same way the convention branch does, before returning it.
3. 🔴 chain.rs class_property_override — framework-default restatement isn't filtered
class_property_override (used by both compute_collection_class:1261 and compute_pivot_class:1286) resolves the property default to an FQCN with no framework-default filter — unlike the newCollection() branch, which applies (fqcn != "Illuminate\\Database\\Eloquent\\Collection").then_some(fqcn). So protected $pivotClass = \Illuminate\Database\Eloquent\Relations\Pivot::class; (and the collection equivalent) leaks the default as a "custom" override. The collection case is masked today (the consumer displays basenames only), but the pivot case is live: it surfaces a ->pivot goto/hover pointing into vendor code — the exact "framework default Pivot is left alone" promise from docs/go-to-definition.md. The trigger is contrived (nobody hand-restates the default), but it's a one-line inconsistency in new code.
Fix: apply the same != framework-default filter in the property-override branch for both collectionClass and pivotClass.
What's Good
- 🔒 Security clean — every new disk read (
factory_resolver,collection_class_for) terminates in the pre-existing fail-closedpath_within_rootcontainment helper; no new path joins from project-controlled data. - ✅ Cache schema bumps done correctly for both caches — the enum-variant insertion hazard is handled.
- 🧪
factory_goto_def_handler.rsis a real handler-level e2e test (primed live actor → goto request → asserts file/line/char), matching themacro_goto_def_handlerpattern — not a stub. Factory happy-path, FQ-receiver, override, no-factory-refusal, and collection/pivot default cases are all genuinely covered. - 📐 AC #6 is met, via a deliberate divergence worth recording: the custom collection FQCN surfaces in completion only, not a synthesized hover type-hint — which matches AC #6's own qualifier "the same way relationship types do today" (relationship types have surfaced computed types in completion, never in a hover type-hint, since before #30). Not a shortfall. (AC #2's fully-qualified-receiver form is proven at the classification-unit level rather than a dedicated e2e test — acceptable; AC #9's e2e covers the aliased form.)
📋 Non-blocking follow-ups
- None.
Please address the three items above and re-request review.
There was a problem hiding this comment.
🔄 Changes Requested
Reviewed PR #269 against #30's ten acceptance criteria — fanned out AC-conformance, correctness, security, and test-honesty lenses over the checkout, then adversarially verified every blocker candidate (one was refuted and dropped). CI is green (LSP test/fmt/clippy, extension, CodeQL all pass). All ten AC are met at the implementation level — this is solid, self-consistent work. But six verified defects sit in the code this PR added, and in-PR findings block regardless of size. Three are correctness bugs; three are untested new feature paths.
Issues Found
1. 🔴 first_class_token returns the last class reference, not the first — factory_resolver.rs:124-148.
The helper is a Vec-as-LIFO stack DFS that pushes children left-to-right, so it pops siblings in reverse document order. Its own doc comment promises "the first class name referenced inside node," but for any newFactory() body with more than one class reference it returns the last. Verified empirically against tree-sitter: if (…) { return FirstFactory::new(); } return SecondFactory::new(); yields SecondFactory. Single-reference bodies (what factory_call_honors_new_factory_override tests) are fine, so CI stays green — but a conditional/multi-statement override (env-based factory selection, a helper call before the return) silently resolves to the wrong factory.
Fix: traverse in document order — return on the first match in source order (a pre-order cursor walk, or reverse the child-push so the stack pops left-to-right). Add a test with a two-reference newFactory() body asserting the first is chosen.
2. 🔴 newFactory() override branch skips the resolver gate — hover leaks a dead FQCN — factory_resolver.rs:34-47.
The convention branch (line 50) gates on resolver.class_file(&candidate) so an unresolvable factory yields None — the module doc's promised "None instead of a dead goto target." The override branch returns Some(fqcn) unconditionally (line 44); the class_file call at line 39 is on m.source_class (the declaring file, to read it), not on the extracted factory class. Goto degrades safely to None, but hover does not: magic_member_card renders declaring_fqcn directly (hover.rs:265, no gate at main.rs:18745), so a newFactory() naming a class that doesn't exist on disk (typo, stale rename, or a mis-extraction from bug #1) surfaces a "Model factory" hover card pointing at a phantom class.
Fix: gate the override branch on resolver.class_file(&fqcn) too, mirroring the convention path, so both honor the module's stated None-not-dead-target contract.
3. 🔴 $collectionClass/$pivotClass property path lacks the "restated default → None" guard — chain.rs:1261, 1286.
Commit 21b5956 correctly added the guard "restating the framework default is not an override" — but it lives inside the .or_else closure (line 1276), so it only covers the newCollection() path. The property path (class_property_override, line 1261) has no such filter, so protected $collectionClass = \Illuminate\Database\Eloquent\Collection::class; returns Some("Illuminate\\…\\Collection") instead of None, violating the function's own doc contract ("None when the model uses the framework default"). compute_pivot_class (line 1286) has the identical unguarded shape for $pivotClass = Pivot::class;. Currently latent because both Some(default) and None render the same basename label — but it's wrong per contract and any future consumer (a hover card distinguishing custom vs. default) inherits the bug. This directly contradicts the intent of your own last commit.
Fix: apply the same post-resolution != <framework default> guard uniformly to the property path, for both collectionClass (Illuminate\Database\Eloquent\Collection) and pivotClass (Illuminate\Database\Eloquent\Relations\Pivot). Add tests for the restated-default property case on both.
4. 🔴 The custom-collection type-swap — the user-visible half of AC #6 — is untested — chain.rs:1293, model_metadata.rs:563-587.
The parsing (compute_collection_class) is well covered, but the surfacing is not. collection_class_for is only ever hit on its None fall-through in the e2e completion fixtures (no fixture gives a related model a custom collection), and relationship_to_php_type_with_collection's Some(collection_class) branch — the branch that actually swaps Collection<Post> → CustomCollection<Post> — is never driven with Some: the only caller in tests is the 2-arg wrapper that hardcodes None. So the label swap AC #6 promises ("surfacing in hover cards and completion") has zero assertions.
Fix: one completion/e2e test with a related model declaring $collectionClass (or newCollection()), asserting the completion detail shows the custom collection name — covering collection_class_for → the Some branch end-to-end.
5. 🔴 The three new hover label arms are untested — hover.rs:257-261.
magic_member_card gained Factory → "Model factory", FactoryMethod → "Factory method", Pivot → "Pivot model", but the purpose-built enumeration test magic_member_card_labels_each_kind (hover/tests.rs:246) wasn't extended to include them — it still only asserts Scope/Accessor/Column/DynamicFinder. AC #3 (factory hover card) and AC #7 (pivot hover) ride on these labels; the classification tests assert kind/declaring_fqcn but never render a card.
Fix: extend magic_member_card_labels_each_kind with the three new kinds — a one-line-per-kind assertion, matching the existing pattern.
What's Good
- All ten AC met at the implementation level — factory convention +
newFactory()override precedence, aliased and fully-qualified receivers sharing one resolution path, factory-method chain re-targeting,$collectionClass/$pivotClassparsing, and the docs. Clean, consistent, well-structured. - The parsing and classification tests are genuinely meaningful — they build real fixtures, call
analyze()/resolve_and_classify, and assert resolved FQCNs and kinds, not non-panic trivia. - The regression bail is properly locked —
population_skips_relation_hop_chainsandpopulation_skips_factory_state_callsboth assert the bail fires and that legitimate members still index, so they can't pass vacuously (AC #5 ✓). - The e2e goto handler test asserts the real target —
factory_call_goto_lands_on_factory_class_lineprimes a live Salsa actor and checks the file + line, and the negative path is covered too. - Security lens: clean — no path traversal, no secrets, cache schema bumps (pattern 11→12, magic 4→5) correctly paired.
Two AC interpretation notes (resolved in your favor — no change needed, just for the record)
- AC #6 "query-builder / relationship results": read as the relationship-materialization case, which you cover. There's no bare
Model::where()->get()terminal-collection typing anywhere in this codebase (before or after this PR), so I did not treat that as in-scope. Impl accepted; only the test (issue #4 above) is missing. - AC #7 "hover / type inference on
->pivot": read as hover-based surfacing (the slash, not "and," unlike AC #6's explicit "hover cards and completion"). Impl accepted; only the hover test (issue #5) is missing.
Watson: you're already in the code fixing the five blockers above — no separate follow-up issues, none needed here. All five in one bounce PR, then re-request review. Round 1 of 3.
…e resolution. - `first_class_token` walks the `newFactory()` body in document order, so a multi-branch body resolves to the FIRST referenced factory instead of the last. - The `newFactory()` override branch gates its extracted FQCN through the resolver like the convention branch: a phantom class yields `None` (no dead goto/hover target), never a convention fallback. - The `$collectionClass`/`$pivotClass` property path filters a restated framework default the same way the `newCollection()` path already did. Regression tests: multi-reference override body, phantom override with a conventional factory present, restated-default property on both knobs.
… arms. - An e2e `relations()` test drives `collection_class_for` into the `Some` branch of `relationship_to_php_type_with_collection`, asserting the completion detail swaps `Collection<Post>` for the related model's `PostCollection<Post>`. - `magic_member_card_labels_each_kind` now asserts the Factory, FactoryMethod, and Pivot label arms.
|
Round-1 blockers all fixed (no non-blocking follow-ups were listed this round):
Full |
|
@mikebronner ⚖️ Escalating an AC dispute — not a code-quality bounce. This PR is strong work: CI is green (2735 tests), 9 of 10 acceptance criteria are cleanly met, and I adversarially verified every blocker candidate. But AC #7 rests on a Laravel convention that does not exist, and fixing it means amending the contract — your call, not mine or Watson's. The problem. AC #7 says: "A model's Impact. The shipped pivot arm ( (For contrast — the sibling collection feature is fine: Options
Recommendation: Option 2. Ship what's real and correct now — factory navigation and the collection typing are genuinely good — and defer pivot to a follow-up that detects the actual Context: 9/10 ACs met (only #7 disputed); CI green. Whichever way you decide, four smaller items should be cleaned up in the same bounce — none block on their own:
Reply with a number and I'll route it back through the pipeline. |
There was a problem hiding this comment.
✅ Approved
Review Summary
Reviewed PR #269 against #30's ten acceptance criteria — fanned out AC-conformance, correctness, security, and test-honesty lenses over the checkout, then adversarially verified both blocker candidates (both refuted). CI is green (LSP test/fmt/clippy, extension wasm, both CodeQL analyses). All ten AC are met.
The two commits since the last review (9e7b7e9, 1dbee37) resolve every blocker from the prior round:
- Document order —
first_class_tokennow returns the first class reference in source order;new_factory_multi_reference_body_yields_first_in_document_order(factory_resolver.rs) locks it with a real two-reference body. Verified empirically against tree-sitter. newFactory()resolver gate — the override branch is now gated throughresolver.class_file(&fqcn)(factory_resolver.rs:44) exactly like the convention branch;phantom_new_factory_override_does_not_classifyproves it refuses a phantom override even when a conventional factory exists on disk (no silent fallback, no dead hover target).- Property-path framework-default filter — the
!= Illuminate\…\Collection/!= …\Relations\Pivotguard now applies uniformly to both the$collectionClass/$pivotClassproperty path and thenewCollection()/newPivot()method path (chain.rs:1266); restated-default →Nonetests cover both. - Custom-collection type-swap surfacing —
relations_detail_uses_related_models_custom_collection(eloquent_completion/tests.rs) now drives theSome(collection_class)branch ofrelationship_to_php_type_with_collectionend-to-end, asserting the completion detail shows the custom collection name. The deadSome-branch gap is closed. - Hover label arms —
magic_member_card_labels_each_kind(hover/tests.rs) is extended to render and assert the three new kinds (Factory → "Model factory", FactoryMethod → "Factory method", Pivot → "Pivot model") through the real card function.
Two blocker candidates re-examined and refuted (for the record)
- AC #5 "regression test is vacuous" (re-raised by the test-honesty lens, echoing an earlier pass) — refuted on adversarial verification.
population_skips_factory_state_callsfiresUser::factory()->active()againstSCOPED_MODEL, which genuinely declaresscopeActive(member_resolver/tests.rs:2113). Remove thefactory()-exclusion bail (member_resolver.rs:846-861) andactivefalls through to matchscopeActiveon the model —has_entry(User, "active")flips true and the assertion fails. So the test does lock in AC #5's stated invariant. (The fixture doesn't exercise the factory-present sub-path, but retargeting swaps the classification subject to the factory ClassView atmember_resolver.rs:699-708/846-848, making a model-scope leak structurally impossible once a factory resolves; that path is covered separately byfactory_chain_declared_state_classifies_as_factory_methodandfactory_chain_undeclared_member_never_degrades_to_class_line.) - AC #6 met via a deliberate divergence, nothing dropped — the custom collection surfaces in completion (
get_class_properties+eloquent_completion::relations, both now routed throughcollection_class_for→relationship_to_php_type_with_collection) but not as a hover type-hint. That is exact parity with the AC's own qualifier "the same way relationship types do today": the hover handler populatestype_hintonly forColumn/DynamicFinder(main.rs:18709-18734) and never forRelationship, both before and after this PR. Not a shortfall.
What's Good
- Security is clean. Every new disk read routes through the pre-existing fail-closed containment: factory resolution via the in-memory
ClassFileResolver/index lookups, andcollection_class_forviafind_php_class_file_in_app_or_vendor, whose every candidate join is gated bypath_within_root. No new raw path join from a project-controlled class name. - Cache schema bumps done correctly for both caches (
pattern_disk_cache11→12,magic_disk_cache4→5) — the newMagicMemberKindvariants were spliced before an existing variant, shifting discriminants, so stale caches can't mis-decode. - Tests are honest and substantive — real fixtures, real
analyze()/classifycalls asserting resolved FQCNs and kinds, plus a genuine e2efactory_goto_def_handler.rsthat primes a live Salsa actor and asserts the resolved file + line. - Docs (AC #10) land the examples —
docs/go-to-definition.mdgains factory-chain goto (User::factory()->suspended()) and->pivotgoto;docs/autocomplete.mdgains thePostCollection<Post>custom-collection completion example.
📋 Non-blocking follow-ups
- Blade variable query-builder inference still hardcodes the default
Collection<>—main.rs:11505($var = Model::all()/::get()/::paginate()) andmain.rs:11536($var = Model::where(...)->get()) infind_variable_type_in_content. These two sites type a Blade variable asCollection<Model>and are disconnected from the newcollection_class_formachinery, so a model with a custom$collectionClassstill shows the default collection in this path. Pre-existing (commit95372b5, untouched by this PR) and a separate mechanism from the relationship-surfacing path AC #6 scopes to — so out of scope here, but a real consistency gap worth closing. Filing as a new anchor with both sites enumerated.
Ready for @mikebronner to merge.
find_variable_type_in_content typed a Blade variable assigned from a query-builder terminal (Model::all(), Model::where(...)->get()) as the hardcoded Collection<Model>, ignoring a model's custom $collectionClass / newCollection() override — a divergent surface from the relationship completion path that #269 already taught to honor custom collections. Thread the project root through find_variable_type_in_content and its call chain (extract_controller_variables → search_php_files_for_view_vars → extract_view_vars_from_content → the extract_vars_from_* helpers) so a new collection_label_for_model helper can resolve the model's FQCN (via the file's use/namespace) and read its custom collection through collection_class_for. Falls back to the default Collection when the model declares no override, leaving every other inference pattern unchanged. Adds four mutation-verified tests covering both terminals, the absence-of-override default, and the single-model pattern no-regression. Fixes #271
…ference (#275) * chore: start work on #271 * fix: 🐛 honor custom $collectionClass in Blade variable inference find_variable_type_in_content typed a Blade variable assigned from a query-builder terminal (Model::all(), Model::where(...)->get()) as the hardcoded Collection<Model>, ignoring a model's custom $collectionClass / newCollection() override — a divergent surface from the relationship completion path that #269 already taught to honor custom collections. Thread the project root through find_variable_type_in_content and its call chain (extract_controller_variables → search_php_files_for_view_vars → extract_view_vars_from_content → the extract_vars_from_* helpers) so a new collection_label_for_model helper can resolve the model's FQCN (via the file's use/namespace) and read its custom collection through collection_class_for. Falls back to the default Collection when the model declares no override, leaving every other inference pattern unchanged. Adds four mutation-verified tests covering both terminals, the absence-of-override default, and the single-model pattern no-regression. Fixes #271
Summary
Implements the remaining scope of #30: factory chain navigation (item 3) and custom Eloquent collection / pivot resolution (item 4). Items 1 (relationship navigation) and 2 (facade resolution) were already shipped in PRs #76/#77 and #254 — see the triage comment on the issue.
Changes
factory_resolver.rs(new) — model → factory FQCN resolution: honors an explicitnewFactory()override (parsed from the declaring file through use-aliases + namespace), falls back to Laravel'sDatabase\Factories\…Factoryconvention, gated on the factory class actually resolving via the PSR-4-awareClassFileResolver.member_resolver.rs—classify_againstgains a factory arm: call-formfactoryon a Model view →MagicMemberKind::Factory(live + recipe/index paths).Model::factory()->…chains re-target the chain subject to the factory FQCN; methods genuinely declared on the factory classify asMagicMemberKind::FactoryMethodand navigate to their declaration. The existing relation-hop bail (factory states vs. scopes) is preserved — regression-locked bypopulation_skips_factory_state_calls.chain.rs—ClassView/ModelMetadataparse$collectionClass(property default,newCollection()return type, or itsnew X(…)body) and$pivotClass; restating the framework default is not surfaced as an override.relationship_to_php_type_with_collectionswaps the custom collection FQCN into both completion sites;->pivotclassifies asMagicMemberKind::Pivotwith the resolved pivot FQCN; hover cards label "Model factory" / "Factory method" / "Pivot model".create_magic_member_locationroutes Factory|Pivot goto to the class line.pattern_disk_cache11→12,magic_disk_cache4→5 (bincode, non-self-describing).docs/go-to-definition.mdanddocs/autocomplete.md.Acceptance Criteria
User::factory()resolves the project's factory FQCN (convention or namespaced factories path), honoring an explicitnewFactory()override; goto-definition jumps to the factory filefactory()works for both imported/aliased (User::factory()) and fully-qualified (\App\Models\User::factory()) receiversfactory()call shows a class card for the resolved factory FQCN, matching the existing hover-card styleUser::factory()->state(…)->create()) — clicking a method genuinely declared on the resolved factory navigates to its declaration in the factory filemember_resolver.rs(factory receivers excluded from relationship/scope classification) is preserved and regression-lockedprotected $collectionClass/newCollection()override types query-builder & relationship results as the custom collection FQCN in hover cards and completionprotected $pivotClasssurfaces as the resolved pivot FQCN on the relationship's->pivotaccessor$collectionClass, and$pivotClassparsing (incl. absence-of-override defaults); e2e handler test covers goto-definition onUser::factory()docs/go-to-definition.mdanddocs/autocomplete.mdgain matching examplesTest Plan
cargo testgreen — 2735 tests (2186 lib + 469 + 80 across targets), 0 failedcargo fmt --checkcleancargo clippy --all-targetsclean (exit 0, no lints)chain/tests.rs(collection/pivot parsing + defaults),member_resolver/tests.rs(classification: convention,newFactory, FQ receiver, chained FactoryMethod, no-factory refusal, pivot),tests/factory_goto_def_handler.rs(e2e goto onUser::factory())Fixes #30