From 75b78abddc3b8e76e22a93870787220b86c7125a Mon Sep 17 00:00:00 2001 From: Mike Bronner Date: Tue, 14 Jul 2026 15:04:40 -0700 Subject: [PATCH 1/5] chore: start work on #246 From 6b12e96effb6877080b1867153c1dd2f92b70b77 Mon Sep 17 00:00:00 2001 From: Mike Bronner Date: Tue, 14 Jul 2026 15:17:57 -0700 Subject: [PATCH 2/5] =?UTF-8?q?fix:=20=F0=9F=90=9B=20type=20executed=20rel?= =?UTF-8?q?ation-query=20collections=20against=20the=20related=20model?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit $var = $user->competitions()->select(…)->get() previously typed $var as an EloquentBuilder on the ROOT model (User), so collection chains on it false-positived "Column not found" against the users table. - flow: detect the executed-relation assignment shape (relation call at the chain root, recognised builder methods between, COLLECTION_TERMINATOR on top; instance or static Eloquent root) via resolve_collection_relation - extractor: emit the RelationProperty receiver for it — collection mode at the base model with the relation queued as a pending hop, resolved by the existing async finalize step - finalize: a collection-mode hop that fails to resolve now clears effective_model (stay quiet) instead of falling back to the root model; builder-mode heuristic hops keep the skip fallback Fixes #246 --- laravel-lsp/src/query_chain/chain.rs | 24 ++-- .../src/query_chain/diagnostics/tests.rs | 70 +++++++++ .../src/query_chain/eloquent_completion.rs | 34 +++-- .../query_chain/eloquent_completion/tests.rs | 26 ++++ laravel-lsp/src/query_chain/extractor.rs | 16 +++ .../src/query_chain/extractor/tests.rs | 71 +++++++++ laravel-lsp/src/query_chain/flow.rs | 78 +++++++++- laravel-lsp/src/query_chain/flow/tests.rs | 136 ++++++++++++++++++ 8 files changed, 435 insertions(+), 20 deletions(-) diff --git a/laravel-lsp/src/query_chain/chain.rs b/laravel-lsp/src/query_chain/chain.rs index 6f3fbf8c..1a52cadd 100644 --- a/laravel-lsp/src/query_chain/chain.rs +++ b/laravel-lsp/src/query_chain/chain.rs @@ -84,15 +84,21 @@ pub enum EloquentReceiver { var: String, php_type: Option, }, - /// `$user->competitions->where(...)` — a relationship accessed as a - /// *property* (not a method call) eager-/lazy-loads it into a hydrated - /// `Collection` of the related model, so the chain runs in - /// `BuilderMode::EloquentCollection`. `base_type` is the declared FQCN of - /// the base variable (`$user` → `App\Models\User`), resolved synchronously - /// by the `var_type` resolver; `relation` is the property name - /// (`competitions`). The related model's FQCN can't be resolved in the - /// (synchronous) extractor pass — it needs a model-file read — so the - /// walker seeds `relation` as a pending relation hop + /// A hydrated `Collection` of a *related* model, reached two ways: + /// + /// - `$user->competitions->where(...)` — a relationship accessed as a + /// *property* (not a method call) eager-/lazy-loads it into a hydrated + /// `Collection` of the related model. + /// - `$regs = $user->competitions()->…->get(); $regs->whereIn(...)` — an + /// executed relation query assigned to a variable (issue #246), detected + /// by [`crate::query_chain::flow::resolve_collection_relation`]. + /// + /// Either way the chain runs in `BuilderMode::EloquentCollection`. + /// `base_type` is the resolved FQCN of the base variable (`$user` → + /// `App\Models\User`); `relation` is the relation name (`competitions`). + /// The related model's FQCN can't be resolved in the (synchronous) + /// extractor pass — it needs a model-file read — so the walker seeds + /// `relation` as a pending relation hop /// ([`ChainContext::pending_relation_hops`]) that the async finalize step /// resolves into `effective_model`. `base_type` is `None` when the base /// variable's type can't be determined, in which case completion no-ops. diff --git a/laravel-lsp/src/query_chain/diagnostics/tests.rs b/laravel-lsp/src/query_chain/diagnostics/tests.rs index 22859230..5da8ac02 100644 --- a/laravel-lsp/src/query_chain/diagnostics/tests.rs +++ b/laravel-lsp/src/query_chain/diagnostics/tests.rs @@ -524,6 +524,76 @@ async fn relationship_property_receiver_still_flags_unknown_column_on_related_ta ); } +#[tokio::test] +async fn executed_relation_collection_variable_validates_against_related_table() { + // The exact issue #246 case: an executed relation query (with + // table-qualified select args) assigned to a variable, then filtered as a + // collection. `type` and `id` are competitions columns — zero diagnostics. + let (_dir, root, db) = competitions_project().await; + let source = "competitions()->select('competitions.id', 'competitions.type')->get();\n$registeredCompetitions->whereIn('type', ['league'])->pluck('id');\n"; + let chains = chains_of(source); + + let diags = chain_diagnostics(&chains, &db, &root, source, DiagnosticSeverity::WARNING).await; + assert!( + diags.is_empty(), + "collection from an executed relation query must validate against competitions: {diags:?}" + ); +} + +#[tokio::test] +async fn executed_relation_collection_variable_flags_typo_on_related_table() { + // Companion negative: the receiver fix narrows the table, it does not + // silence diagnostics. `typo` isn't a competitions column — still flagged, + // and against the competitions table (proving validation moved tables). + let (_dir, root, db) = competitions_project().await; + let source = "competitions()->select('competitions.id', 'competitions.type')->get();\n$registeredCompetitions->whereIn('typo', ['league'])->pluck('id');\n"; + let chains = chains_of(source); + + let diags = chain_diagnostics(&chains, &db, &root, source, DiagnosticSeverity::WARNING).await; + assert_eq!( + diags.len(), + 1, + "typo in the collection chain still flags: {diags:?}" + ); + assert_eq!(code_of(&diags[0]), super::CODE_UNKNOWN_COLUMN); + assert!( + diags[0].message.contains("competitions"), + "diagnostic should name the competitions table; got: {}", + diags[0].message + ); +} + +#[tokio::test] +async fn executed_relation_collection_variable_from_static_root() { + // Same detection when the assignment chain is rooted at a static Eloquent + // call instead of an instance variable. + let (_dir, root, db) = competitions_project().await; + let source = "competitions()->get();\n$regs->whereIn('type', ['league'])->pluck('id');\n"; + let chains = chains_of(source); + + let diags = chain_diagnostics(&chains, &db, &root, source, DiagnosticSeverity::WARNING).await; + assert!( + diags.is_empty(), + "static-rooted executed relation must validate against competitions: {diags:?}" + ); +} + +#[tokio::test] +async fn executed_unknown_relation_collection_stays_quiet() { + // The detected relation doesn't exist on User — the collection's element + // type is unknown, so the receiver falls back to no-validation rather + // than false-positiving against the users table. + let (_dir, root, db) = competitions_project().await; + let source = "missingRelation()->get();\n$things->whereIn('type', ['league']);\n"; + let chains = chains_of(source); + + let diags = chain_diagnostics(&chains, &db, &root, source, DiagnosticSeverity::WARNING).await; + assert!( + diags.is_empty(), + "unknown relation must fall back to quiet, not flag against users: {diags:?}" + ); +} + #[tokio::test] async fn flags_unknown_table_in_db_table() { let (_dir, root) = project_with_models(&[("User", USER_MODEL)]); diff --git a/laravel-lsp/src/query_chain/eloquent_completion.rs b/laravel-lsp/src/query_chain/eloquent_completion.rs index d01ae702..0b723c11 100644 --- a/laravel-lsp/src/query_chain/eloquent_completion.rs +++ b/laravel-lsp/src/query_chain/eloquent_completion.rs @@ -749,13 +749,22 @@ pub async fn resolve_related_model( /// whose `competitions` property hop is seeded as the first pending hop. /// /// Each hop resolves against the *running* model via [`resolve_related_model`]. -/// A hop that doesn't name a relationship (an unrecognised builder method like -/// `->limit()`, or a non-relation property) returns `None` and is skipped, -/// leaving the model unchanged — the `ChainEffect::None` fallback. The mode is -/// untouched: a relationship method returns a builder of the related model -/// (`EloquentBuilder` stays `EloquentBuilder`), and the property form already -/// entered the walker as `EloquentCollection`. No-op when there are no hops or -/// the chain has no resolved model to start from. +/// A failed hop (not a relationship, or no readable model file) is handled by +/// mode: +/// +/// - `EloquentBuilder` — the hop was a heuristic guess (any unrecognised +/// method name mid-chain, e.g. `->limit()`), so a miss is skipped, leaving +/// the model unchanged — the `ChainEffect::None` fallback. +/// - `EloquentCollection` — the hop is a genuine relation claim (a +/// `$user->rel->…` property receiver or an executed relation assignment, +/// issue #246), so a miss means the collection's element type is unknown. +/// `effective_model` is cleared and consumers stay quiet rather than +/// false-positiving against the base model's columns. +/// +/// The mode is untouched either way: a relationship method returns a builder +/// of the related model (`EloquentBuilder` stays `EloquentBuilder`), and the +/// collection forms already entered the walker as `EloquentCollection`. No-op +/// when there are no hops or the chain has no resolved model to start from. pub async fn apply_relation_method_hops(ctx: &mut ChainContext, project_root: &Path) { if ctx.pending_relation_hops.is_empty() { return; @@ -765,10 +774,15 @@ pub async fn apply_relation_method_hops(ctx: &mut ChainContext, project_root: &P return; }; for hop in hops { - if let Some(related) = resolve_related_model(¤t, &hop, project_root).await { - current = related; + match resolve_related_model(¤t, &hop, project_root).await { + Some(related) => current = related, + None if ctx.mode == BuilderMode::EloquentCollection => { + ctx.effective_model = None; + return; + } + // Builder mode: not a relationship — keep `current`, try the next. + None => {} } - // `None` → not a relationship; keep `current` and try the next hop. } ctx.effective_model = Some(current); } diff --git a/laravel-lsp/src/query_chain/eloquent_completion/tests.rs b/laravel-lsp/src/query_chain/eloquent_completion/tests.rs index 43bf110d..dd1dc7f5 100644 --- a/laravel-lsp/src/query_chain/eloquent_completion/tests.rs +++ b/laravel-lsp/src/query_chain/eloquent_completion/tests.rs @@ -617,6 +617,32 @@ class Competition extends Model {} ); } +#[tokio::test] +async fn apply_relation_method_hops_clears_model_on_collection_mode_miss() { + // Issue #246 AC: in EloquentCollection mode the hop is a genuine relation + // claim ($user->rel->… or an executed relation assignment). When it + // doesn't resolve, the element type is unknown — effective_model must be + // cleared so consumers stay quiet instead of false-positiving against the + // base model's columns. + let user = r#"hasMany(Competition::class); } +} +"#; + let (_dir, root) = project_with_models_helper(&[("User", user)]).await; + + let mut ctx = make_ctx("App\\Models\\User"); + ctx.mode = BuilderMode::EloquentCollection; + ctx.pending_relation_hops = vec!["notARelation".to_string()]; + apply_relation_method_hops(&mut ctx, &root).await; + assert_eq!( + ctx.effective_model, None, + "unresolvable collection-mode hop must clear the model" + ); +} + #[tokio::test] async fn columns_for_collection_falls_back_when_model_missing() { let dir = TempDir::new().unwrap(); diff --git a/laravel-lsp/src/query_chain/extractor.rs b/laravel-lsp/src/query_chain/extractor.rs index fb234f8e..a65b66ae 100644 --- a/laravel-lsp/src/query_chain/extractor.rs +++ b/laravel-lsp/src/query_chain/extractor.rs @@ -623,6 +623,22 @@ fn member_chain_receiver(node: Node, bytes: &[u8], aliases: &UseAliases) -> Chai return ChainReceiver::Unknown; }; let var = raw.trim_start_matches('$').to_string(); + // Issue #246: `$var = $base->relation()->…->get()` — the variable + // holds a hydrated Collection of the *related* model, not a + // builder on the base model. Detect the executed-relation + // assignment shape first and emit the same receiver as the + // property-access form (`$base->relation->…`): collection mode at + // the base model, with the relation queued as a pending hop for + // the async finalize step. + if let Some((base_type, relation)) = + super::flow::resolve_collection_relation(node, bytes, &var, aliases) + { + return ChainReceiver::Eloquent(EloquentReceiver::RelationProperty { + var, + base_type: Some(base_type), + relation, + }); + } // Phase 9: try to resolve `$var`'s declared class via either a // typed function parameter (`function show(User $user)`) or an // `@var` docblock immediately above the variable's assignment. diff --git a/laravel-lsp/src/query_chain/extractor/tests.rs b/laravel-lsp/src/query_chain/extractor/tests.rs index 76979c14..563b6a08 100644 --- a/laravel-lsp/src/query_chain/extractor/tests.rs +++ b/laravel-lsp/src/query_chain/extractor/tests.rs @@ -1347,3 +1347,74 @@ fn non_subquery_methods_have_no_subquery_columns() { } } } + +// ---- Executed-relation collection receivers (issue #246) ---------------- + +#[test] +fn executed_relation_assignment_yields_relation_property_receiver() { + // `$regs = $user->competitions()->select(…)->get()` then `$regs->whereIn(…)` + // — the use-site chain's receiver must be the collection-mode + // RelationProperty shape (base model + pending relation), NOT an + // InstanceVar typed as the root model in builder mode. + let chains = extract( + r#" +use App\Models\User; +function run(User $user) { + $regs = $user->competitions()->select('competitions.id', 'competitions.type')->get(); + $regs->whereIn('type', ['league']); +} +"#, + ); + let chain = chains + .iter() + .find(|c| { + matches!( + &c.receiver, + ChainReceiver::Eloquent(EloquentReceiver::RelationProperty { var, .. }) + if var == "regs" + ) + }) + .expect("use-site chain should have a RelationProperty receiver"); + match &chain.receiver { + ChainReceiver::Eloquent(EloquentReceiver::RelationProperty { + var, + base_type, + relation, + }) => { + assert_eq!(var, "regs"); + assert_eq!(base_type.as_deref(), Some("App\\Models\\User")); + assert_eq!(relation, "competitions"); + } + _ => unreachable!(), + } +} + +#[test] +fn plain_collection_assignment_keeps_instance_var_receiver() { + // No relation hop in the assignment chain — the variable is a collection + // of the ROOT model and must keep the existing InstanceVar typing. + let chains = extract( + r#" +function run() { + $users = User::query()->where('active', 1)->get(); + $users->pluck('id'); +} +"#, + ); + let chain = chains + .iter() + .find(|c| { + matches!( + &c.receiver, + ChainReceiver::Eloquent(EloquentReceiver::InstanceVar { var, .. }) + if var == "users" + ) + }) + .expect("use-site chain should keep an InstanceVar receiver"); + match &chain.receiver { + ChainReceiver::Eloquent(EloquentReceiver::InstanceVar { php_type, .. }) => { + assert_eq!(php_type.as_deref(), Some("User")); + } + _ => unreachable!(), + } +} diff --git a/laravel-lsp/src/query_chain/flow.rs b/laravel-lsp/src/query_chain/flow.rs index 19a20ce9..740964d1 100644 --- a/laravel-lsp/src/query_chain/flow.rs +++ b/laravel-lsp/src/query_chain/flow.rs @@ -60,7 +60,7 @@ use tree_sitter::Node; -use super::methods::is_eloquent_static_starter; +use super::methods::{is_eloquent_static_starter, is_known_builder_method, COLLECTION_TERMINATORS}; use super::use_aliases::{resolve_class_name, UseAliases}; use crate::salsa_impl::Confidence; @@ -120,6 +120,82 @@ pub fn resolve_expression( classify_rhs(node, bytes, aliases, 0, node.start_byte()) } +/// Detect an executed-relation-query assignment for `var_name` (issue #246): +/// +/// ```php +/// $registeredCompetitions = $user->competitions()->select('competitions.id')->get(); +/// $registeredCompetitions->whereIn('type', …); // ← use site +/// ``` +/// +/// The variable holds a hydrated `Collection` of the *related* model +/// (`Competition`), not a builder on the root model (`User`). The shape is: +/// the latest assignment's RHS is a member-call chain whose deepest call is +/// NOT a recognised builder method (a potential relation hop), every call in +/// between IS a recognised builder method, and the outermost call is a +/// [`COLLECTION_TERMINATORS`] entry (`get`, `pluck`, …). The chain root is +/// either a variable resolvable to a model (via the normal flow walk) or an +/// Eloquent static starter (`User::query()`). +/// +/// Returns `(base_model_fqcn, relation_name)` so the extractor can emit a +/// collection-mode receiver with the relation queued as a pending hop — the +/// relation→related-model resolution needs a model-file read, so it stays +/// deferred to the async finalize step. `None` means the assignment doesn't +/// match the shape (callers fall back to plain flow typing). +pub fn resolve_collection_relation( + use_site: Node, + bytes: &[u8], + var_name: &str, + aliases: &UseAliases, +) -> Option<(String, String)> { + let scope = enclosing_scope(use_site)?; + let (rhs, assignment_start) = + latest_assignment_rhs(scope, bytes, var_name, use_site.start_byte())?; + let outer = unwrap_parens(rhs); + if outer.kind() != "member_call_expression" { + return None; + } + + // Collect the chain's method names outermost → innermost, stopping at + // the first non-call node (the chain root). + let mut methods: Vec<&str> = Vec::new(); + let mut node = outer; + let root = loop { + if node.kind() == "member_call_expression" { + methods.push(node_text(node.child_by_field_name("name")?, bytes)?); + node = node.child_by_field_name("object")?; + } else { + break node; + } + }; + + // Shape gate: a collection terminator on top, the relation candidate at + // the bottom, only recognised builder methods in between. Table-qualified + // column strings in the middle links are just arguments — they never + // affect this method-name walk. + let (&terminator, rest) = methods.split_first()?; + let (&relation, middle) = rest.split_last()?; + if !COLLECTION_TERMINATORS.contains(&terminator) + || is_known_builder_method(relation) + || !middle.iter().all(|m| is_known_builder_method(m)) + { + return None; + } + + // Type the chain root: `$base` via the normal flow walk (bounded at this + // assignment, so we don't re-find the assignment we're inside), or a + // static Eloquent starter (`User::query()->relation()->get()`). + let base = match root.kind() { + "variable_name" => { + let raw = node_text(root, bytes)?; + let base_var = raw.trim_start_matches('$').to_string(); + resolve_with_boundary(root, assignment_start, bytes, &base_var, aliases, 0)?.0 + } + "scoped_call_expression" => classify_scoped_call(root, bytes, aliases)?, + _ => return None, + }; + Some((base, relation.to_string())) +} + /// Like [`resolve`], but also reports the [`Confidence`] tier of the /// resolution. The magic-member engine (M3) uses this so each resolved site /// can be confidence-gated by consumers (find-references/lens take HIGH+MEDIUM; diff --git a/laravel-lsp/src/query_chain/flow/tests.rs b/laravel-lsp/src/query_chain/flow/tests.rs index 4f8daba7..1838c34e 100644 --- a/laravel-lsp/src/query_chain/flow/tests.rs +++ b/laravel-lsp/src/query_chain/flow/tests.rs @@ -552,3 +552,139 @@ function search() { "an extra flow hop lowers confidence" ); } + +// ---- resolve_collection_relation (issue #246) ---------------------------- + +/// Run [`super::resolve_collection_relation`] against the Nth occurrence of +/// `$var` in the snippet. +fn resolve_collection_at(src: &str, var: &str, n: usize) -> Option<(String, String)> { + let wrapped = format!("competitions()->select('competitions.id', 'competitions.type')->get(); + $regs->whereIn('type', ['league']); +} +"#; + assert_eq!( + resolve_collection_at(src, "regs", 1), + Some(("App\\Models\\User".to_string(), "competitions".to_string())) + ); +} + +#[test] +fn collection_relation_detects_static_rooted_chain() { + let src = r#" +function run() { + $regs = User::query()->competitions()->get(); + $regs->whereIn('type', ['league']); +} +"#; + assert_eq!( + resolve_collection_at(src, "regs", 1), + Some(("User".to_string(), "competitions".to_string())) + ); +} + +#[test] +fn collection_relation_accepts_pluck_terminator() { + let src = r#" +function run(User $user) { + $ids = $user->competitions()->pluck('id'); + $ids->filter(); +} +"#; + assert_eq!( + resolve_collection_at(src, "ids", 1), + Some(("User".to_string(), "competitions".to_string())) + ); +} + +#[test] +fn collection_relation_rejects_unexecuted_relation_builder() { + // No collection terminator — `$q` is a relation *builder*, not a + // hydrated collection. Detection must not fire. + let src = r#" +function run(User $user) { + $q = $user->competitions(); + $q->where('type', 'league'); +} +"#; + assert_eq!(resolve_collection_at(src, "q", 1), None); +} + +#[test] +fn collection_relation_rejects_single_model_terminator() { + // `first()` yields a Model, not a Collection. + let src = r#" +function run(User $user) { + $c = $user->competitions()->first(); + $c->load('players'); +} +"#; + assert_eq!(resolve_collection_at(src, "c", 1), None); +} + +#[test] +fn collection_relation_rejects_known_builder_root_call() { + // `where()` is a recognised builder method — the collection holds the + // ROOT model, so the plain flow path (InstanceVar) must keep handling it. + let src = r#" +function run(User $user) { + $users = $user->where('active', 1)->get(); + $users->pluck('id'); +} +"#; + assert_eq!(resolve_collection_at(src, "users", 1), None); +} + +#[test] +fn collection_relation_rejects_unrecognised_middle_call() { + // A second unrecognised call mid-chain could be another relation hop — + // we only model a single hop, so stay out entirely (fall back to the + // plain flow path) rather than resolve to the wrong model. + let src = r#" +function run(User $user) { + $x = $user->competitions()->organizer()->get(); + $x->pluck('id'); +} +"#; + assert_eq!(resolve_collection_at(src, "x", 1), None); +} + +#[test] +fn collection_relation_requires_resolvable_base() { + // `$user` has no typed param / docblock / assignment — the base model is + // unknown, so detection returns None. + let src = r#" +function run($user) { + $regs = $user->competitions()->get(); + $regs->whereIn('type', ['league']); +} +"#; + assert_eq!(resolve_collection_at(src, "regs", 1), None); +} + +#[test] +fn collection_relation_rejects_non_starter_static_root() { + // `Carbon::now()` isn't an Eloquent static starter — not a chain we type. + let src = r#" +function run() { + $x = Carbon::now()->addDays(3)->get(); + $x->whereIn('type', ['league']); +} +"#; + assert_eq!(resolve_collection_at(src, "x", 1), None); +} From 850c93aec2ca63c6f19615125b9a63a63935364f Mon Sep 17 00:00:00 2001 From: Mike Bronner Date: Tue, 14 Jul 2026 17:36:30 -0700 Subject: [PATCH 3/5] =?UTF-8?q?fix:=20=F0=9F=90=9B=20gate=20strict=20relat?= =?UTF-8?q?ion-hop=20clear=20on=20claim=20hops,=20not=20collection=20mode?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #246 strict clear fired for ANY unresolvable pending hop once the chain reached EloquentCollection mode — but heuristic guesses (local scopes like User::forCurrentTenant(), unmodeled builder methods) share the pending-hops queue with genuine relation claims. A scope followed by ->get() flipped the mode, the scope name failed to resolve as a relation, and effective_model was cleared — silencing column-typo diagnostics that main correctly flagged on the root table. - chain: type the queue — RelationHop { name, kind } with RelationHopKind::{Claim, Heuristic} replacing the untyped Vec - cursor: the RelationProperty receiver seed (property access or executed relation assignment) is a Claim; mid-chain unknowns are Heuristic - finalize: a miss clears effective_model only for Claim hops; Heuristic misses keep the skip-and-continue fallback in every mode - tests: regression coverage for the scope-then-terminator shape (diagnostics + unit), plus a dedicated end-to-end completion test for the executed-relation collection shape (review follow-up, AC #5) --- laravel-lsp/src/query_chain/chain.rs | 47 +++++-- laravel-lsp/src/query_chain/cursor.rs | 30 +++-- laravel-lsp/src/query_chain/cursor/tests.rs | 27 ++-- .../src/query_chain/diagnostics/tests.rs | 38 ++++++ .../src/query_chain/eloquent_completion.rs | 20 +-- .../query_chain/eloquent_completion/tests.rs | 119 ++++++++++++++++-- 6 files changed, 239 insertions(+), 42 deletions(-) diff --git a/laravel-lsp/src/query_chain/chain.rs b/laravel-lsp/src/query_chain/chain.rs index 1a52cadd..b80db979 100644 --- a/laravel-lsp/src/query_chain/chain.rs +++ b/laravel-lsp/src/query_chain/chain.rs @@ -366,21 +366,52 @@ pub struct ChainContext { /// Relationship method calls the chain walked through *as builders* /// (`User::query()->competitions()->whereIn('type', …)`) or a /// relationship accessed as a property on the receiver - /// (`$user->competitions->where('type', …)`), in source order. Each is a - /// method/property name on the *running* `effective_model`; the synchronous - /// walker can't read model files, so it records the names here and the - /// async finalize step + /// (`$user->competitions->where('type', …)`), in source order. Each hop + /// names a method/property on the *running* `effective_model`; the + /// synchronous walker can't read model files, so it records them here and + /// the async finalize step /// ([`crate::query_chain::eloquent_completion::apply_relation_method_hops`]) /// walks them via `resolve_related_model`, advancing `effective_model` on - /// each successful hop. A name that doesn't resolve to a relationship is - /// skipped (it was an unrecognised builder call), leaving `effective_model` - /// unchanged — the `ChainEffect::None` fallback. Empty for the common case. - pub pending_relation_hops: Vec, + /// each successful hop. What a *failed* hop means depends on its + /// [`RelationHopKind`] — see that enum for the split. Empty for the + /// common case. + pub pending_relation_hops: Vec, /// The quote character the user is typing inside (`'` or `"`). Used so the /// completion item doesn't double up quotes when inserting. pub quote: char, } +/// One deferred relationship hop ([`ChainContext::pending_relation_hops`]): +/// the method/property name to resolve against the running `effective_model`, +/// plus how confident the walker is that it *is* a relation — which decides +/// what a failed resolve means (see [`RelationHopKind`]). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RelationHop { + pub name: String, + pub kind: RelationHopKind, +} + +/// How a pending relation hop was collected, deciding the failure semantics in +/// [`crate::query_chain::eloquent_completion::apply_relation_method_hops`]. +/// The two kinds share one queue but mean very different things on a miss — +/// conflating them silenced diagnostics on local-scope chains +/// (`User::forCurrentTenant()->get()->where('emial', 1)`). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RelationHopKind { + /// A genuine relation claim, seeded from the + /// [`EloquentReceiver::RelationProperty`] receiver — a relationship read + /// as a property (`$user->competitions->…`) or an executed relation + /// assignment (issue #246). A failed resolve means the collection's + /// element type is unknown: `effective_model` is cleared so consumers + /// stay quiet rather than false-positiving against the base model. + Claim, + /// A heuristic guess — any unrecognised method name seen mid-chain in + /// `EloquentBuilder` mode (a custom local scope, or a builder method we + /// simply don't model). A failed resolve is skipped, leaving the model + /// unchanged — the `ChainEffect::None` fallback. + Heuristic, +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] pub enum BuilderMode { /// Pre-execution Eloquent: full property/relation/cast support. diff --git a/laravel-lsp/src/query_chain/cursor.rs b/laravel-lsp/src/query_chain/cursor.rs index 357034ae..3cd38614 100644 --- a/laravel-lsp/src/query_chain/cursor.rs +++ b/laravel-lsp/src/query_chain/cursor.rs @@ -417,22 +417,31 @@ fn initial_receiver_context( } /// The relationship hops the walker must apply (deferred to async finalize), -/// assembled in source order: the receiver's property hop first (for a -/// `$var->relation->…` chain), then every unrecognised method call seen *before* -/// `up_to_idx` while the chain is still an `EloquentBuilder`. Collecting only in -/// `EloquentBuilder` mode is deliberate — once the chain flips to a Collection -/// or base builder, an unknown call is a Collection/array method, not a relation -/// on the model. +/// assembled in source order: the receiver's hop first (for a +/// `$var->relation->…` chain or an executed relation assignment, issue #246), +/// then every unrecognised method call seen *before* `up_to_idx` while the +/// chain is still an `EloquentBuilder`. Collecting only in `EloquentBuilder` +/// mode is deliberate — once the chain flips to a Collection or base builder, +/// an unknown call is a Collection/array method, not a relation on the model. +/// +/// The receiver's hop is a [`RelationHopKind::Claim`] (the walker *knows* it +/// names a relation-shaped access); mid-chain unknowns are +/// [`RelationHopKind::Heuristic`] guesses (they may be local scopes or +/// unmodeled builder methods). The finalize step treats a miss differently +/// per kind — see [`RelationHopKind`]. fn pending_relation_hops_through( chain: &BuilderChain, mode: BuilderMode, up_to_idx: usize, -) -> Vec { +) -> Vec { let mut hops = Vec::new(); if let ChainReceiver::Eloquent(EloquentReceiver::RelationProperty { relation, .. }) = &chain.receiver { - hops.push(relation.clone()); + hops.push(RelationHop { + name: relation.clone(), + kind: RelationHopKind::Claim, + }); } let mut running = mode; for link in chain.links.iter().take(up_to_idx) { @@ -447,7 +456,10 @@ fn pending_relation_hops_through( ChainEffect::None => { if running == BuilderMode::EloquentBuilder && !is_known_builder_method(&link.method) { - hops.push(link.method.clone()); + hops.push(RelationHop { + name: link.method.clone(), + kind: RelationHopKind::Heuristic, + }); } } } diff --git a/laravel-lsp/src/query_chain/cursor/tests.rs b/laravel-lsp/src/query_chain/cursor/tests.rs index 267f5eb4..a71aef27 100644 --- a/laravel-lsp/src/query_chain/cursor/tests.rs +++ b/laravel-lsp/src/query_chain/cursor/tests.rs @@ -542,18 +542,27 @@ fn eloquent_static_with_first_arg_resolves_to_relation_completion() { // ---- relation hops (issue #211) ------------------------------------------ +/// Shorthand for the expected-hops assertions below. +fn hop(name: &str, kind: RelationHopKind) -> RelationHop { + RelationHop { + name: name.to_string(), + kind, + } +} + #[test] fn unknown_relation_method_call_queues_a_pending_hop() { // `competitions()` isn't a known builder method, so in EloquentBuilder mode - // the walker queues it as a relation hop to resolve against the model. Mode - // stays EloquentBuilder (a relation method returns a builder). + // the walker queues it as a relation hop to resolve against the model — as + // a Heuristic guess (it could equally be a local scope). Mode stays + // EloquentBuilder (a relation method returns a builder). let ctx = detect("User::query()->competitions()->whereIn('ty|pe', ['x']);").expect("ctx"); assert_eq!(ctx.mode, BuilderMode::EloquentBuilder); assert_eq!(ctx.effective_model.as_deref(), Some("User")); assert!( ctx.pending_relation_hops - .contains(&"competitions".to_string()), - "competitions() should be queued as a relation hop; got {:?}", + .contains(&hop("competitions", RelationHopKind::Heuristic)), + "competitions() should be queued as a heuristic relation hop; got {:?}", ctx.pending_relation_hops ); } @@ -581,7 +590,7 @@ fn eloquent_static_starter_methods_do_not_queue_relation_hops() { .expect("ctx"); assert_eq!( ctx.pending_relation_hops, - vec!["competitions".to_string()], + vec![hop("competitions", RelationHopKind::Heuristic)], "only the relation accessor should be queued; got {:?}", ctx.pending_relation_hops ); @@ -591,14 +600,18 @@ fn eloquent_static_starter_methods_do_not_queue_relation_hops() { fn relation_property_receiver_starts_collection_with_pending_hop() { // `$user->competitions->where('|')` — the relation read as a property is a // Collection of the related model; the property name is the first pending - // hop and the chain runs in EloquentCollection mode. + // hop (a genuine Claim, not a heuristic guess) and the chain runs in + // EloquentCollection mode. let ctx = detect( "use App\\Models\\User;\n/** @var User $user */\n$user->competitions->where('ty|pe', 'x');", ) .expect("ctx"); assert_eq!(ctx.mode, BuilderMode::EloquentCollection); assert_eq!(ctx.effective_model.as_deref(), Some("App\\Models\\User")); - assert_eq!(ctx.pending_relation_hops, vec!["competitions".to_string()]); + assert_eq!( + ctx.pending_relation_hops, + vec![hop("competitions", RelationHopKind::Claim)] + ); } #[test] diff --git a/laravel-lsp/src/query_chain/diagnostics/tests.rs b/laravel-lsp/src/query_chain/diagnostics/tests.rs index 5da8ac02..275cbf94 100644 --- a/laravel-lsp/src/query_chain/diagnostics/tests.rs +++ b/laravel-lsp/src/query_chain/diagnostics/tests.rs @@ -578,6 +578,44 @@ async fn executed_relation_collection_variable_from_static_root() { ); } +#[tokio::test] +async fn local_scope_before_terminator_still_flags_typo_on_root_table() { + // Regression (PR #266 review): `forCurrentTenant` is a local scope, not a + // relation — it's queued as a *heuristic* hop, and `->get()` then flips + // the chain to EloquentCollection. The heuristic miss must be skipped + // (the collection still holds Users), so the `emial` typo stays flagged + // on the users table. The strict claim-miss clear must not fire here. + let user_with_scope = r#"where('id', 1); } +} +"#; + let (_dir, root) = project_with_models(&[("User", user_with_scope)]); + let db = provider_with( + root.clone(), + &[("users", &[("id", "int"), ("email", "string")])], + ) + .await; + let source = + "get()->where('emial', 1);\n"; + let chains = chains_of(source); + + let diags = chain_diagnostics(&chains, &db, &root, source, DiagnosticSeverity::WARNING).await; + assert_eq!( + diags.len(), + 1, + "typo after a scope-then-terminator chain must still flag: {diags:?}" + ); + assert_eq!(code_of(&diags[0]), super::CODE_UNKNOWN_COLUMN); + assert!( + diags[0].message.contains("users"), + "diagnostic should name the users table; got: {}", + diags[0].message + ); +} + #[tokio::test] async fn executed_unknown_relation_collection_stays_quiet() { // The detected relation doesn't exist on User — the collection's element diff --git a/laravel-lsp/src/query_chain/eloquent_completion.rs b/laravel-lsp/src/query_chain/eloquent_completion.rs index 0b723c11..af4effd9 100644 --- a/laravel-lsp/src/query_chain/eloquent_completion.rs +++ b/laravel-lsp/src/query_chain/eloquent_completion.rs @@ -750,12 +750,16 @@ pub async fn resolve_related_model( /// /// Each hop resolves against the *running* model via [`resolve_related_model`]. /// A failed hop (not a relationship, or no readable model file) is handled by -/// mode: +/// the hop's [`RelationHopKind`] — NOT the chain's cursor-time mode, which +/// says nothing about which *hop* missed (a heuristic hop collected in +/// builder mode is still a heuristic after `->get()` flips the chain to a +/// Collection): /// -/// - `EloquentBuilder` — the hop was a heuristic guess (any unrecognised -/// method name mid-chain, e.g. `->limit()`), so a miss is skipped, leaving -/// the model unchanged — the `ChainEffect::None` fallback. -/// - `EloquentCollection` — the hop is a genuine relation claim (a +/// - [`RelationHopKind::Heuristic`] — a guess (any unrecognised method name +/// mid-chain: a local scope like `->forCurrentTenant()`, or a builder +/// method we don't model), so a miss is skipped, leaving the model +/// unchanged — the `ChainEffect::None` fallback. +/// - [`RelationHopKind::Claim`] — a genuine relation claim (a /// `$user->rel->…` property receiver or an executed relation assignment, /// issue #246), so a miss means the collection's element type is unknown. /// `effective_model` is cleared and consumers stay quiet rather than @@ -774,13 +778,13 @@ pub async fn apply_relation_method_hops(ctx: &mut ChainContext, project_root: &P return; }; for hop in hops { - match resolve_related_model(¤t, &hop, project_root).await { + match resolve_related_model(¤t, &hop.name, project_root).await { Some(related) => current = related, - None if ctx.mode == BuilderMode::EloquentCollection => { + None if hop.kind == RelationHopKind::Claim => { ctx.effective_model = None; return; } - // Builder mode: not a relationship — keep `current`, try the next. + // Heuristic miss: not a relationship — keep `current`, try the next. None => {} } } diff --git a/laravel-lsp/src/query_chain/eloquent_completion/tests.rs b/laravel-lsp/src/query_chain/eloquent_completion/tests.rs index dd1dc7f5..dfc751c0 100644 --- a/laravel-lsp/src/query_chain/eloquent_completion/tests.rs +++ b/laravel-lsp/src/query_chain/eloquent_completion/tests.rs @@ -104,6 +104,14 @@ async fn provider_with_table( provider } +/// Shorthand for seeding `pending_relation_hops` in the tests below. +fn hop(name: &str, kind: RelationHopKind) -> RelationHop { + RelationHop { + name: name.to_string(), + kind, + } +} + fn make_ctx(class: &str) -> ChainContext { ChainContext { mode: BuilderMode::EloquentBuilder, @@ -559,7 +567,7 @@ class Competition extends Model {} // The shape the chain walker produces for `$user->competitions->where('|')`. let mut ctx = make_ctx("App\\Models\\User"); ctx.mode = BuilderMode::EloquentCollection; - ctx.pending_relation_hops = vec!["competitions".to_string()]; + ctx.pending_relation_hops = vec![hop("competitions", RelationHopKind::Claim)]; apply_relation_method_hops(&mut ctx, &root).await; assert_eq!( @@ -580,6 +588,66 @@ class Competition extends Model {} ); } +#[tokio::test] +async fn executed_relation_collection_variable_offers_related_columns() { + // Issue #246 AC #5, end-to-end from source: column completion on a + // collection variable holding an executed relation query must offer the + // *related* model's columns. Unlike the hand-built-ctx test above, this + // drives the real extractor → cursor → finalize pipeline, so it also + // covers the executed-relation receiver construction (flow.rs). + let user = r#"hasMany(Competition::class); } +} +"#; + let competition = r#"competitions()->select('competitions.id', 'competitions.type')->get();\n$registeredCompetitions->where('');\n"; + // Cursor between the quotes of `where('|')`. + let pos = source.rfind("where('").expect("fixture") + "where('".len(); + let tree = crate::parser::parse_php(source).expect("parse"); + let chains: Vec> = + crate::query_chain::extract_chains(&tree, source) + .into_iter() + .map(std::sync::Arc::new) + .collect(); + let mut ctx = crate::query_chain::cursor::detect_chain_context_at(&chains, pos) + .expect("ctx for the collection where('|')"); + assert_eq!(ctx.mode, BuilderMode::EloquentCollection); + + apply_relation_method_hops(&mut ctx, &root).await; + assert_eq!( + ctx.effective_model.as_deref(), + Some("App\\Models\\Competition"), + "the pending claim hop must advance effective_model to the related class" + ); + + let items = columns_for_collection(&ctx, &db, None, &root).await; + let labels: Vec<&str> = items.iter().map(|i| i.label.as_str()).collect(); + assert!( + labels.contains(&"type"), + "must offer competitions.type; got {labels:?}" + ); + assert!( + labels.contains(&"name"), + "must offer competitions.name; got {labels:?}" + ); +} + #[tokio::test] async fn apply_relation_method_hops_skips_unresolvable_names() { // A hop that isn't a relationship (an unrecognised builder method like @@ -604,7 +672,10 @@ class Competition extends Model {} let mut ctx = make_ctx("App\\Models\\User"); // `query` is not a relation → skipped; `competitions` resolves. - ctx.pending_relation_hops = vec!["query".to_string(), "competitions".to_string()]; + ctx.pending_relation_hops = vec![ + hop("query", RelationHopKind::Heuristic), + hop("competitions", RelationHopKind::Heuristic), + ]; apply_relation_method_hops(&mut ctx, &root).await; assert_eq!( ctx.effective_model.as_deref(), @@ -618,12 +689,12 @@ class Competition extends Model {} } #[tokio::test] -async fn apply_relation_method_hops_clears_model_on_collection_mode_miss() { - // Issue #246 AC: in EloquentCollection mode the hop is a genuine relation - // claim ($user->rel->… or an executed relation assignment). When it - // doesn't resolve, the element type is unknown — effective_model must be - // cleared so consumers stay quiet instead of false-positiving against the - // base model's columns. +async fn apply_relation_method_hops_clears_model_on_claim_miss() { + // Issue #246 AC: a Claim hop is a genuine relation claim ($user->rel->… + // or an executed relation assignment). When it doesn't resolve, the + // collection's element type is unknown — effective_model must be cleared + // so consumers stay quiet instead of false-positiving against the base + // model's columns. let user = r#"…` — a local scope) is still a heuristic + // after `->get()` flips the chain to EloquentCollection. A miss must be + // skipped (the collection's elements are still Users), NOT clear the + // model — clearing here silenced typo diagnostics on scope-then-terminator + // chains. + let user = r#"hasMany(Competition::class); } +} +"#; + let (_dir, root) = project_with_models_helper(&[("User", user)]).await; + + let mut ctx = make_ctx("App\\Models\\User"); + ctx.mode = BuilderMode::EloquentCollection; + ctx.pending_relation_hops = vec![hop("forCurrentTenant", RelationHopKind::Heuristic)]; + apply_relation_method_hops(&mut ctx, &root).await; + assert_eq!( + ctx.effective_model.as_deref(), + Some("App\\Models\\User"), + "heuristic miss must keep the model even in collection mode" ); } From d1ec859b8d899509af90dc5adffff2976a2a2860 Mon Sep 17 00:00:00 2001 From: Mike Bronner Date: Tue, 14 Jul 2026 19:00:34 -0700 Subject: [PATCH 4/5] =?UTF-8?q?fix:=20=F0=9F=90=9B=20discriminate=20scope?= =?UTF-8?q?=20calls=20and=20mode-flips=20in=20executed-relation=20collecti?= =?UTF-8?q?ons?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- laravel-lsp/src/query_chain/chain.rs | 33 +++- laravel-lsp/src/query_chain/cursor.rs | 19 ++- .../src/query_chain/diagnostics/tests.rs | 97 +++++++++++- .../src/query_chain/eloquent_completion.rs | 63 ++++++-- .../query_chain/eloquent_completion/tests.rs | 141 ++++++++++++++++++ laravel-lsp/src/query_chain/extractor.rs | 20 ++- .../src/query_chain/extractor/tests.rs | 1 + laravel-lsp/src/query_chain/flow.rs | 133 +++++++++++++---- laravel-lsp/src/query_chain/flow/tests.rs | 61 +++++++- 9 files changed, 507 insertions(+), 61 deletions(-) diff --git a/laravel-lsp/src/query_chain/chain.rs b/laravel-lsp/src/query_chain/chain.rs index b80db979..56456973 100644 --- a/laravel-lsp/src/query_chain/chain.rs +++ b/laravel-lsp/src/query_chain/chain.rs @@ -102,10 +102,19 @@ pub enum EloquentReceiver { /// ([`ChainContext::pending_relation_hops`]) that the async finalize step /// resolves into `effective_model`. `base_type` is `None` when the base /// variable's type can't be determined, in which case completion no-ops. + /// + /// `from_call` records which of the two construction sites produced the + /// receiver — `false` for the property access, `true` for the executed + /// relation *call*. The distinction matters on a failed hop: a property + /// name that isn't a relation is an unknown collection (stay quiet), but + /// a *called* name may still be a local scope returning a builder of the + /// base model itself — see [`RelationHopKind::CallClaim`]. RelationProperty { var: String, base_type: Option, relation: String, + #[serde(default)] + from_call: bool, }, } @@ -393,18 +402,30 @@ pub struct RelationHop { /// How a pending relation hop was collected, deciding the failure semantics in /// [`crate::query_chain::eloquent_completion::apply_relation_method_hops`]. -/// The two kinds share one queue but mean very different things on a miss — +/// The kinds share one queue but mean very different things on a miss — /// conflating them silenced diagnostics on local-scope chains -/// (`User::forCurrentTenant()->get()->where('emial', 1)`). +/// (`User::forCurrentTenant()->get()->where('emial', 1)`), first inline +/// (fixed by the `Claim`/`Heuristic` split) and then in the assigned form +/// (`$x = $user->forCurrentTenant()->get()`, fixed by `CallClaim`). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum RelationHopKind { - /// A genuine relation claim, seeded from the - /// [`EloquentReceiver::RelationProperty`] receiver — a relationship read - /// as a property (`$user->competitions->…`) or an executed relation - /// assignment (issue #246). A failed resolve means the collection's + /// A genuine relation claim, seeded from a *property* access + /// ([`EloquentReceiver::RelationProperty`] with `from_call: false`, + /// `$user->competitions->…`). A property that isn't a relation has no + /// modelled type at all, so a failed resolve means the collection's /// element type is unknown: `effective_model` is cleared so consumers /// stay quiet rather than false-positiving against the base model. Claim, + /// A relation claim seeded from an *executed relation-query assignment* + /// ([`EloquentReceiver::RelationProperty`] with `from_call: true`, + /// `$x = $user->competitions()->get()`, issue #246). The claimed name is + /// a method *call*, so a resolve miss splits further: a **local scope** + /// (`scopeForCurrentTenant` / `#[Scope]`) provably returns a builder of + /// the *same* model — the collection's element type is the running model, + /// which is kept so its columns still validate. Any other miss (an + /// undeclared name, or a declared relation whose related model can't be + /// resolved — AC #7) clears `effective_model`, same as [`Self::Claim`]. + CallClaim, /// A heuristic guess — any unrecognised method name seen mid-chain in /// `EloquentBuilder` mode (a custom local scope, or a builder method we /// simply don't model). A failed resolve is skipped, leaving the model diff --git a/laravel-lsp/src/query_chain/cursor.rs b/laravel-lsp/src/query_chain/cursor.rs index 3cd38614..6c2566c7 100644 --- a/laravel-lsp/src/query_chain/cursor.rs +++ b/laravel-lsp/src/query_chain/cursor.rs @@ -424,8 +424,10 @@ fn initial_receiver_context( /// mode is deliberate — once the chain flips to a Collection or base builder, /// an unknown call is a Collection/array method, not a relation on the model. /// -/// The receiver's hop is a [`RelationHopKind::Claim`] (the walker *knows* it -/// names a relation-shaped access); mid-chain unknowns are +/// The receiver's hop is a claim (the walker *knows* it names a +/// relation-shaped access): [`RelationHopKind::Claim`] for the property form, +/// [`RelationHopKind::CallClaim`] for the executed-relation call form (whose +/// miss may still be a local scope). Mid-chain unknowns are /// [`RelationHopKind::Heuristic`] guesses (they may be local scopes or /// unmodeled builder methods). The finalize step treats a miss differently /// per kind — see [`RelationHopKind`]. @@ -435,12 +437,19 @@ fn pending_relation_hops_through( up_to_idx: usize, ) -> Vec { let mut hops = Vec::new(); - if let ChainReceiver::Eloquent(EloquentReceiver::RelationProperty { relation, .. }) = - &chain.receiver + if let ChainReceiver::Eloquent(EloquentReceiver::RelationProperty { + relation, + from_call, + .. + }) = &chain.receiver { hops.push(RelationHop { name: relation.clone(), - kind: RelationHopKind::Claim, + kind: if *from_call { + RelationHopKind::CallClaim + } else { + RelationHopKind::Claim + }, }); } let mut running = mode; diff --git a/laravel-lsp/src/query_chain/diagnostics/tests.rs b/laravel-lsp/src/query_chain/diagnostics/tests.rs index 275cbf94..261115ec 100644 --- a/laravel-lsp/src/query_chain/diagnostics/tests.rs +++ b/laravel-lsp/src/query_chain/diagnostics/tests.rs @@ -618,9 +618,12 @@ class User extends Model { #[tokio::test] async fn executed_unknown_relation_collection_stays_quiet() { - // The detected relation doesn't exist on User — the collection's element - // type is unknown, so the receiver falls back to no-validation rather - // than false-positiving against the users table. + // The called name is genuinely undeclared on User — not a relation, not + // a local scope, not any method — so the collection's element type is + // unknown and the receiver falls back to no-validation rather than + // false-positiving against the users table (AC #7). Contrast the + // assigned_local_scope_* tests below, where a *declared scope* keeps + // validating against the root table. let (_dir, root, db) = competitions_project().await; let source = "missingRelation()->get();\n$things->whereIn('type', ['league']);\n"; let chains = chains_of(source); @@ -632,6 +635,94 @@ async fn executed_unknown_relation_collection_stays_quiet() { ); } +/// `User` with a declared local scope, for the assigned scope-collection +/// regression pair (PR #266 review, round 3): a scope call returns a builder +/// of User itself, so the executed collection still holds Users. +const USER_WITH_TENANT_SCOPE: &str = r#"where('id', 1); } +} +"#; + +#[tokio::test] +async fn assigned_local_scope_collection_still_flags_typo_on_root_table() { + // Regression (PR #266 review, round 3): the *assigned* form of the + // scope-then-terminator chain. `forCurrentTenant` is a declared local + // scope, not a relation — the executed collection still holds Users, so + // the `emial` typo stays flagged on the users table. The CallClaim miss + // must consult the model's scopes instead of blanket-clearing. + let (_dir, root) = project_with_models(&[("User", USER_WITH_TENANT_SCOPE)]); + let db = provider_with( + root.clone(), + &[("users", &[("id", "int"), ("email", "string")])], + ) + .await; + let source = "forCurrentTenant()->get();\n$x->where('emial', 1);\n"; + let chains = chains_of(source); + + let diags = chain_diagnostics(&chains, &db, &root, source, DiagnosticSeverity::WARNING).await; + assert_eq!( + diags.len(), + 1, + "typo on an assigned scope collection must still flag: {diags:?}" + ); + assert_eq!(code_of(&diags[0]), super::CODE_UNKNOWN_COLUMN); + assert!( + diags[0].message.contains("users"), + "diagnostic should name the users table; got: {}", + diags[0].message + ); +} + +#[tokio::test] +async fn assigned_local_scope_collection_keeps_valid_columns_quiet() { + // Companion positive: a valid users column on the same shape stays quiet + // — the kept root model validates, it doesn't blanket-flag. + let (_dir, root) = project_with_models(&[("User", USER_WITH_TENANT_SCOPE)]); + let db = provider_with( + root.clone(), + &[("users", &[("id", "int"), ("email", "string")])], + ) + .await; + let source = "forCurrentTenant()->get();\n$x->where('email', 1);\n"; + let chains = chains_of(source); + + let diags = chain_diagnostics(&chains, &db, &root, source, DiagnosticSeverity::WARNING).await; + assert!( + diags.is_empty(), + "valid column on an assigned scope collection must stay quiet: {diags:?}" + ); +} + +#[tokio::test] +async fn assigned_static_scope_collection_still_flags_typo_on_root_table() { + // Static-rooted variant of the same regression: + // `User::query()->forCurrentTenant()->get()` assigned to a variable. + let (_dir, root) = project_with_models(&[("User", USER_WITH_TENANT_SCOPE)]); + let db = provider_with( + root.clone(), + &[("users", &[("id", "int"), ("email", "string")])], + ) + .await; + let source = "forCurrentTenant()->get();\n$x->where('emial', 1);\n"; + let chains = chains_of(source); + + let diags = chain_diagnostics(&chains, &db, &root, source, DiagnosticSeverity::WARNING).await; + assert_eq!( + diags.len(), + 1, + "typo on a static-rooted assigned scope collection must still flag: {diags:?}" + ); + assert_eq!(code_of(&diags[0]), super::CODE_UNKNOWN_COLUMN); + assert!( + diags[0].message.contains("users"), + "diagnostic should name the users table; got: {}", + diags[0].message + ); +} + #[tokio::test] async fn flags_unknown_table_in_db_table() { let (_dir, root) = project_with_models(&[("User", USER_MODEL)]); diff --git a/laravel-lsp/src/query_chain/eloquent_completion.rs b/laravel-lsp/src/query_chain/eloquent_completion.rs index af4effd9..9648cf4b 100644 --- a/laravel-lsp/src/query_chain/eloquent_completion.rs +++ b/laravel-lsp/src/query_chain/eloquent_completion.rs @@ -759,11 +759,17 @@ pub async fn resolve_related_model( /// mid-chain: a local scope like `->forCurrentTenant()`, or a builder /// method we don't model), so a miss is skipped, leaving the model /// unchanged — the `ChainEffect::None` fallback. -/// - [`RelationHopKind::Claim`] — a genuine relation claim (a -/// `$user->rel->…` property receiver or an executed relation assignment, -/// issue #246), so a miss means the collection's element type is unknown. -/// `effective_model` is cleared and consumers stay quiet rather than -/// false-positiving against the base model's columns. +/// - [`RelationHopKind::Claim`] — a relation claim from a `$user->rel->…` +/// *property* receiver, so a miss means the collection's element type is +/// unknown. `effective_model` is cleared and consumers stay quiet rather +/// than false-positiving against the base model's columns. +/// - [`RelationHopKind::CallClaim`] — a relation claim from an executed +/// relation *call* assignment (`$x = $user->rel()->get()`, issue #246). +/// A miss splits on what the called name is (PR #266 review): a declared +/// **local scope** returns a builder of the *same* model, so the running +/// model is kept and its columns still validate; anything else (an +/// undeclared name, or a declared relation with no resolvable related +/// model — AC #7) clears `effective_model`, same as `Claim`. /// /// The mode is untouched either way: a relationship method returns a builder /// of the related model (`EloquentBuilder` stays `EloquentBuilder`), and the @@ -780,17 +786,52 @@ pub async fn apply_relation_method_hops(ctx: &mut ChainContext, project_root: &P for hop in hops { match resolve_related_model(¤t, &hop.name, project_root).await { Some(related) => current = related, - None if hop.kind == RelationHopKind::Claim => { - ctx.effective_model = None; - return; - } - // Heuristic miss: not a relationship — keep `current`, try the next. - None => {} + None => match hop.kind { + // Heuristic miss: not a relationship — keep `current`, try + // the next. + RelationHopKind::Heuristic => {} + // Property-form claim miss: the element type is unknown — + // stay quiet. + RelationHopKind::Claim => { + ctx.effective_model = None; + return; + } + // Call-form claim miss: a declared local scope returns a + // builder of the *same* model, so the collection still holds + // `current` — keep it. Anything else is an unknown element + // type — stay quiet (AC #7). + RelationHopKind::CallClaim => { + if !is_local_scope(¤t, &hop.name, project_root).await { + ctx.effective_model = None; + return; + } + } + }, } } ctx.effective_model = Some(current); } +/// Whether `method_name` is a declared Eloquent local scope on `class` — +/// either style (`scopeForCurrentTenant` prefix or `#[Scope]` attribute), +/// inheritance- and trait-aware via the class-chain walk. Drives the +/// `CallClaim` miss split in [`apply_relation_method_hops`]: a scope call +/// provably returns a builder of the same model, so the executed collection's +/// element type is the running model, not an unknown. +async fn is_local_scope(class: &str, method_name: &str, project_root: &Path) -> bool { + let Some(path) = find_php_class_file(class, project_root) else { + return false; + }; + let root = project_root.to_path_buf(); + let name = method_name.to_string(); + tokio::task::spawn_blocking(move || { + crate::laravel_introspector::chain::analyze(&path, &root) + .is_some_and(|view| view.scopes.iter().any(|s| s.name == name)) + }) + .await + .unwrap_or(false) +} + /// Phase 6 helper: resolve a model class to its table name. /// /// Used for the post-`->toBase()` case where the chain has flipped to diff --git a/laravel-lsp/src/query_chain/eloquent_completion/tests.rs b/laravel-lsp/src/query_chain/eloquent_completion/tests.rs index dfc751c0..f291501b 100644 --- a/laravel-lsp/src/query_chain/eloquent_completion/tests.rs +++ b/laravel-lsp/src/query_chain/eloquent_completion/tests.rs @@ -648,6 +648,147 @@ class Competition extends Model {} ); } +#[tokio::test] +async fn apply_relation_method_hops_keeps_model_on_call_claim_scope_miss() { + // PR #266 review (round 3): an executed-relation assignment whose called + // name is a declared local scope — the collection holds the *root* + // model, so the CallClaim miss keeps effective_model and its columns + // still validate. + let user = r#"competitions()->get()` — mirroring the + // instance-rooted test above. + let user = r#"hasMany(Competition::class); } +} +"#; + let competition = r#"competitions()->get();\n$regs->where('');\n"; + let pos = source.rfind("where('").expect("fixture") + "where('".len(); + let tree = crate::parser::parse_php(source).expect("parse"); + let chains: Vec> = + crate::query_chain::extract_chains(&tree, source) + .into_iter() + .map(std::sync::Arc::new) + .collect(); + let mut ctx = crate::query_chain::cursor::detect_chain_context_at(&chains, pos) + .expect("ctx for the collection where('|')"); + assert_eq!(ctx.mode, BuilderMode::EloquentCollection); + + apply_relation_method_hops(&mut ctx, &root).await; + assert_eq!( + ctx.effective_model.as_deref(), + Some("App\\Models\\Competition"), + "the pending claim hop must advance effective_model to the related class" + ); + + let items = columns_for_collection(&ctx, &db, None, &root).await; + let labels: Vec<&str> = items.iter().map(|i| i.label.as_str()).collect(); + assert!( + labels.contains(&"type"), + "must offer competitions.type; got {labels:?}" + ); +} + +#[tokio::test] +async fn unknown_relation_collection_offers_no_completions() { + // Follow-up (PR #266 review): AC #7's no-completion half, asserted + // directly — an unresolvable relation's collection variable offers an + // *empty* completion list, never the root model's columns. + let user = r#"missingRelation()->get();\n$things->where('');\n"; + let pos = source.rfind("where('").expect("fixture") + "where('".len(); + let tree = crate::parser::parse_php(source).expect("parse"); + let chains: Vec> = + crate::query_chain::extract_chains(&tree, source) + .into_iter() + .map(std::sync::Arc::new) + .collect(); + let mut ctx = crate::query_chain::cursor::detect_chain_context_at(&chains, pos) + .expect("ctx for the collection where('|')"); + + apply_relation_method_hops(&mut ctx, &root).await; + assert_eq!( + ctx.effective_model, None, + "an unresolvable relation clears the model" + ); + + let items = columns_for_collection(&ctx, &db, None, &root).await; + assert!( + items.is_empty(), + "no completions for an unknown-relation collection; got {:?}", + items.iter().map(|i| &i.label).collect::>() + ); +} + #[tokio::test] async fn apply_relation_method_hops_skips_unresolvable_names() { // A hop that isn't a relationship (an unrecognised builder method like diff --git a/laravel-lsp/src/query_chain/extractor.rs b/laravel-lsp/src/query_chain/extractor.rs index a65b66ae..9677a469 100644 --- a/laravel-lsp/src/query_chain/extractor.rs +++ b/laravel-lsp/src/query_chain/extractor.rs @@ -623,20 +623,26 @@ fn member_chain_receiver(node: Node, bytes: &[u8], aliases: &UseAliases) -> Chai return ChainReceiver::Unknown; }; let var = raw.trim_start_matches('$').to_string(); + // One innermost-scope assignment fetch feeds both the issue-#246 + // shape detection and the plain flow-typing fallback below. + let assignment = super::flow::latest_assignment_before(node, bytes, &var); // Issue #246: `$var = $base->relation()->…->get()` — the variable // holds a hydrated Collection of the *related* model, not a // builder on the base model. Detect the executed-relation // assignment shape first and emit the same receiver as the // property-access form (`$base->relation->…`): collection mode at // the base model, with the relation queued as a pending hop for - // the async finalize step. - if let Some((base_type, relation)) = - super::flow::resolve_collection_relation(node, bytes, &var, aliases) - { + // the async finalize step. `from_call: true` — the claimed name + // is a method call, so a finalize miss may still be a local + // scope (see [`RelationHopKind::CallClaim`]). + if let Some((base_type, relation)) = assignment.and_then(|(rhs, start)| { + super::flow::resolve_collection_relation(rhs, start, bytes, aliases) + }) { return ChainReceiver::Eloquent(EloquentReceiver::RelationProperty { var, base_type: Some(base_type), relation, + from_call: true, }); } // Phase 9: try to resolve `$var`'s declared class via either a @@ -646,7 +652,10 @@ fn member_chain_receiver(node: Node, bytes: &[u8], aliases: &UseAliases) -> Chai // and Livewire components. `None` is acceptable — receiver // resolution will still fall back to Phase 8's closure-scope // path if applicable; otherwise completion silently no-ops. - let php_type = super::var_type::resolve(node, bytes, &var, aliases); + // The pre-fetched assignment is threaded through so the flow + // walk's first scope iteration doesn't re-scan for it. + let php_type = + super::flow::resolve_with_assignment(node, bytes, &var, aliases, assignment); ChainReceiver::Eloquent(EloquentReceiver::InstanceVar { var, php_type }) } // `(new self)->with(...)` / `(new User)->where(...)` etc. — the @@ -710,6 +719,7 @@ fn member_access_receiver(node: Node, bytes: &[u8], aliases: &UseAliases) -> Cha var, base_type, relation, + from_call: false, }) } diff --git a/laravel-lsp/src/query_chain/extractor/tests.rs b/laravel-lsp/src/query_chain/extractor/tests.rs index 563b6a08..aaea7176 100644 --- a/laravel-lsp/src/query_chain/extractor/tests.rs +++ b/laravel-lsp/src/query_chain/extractor/tests.rs @@ -1379,6 +1379,7 @@ function run(User $user) { ChainReceiver::Eloquent(EloquentReceiver::RelationProperty { var, base_type, + from_call: _, relation, }) => { assert_eq!(var, "regs"); diff --git a/laravel-lsp/src/query_chain/flow.rs b/laravel-lsp/src/query_chain/flow.rs index 740964d1..8b52f893 100644 --- a/laravel-lsp/src/query_chain/flow.rs +++ b/laravel-lsp/src/query_chain/flow.rs @@ -60,7 +60,10 @@ use tree_sitter::Node; -use super::methods::{is_eloquent_static_starter, is_known_builder_method, COLLECTION_TERMINATORS}; +use super::chain::ChainEffect; +use super::methods::{ + chain_effect, is_eloquent_static_starter, is_known_builder_method, COLLECTION_TERMINATORS, +}; use super::use_aliases::{resolve_class_name, UseAliases}; use crate::salsa_impl::Confidence; @@ -120,7 +123,22 @@ pub fn resolve_expression( classify_rhs(node, bytes, aliases, 0, node.start_byte()) } -/// Detect an executed-relation-query assignment for `var_name` (issue #246): +/// Find the latest `$var_name = ` assignment in the use site's innermost +/// enclosing scope, strictly before the use site. One shared fetch: the +/// extractor runs this once and threads the result into both +/// [`resolve_collection_relation`] (the issue-#246 shape detection) and +/// [`resolve_with_assignment`] (the plain flow-typing fallback), so the +/// scope-subtree scan isn't done twice per bare-`$var` receiver. +pub fn latest_assignment_before<'tree>( + use_site: Node<'tree>, + bytes: &[u8], + var_name: &str, +) -> Option<(Node<'tree>, usize)> { + let scope = enclosing_scope(use_site)?; + latest_assignment_rhs(scope, bytes, var_name, use_site.start_byte()) +} + +/// Detect an executed-relation-query assignment (issue #246): /// /// ```php /// $registeredCompetitions = $user->competitions()->select('competitions.id')->get(); @@ -128,13 +146,17 @@ pub fn resolve_expression( /// ``` /// /// The variable holds a hydrated `Collection` of the *related* model -/// (`Competition`), not a builder on the root model (`User`). The shape is: -/// the latest assignment's RHS is a member-call chain whose deepest call is -/// NOT a recognised builder method (a potential relation hop), every call in -/// between IS a recognised builder method, and the outermost call is a -/// [`COLLECTION_TERMINATORS`] entry (`get`, `pluck`, …). The chain root is -/// either a variable resolvable to a model (via the normal flow walk) or an -/// Eloquent static starter (`User::query()`). +/// (`Competition`), not a builder on the root model (`User`). The caller +/// passes the already-fetched latest assignment (`rhs` + `assignment_start`, +/// from [`latest_assignment_before`]). The shape is: the RHS is a member-call +/// chain whose deepest call is NOT a recognised builder method (a potential +/// relation hop), every call in between is a recognised builder method that +/// *keeps the chain an Eloquent builder* ([`ChainEffect::None`] — a mid-chain +/// terminator or `toBase()` means the tail of the chain no longer types the +/// relation query, so the shape doesn't apply), and the outermost call is a +/// [`COLLECTION_TERMINATORS`] entry (`get`, `pluck`, …). The chain root is a +/// variable resolvable to a model (via the normal flow walk), an Eloquent +/// static starter (`User::query()`), or a construction (`(new User)`). /// /// Returns `(base_model_fqcn, relation_name)` so the extractor can emit a /// collection-mode receiver with the relation queued as a pending hop — the @@ -142,55 +164,64 @@ pub fn resolve_expression( /// deferred to the async finalize step. `None` means the assignment doesn't /// match the shape (callers fall back to plain flow typing). pub fn resolve_collection_relation( - use_site: Node, + rhs: Node, + assignment_start: usize, bytes: &[u8], - var_name: &str, aliases: &UseAliases, ) -> Option<(String, String)> { - let scope = enclosing_scope(use_site)?; - let (rhs, assignment_start) = - latest_assignment_rhs(scope, bytes, var_name, use_site.start_byte())?; let outer = unwrap_parens(rhs); if outer.kind() != "member_call_expression" { return None; } // Collect the chain's method names outermost → innermost, stopping at - // the first non-call node (the chain root). + // the first non-call node (the chain root). Parens along the way are + // syntactic wrapping, not chain structure — unwrap them so a + // `(new User)->competitions()->get()` root is still reached. let mut methods: Vec<&str> = Vec::new(); let mut node = outer; let root = loop { if node.kind() == "member_call_expression" { methods.push(node_text(node.child_by_field_name("name")?, bytes)?); - node = node.child_by_field_name("object")?; + node = unwrap_parens(node.child_by_field_name("object")?); } else { break node; } }; // Shape gate: a collection terminator on top, the relation candidate at - // the bottom, only recognised builder methods in between. Table-qualified - // column strings in the middle links are just arguments — they never - // affect this method-name walk. + // the bottom, and in between only recognised builder methods whose + // [`ChainEffect`] is `None` — calls that keep the chain a builder on the + // relation query. A mid-chain `get()` (FlipToCollection), `toBase()` + // (FlipToBase), or `first()` (Terminate) means everything after it + // operates on a collection / base builder / single model, not the + // relation query, so typing the variable to the related model would be + // wrong. Table-qualified column strings in the middle links are just + // arguments — they never affect this method-name walk. let (&terminator, rest) = methods.split_first()?; let (&relation, middle) = rest.split_last()?; if !COLLECTION_TERMINATORS.contains(&terminator) || is_known_builder_method(relation) - || !middle.iter().all(|m| is_known_builder_method(m)) + || !middle + .iter() + .all(|m| is_known_builder_method(m) && chain_effect(m) == ChainEffect::None) { return None; } // Type the chain root: `$base` via the normal flow walk (bounded at this - // assignment, so we don't re-find the assignment we're inside), or a - // static Eloquent starter (`User::query()->relation()->get()`). + // assignment, so we don't re-find the assignment we're inside), a static + // Eloquent starter (`User::query()->relation()->get()`), or a + // construction (`(new User)->relation()->get()` — parens already + // unwrapped by the walk above). let base = match root.kind() { "variable_name" => { let raw = node_text(root, bytes)?; let base_var = raw.trim_start_matches('$').to_string(); - resolve_with_boundary(root, assignment_start, bytes, &base_var, aliases, 0)?.0 + resolve_with_boundary(root, assignment_start, bytes, &base_var, aliases, 0, None)?.0 } "scoped_call_expression" => classify_scoped_call(root, bytes, aliases)?, + "object_creation_expression" => extract_new_class(root, bytes, aliases)?, _ => return None, }; Some((base, relation.to_string())) @@ -207,7 +238,33 @@ pub fn resolve_with_confidence( aliases: &UseAliases, ) -> Option<(String, Confidence)> { let before_byte = use_site.start_byte(); - resolve_with_boundary(use_site, before_byte, bytes, var_name, aliases, 0) + resolve_with_boundary(use_site, before_byte, bytes, var_name, aliases, 0, None) +} + +/// Like [`resolve`], but takes the innermost scope's latest-assignment lookup +/// pre-fetched by the caller (from [`latest_assignment_before`], including a +/// `None` "no assignment found" result), so the first scope iteration reuses +/// it instead of re-scanning the scope subtree. The extractor's bare-`$var` +/// receiver path fetches once and feeds both [`resolve_collection_relation`] +/// and this fallback. +pub fn resolve_with_assignment( + use_site: Node, + bytes: &[u8], + var_name: &str, + aliases: &UseAliases, + assignment: Option<(Node, usize)>, +) -> Option { + let before_byte = use_site.start_byte(); + resolve_with_boundary( + use_site, + before_byte, + bytes, + var_name, + aliases, + 0, + Some(assignment), + ) + .map(|(fqcn, _)| fqcn) } /// Like `resolve`, but the caller specifies the byte boundary explicitly. @@ -219,13 +276,19 @@ pub fn resolve_with_confidence( /// Because the boundary strictly decreases with each recursive call (or /// stays the same only when we walk into an outer scope, where it /// becomes the closure node's own start), cycles terminate naturally. -fn resolve_with_boundary( - use_site: Node, +/// `prefetched` carries the innermost scope's latest-assignment lookup when +/// the caller already ran it (`Some(result)`, consumed by the first loop +/// iteration only — outer-scope iterations and recursive calls always scan); +/// `None` means "not pre-fetched, compute here". +#[allow(clippy::too_many_arguments)] +fn resolve_with_boundary<'tree>( + use_site: Node<'tree>, mut before_byte: usize, bytes: &[u8], var_name: &str, aliases: &UseAliases, depth: usize, + mut prefetched: Option, usize)>>, ) -> Option<(String, Confidence)> { if depth > MAX_RECURSION_DEPTH { return None; @@ -242,9 +305,11 @@ fn resolve_with_boundary( // fall through to declared types (`typed_param` / `docblock`), // because those represent the user's stated intent — they're // not silently invalidated by an unrecognised reassignment. - if let Some((rhs, assignment_start)) = - latest_assignment_rhs(scope, bytes, var_name, before_byte) - { + let assignment = match prefetched.take() { + Some(pre) => pre, + None => latest_assignment_rhs(scope, bytes, var_name, before_byte), + }; + if let Some((rhs, assignment_start)) = assignment { if let Some(resolved) = classify_rhs(rhs, bytes, aliases, depth, assignment_start) { return Some(resolved); } @@ -434,7 +499,15 @@ fn classify_rhs( let inner = raw.trim_start_matches('$').to_string(); // Resolving another variable is an extra flow hop — the // deeper recursion carries the (lower) confidence. - resolve_with_boundary(root, boundary_before, bytes, &inner, aliases, depth + 1) + resolve_with_boundary( + root, + boundary_before, + bytes, + &inner, + aliases, + depth + 1, + None, + ) } "parenthesized_expression" => { // Unwrapping parens is syntactic, not a flow hop — keep the diff --git a/laravel-lsp/src/query_chain/flow/tests.rs b/laravel-lsp/src/query_chain/flow/tests.rs index 1838c34e..83260d42 100644 --- a/laravel-lsp/src/query_chain/flow/tests.rs +++ b/laravel-lsp/src/query_chain/flow/tests.rs @@ -563,7 +563,8 @@ fn resolve_collection_at(src: &str, var: &str, n: usize) -> Option<(String, Stri let bytes = wrapped.as_bytes(); let aliases = extract_use_aliases(&tree, &wrapped); let node = find_nth_var(&tree, bytes, var, n)?; - super::resolve_collection_relation(node, bytes, var, &aliases) + let (rhs, start) = super::latest_assignment_before(node, bytes, var)?; + super::resolve_collection_relation(rhs, start, bytes, &aliases) } #[test] @@ -664,6 +665,64 @@ function run(User $user) { assert_eq!(resolve_collection_at(src, "x", 1), None); } +#[test] +fn collection_relation_rejects_mid_chain_collection_terminator() { + // PR #266 review (round 3): `get()` mid-chain flips the tail to a + // Collection — `pluck('id')` then yields scalars, not related models. + // The gate must reject rather than type `$ids` to the related model. + let src = r#" +function run(User $user) { + $ids = $user->competitions()->get()->pluck('id'); + $ids->filter(); +} +"#; + assert_eq!(resolve_collection_at(src, "ids", 1), None); +} + +#[test] +fn collection_relation_rejects_mid_chain_mode_flip() { + // PR #266 review (round 3): `toBase()` mid-chain drops to the base + // builder — `get()` then yields raw stdClass rows, not hydrated related + // models. + let src = r#" +function run(User $user) { + $rows = $user->competitions()->toBase()->get(); + $rows->filter(); +} +"#; + assert_eq!(resolve_collection_at(src, "rows", 1), None); +} + +#[test] +fn collection_relation_rejects_mid_chain_single_model_terminator() { + // Same bug class: `first()` mid-chain terminates the query into a single + // model — the tail no longer types the relation query. + let src = r#" +function run(User $user) { + $x = $user->competitions()->first()->get(); + $x->filter(); +} +"#; + assert_eq!(resolve_collection_at(src, "x", 1), None); +} + +#[test] +fn collection_relation_detects_parenthesized_new_root() { + // `(new User)` as the chain root — parens are unwrapped during the root + // walk and the construction types the base model. + let src = r#" +use App\Models\User; +function run() { + $regs = (new User)->competitions()->get(); + $regs->pluck('id'); +} +"#; + assert_eq!( + resolve_collection_at(src, "regs", 1), + Some(("App\\Models\\User".to_string(), "competitions".to_string())) + ); +} + #[test] fn collection_relation_requires_resolvable_base() { // `$user` has no typed param / docblock / assignment — the base model is From b33ad01178b4b77cd5f2788f26e3ca1ec2fd1276 Mon Sep 17 00:00:00 2001 From: Mike Bronner Date: Wed, 15 Jul 2026 12:16:29 -0700 Subject: [PATCH 5/5] =?UTF-8?q?fix:=20=F0=9F=90=9B=20collect=20unrecognise?= =?UTF-8?q?d=20middle=20calls=20as=20heuristic=20hops=20in=20executed-rela?= =?UTF-8?q?tion=20collections?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resolve_collection_relation shape gate rejected any assignment chain with an unrecognised middle call, so idiomatic Laravel chains kept the original #246 false positive: pivot builder methods (->wherePivot(...), absent from the recognised-builder catalog), scopes on the related model (->competitions()->approved()->get()), and root scopes before the relation (->forCurrentTenant()->competitions()->get()). Mirror the inline cursor collector: unrecognised middles no longer reject — each becomes a RelationHopKind::Heuristic hop carried on the receiver (call_hops) and resolved in source order after the relation claim by the shared finalize step; a miss keeps the running model. The round-3 mode-flip gate is unchanged: a recognised middle whose ChainEffect isn't None (mid-chain get()/toBase()/first()) still rejects. Proactive round-4 hardening per human review direction on PR #266. --- laravel-lsp/src/query_chain/chain.rs | 10 +++ laravel-lsp/src/query_chain/cursor.rs | 8 ++ .../src/query_chain/diagnostics/tests.rs | 42 ++++++++++ laravel-lsp/src/query_chain/extractor.rs | 8 +- .../src/query_chain/extractor/tests.rs | 5 ++ laravel-lsp/src/query_chain/flow.rs | 57 +++++++++----- laravel-lsp/src/query_chain/flow/tests.rs | 77 ++++++++++++++++--- 7 files changed, 175 insertions(+), 32 deletions(-) diff --git a/laravel-lsp/src/query_chain/chain.rs b/laravel-lsp/src/query_chain/chain.rs index 56456973..2e1c2c54 100644 --- a/laravel-lsp/src/query_chain/chain.rs +++ b/laravel-lsp/src/query_chain/chain.rs @@ -109,12 +109,22 @@ pub enum EloquentReceiver { /// name that isn't a relation is an unknown collection (stay quiet), but /// a *called* name may still be a local scope returning a builder of the /// base model itself — see [`RelationHopKind::CallClaim`]. + /// + /// `call_hops` carries the assignment chain's *unrecognised middle + /// calls* (executed-relation form only; always empty for the property + /// access) in source order — scopes on the running model, pivot builder + /// methods we don't model, or further relation hops. The walker queues + /// each as a [`RelationHopKind::Heuristic`] hop after the `relation` + /// claim, mirroring how the inline cursor collector treats unrecognised + /// mid-chain calls. RelationProperty { var: String, base_type: Option, relation: String, #[serde(default)] from_call: bool, + #[serde(default)] + call_hops: Vec, }, } diff --git a/laravel-lsp/src/query_chain/cursor.rs b/laravel-lsp/src/query_chain/cursor.rs index 6c2566c7..19be4edb 100644 --- a/laravel-lsp/src/query_chain/cursor.rs +++ b/laravel-lsp/src/query_chain/cursor.rs @@ -440,6 +440,7 @@ fn pending_relation_hops_through( if let ChainReceiver::Eloquent(EloquentReceiver::RelationProperty { relation, from_call, + call_hops, .. }) = &chain.receiver { @@ -451,6 +452,13 @@ fn pending_relation_hops_through( RelationHopKind::Claim }, }); + // The executed-relation assignment's unrecognised middle calls + // (source order) — heuristic guesses, same as mid-chain unknowns + // collected below: a miss keeps the running model. + hops.extend(call_hops.iter().map(|name| RelationHop { + name: name.clone(), + kind: RelationHopKind::Heuristic, + })); } let mut running = mode; for link in chain.links.iter().take(up_to_idx) { diff --git a/laravel-lsp/src/query_chain/diagnostics/tests.rs b/laravel-lsp/src/query_chain/diagnostics/tests.rs index 261115ec..d5e9e27f 100644 --- a/laravel-lsp/src/query_chain/diagnostics/tests.rs +++ b/laravel-lsp/src/query_chain/diagnostics/tests.rs @@ -578,6 +578,48 @@ async fn executed_relation_collection_variable_from_static_root() { ); } +#[tokio::test] +async fn pivot_filtered_relation_collection_validates_against_related_table() { + // Round 4 (proactive): `wherePivot` isn't in the recognised-builder + // catalog — it must ride the heuristic-hop path (miss → keep the running + // model) instead of rejecting the shape, or the exact issue #246 false + // positive returns for pivot-filtered BelongsToMany chains. + let (_dir, root, db) = competitions_project().await; + let source = "competitions()->wherePivot('active', 1)->get();\n$regs->whereIn('type', ['league'])->pluck('id');\n"; + let chains = chains_of(source); + + let diags = chain_diagnostics(&chains, &db, &root, source, DiagnosticSeverity::WARNING).await; + assert!( + diags.is_empty(), + "pivot-filtered executed relation must validate against competitions: {diags:?}" + ); +} + +#[tokio::test] +async fn scoped_relation_collection_flags_typo_on_related_table() { + // Round 4 (proactive): a scope on the *related* model mid-chain + // (`->approved()` after `->competitions()`) is a heuristic hop — the + // miss keeps Competition as the running model, so a typo in the + // collection chain is still flagged against the competitions table (not + // silenced, and not validated against users). + let (_dir, root, db) = competitions_project().await; + let source = "competitions()->approved()->get();\n$regs->whereIn('typo', ['league'])->pluck('id');\n"; + let chains = chains_of(source); + + let diags = chain_diagnostics(&chains, &db, &root, source, DiagnosticSeverity::WARNING).await; + assert_eq!( + diags.len(), + 1, + "typo after a scoped relation chain still flags: {diags:?}" + ); + assert_eq!(code_of(&diags[0]), super::CODE_UNKNOWN_COLUMN); + assert!( + diags[0].message.contains("competitions"), + "diagnostic should name the competitions table; got: {}", + diags[0].message + ); +} + #[tokio::test] async fn local_scope_before_terminator_still_flags_typo_on_root_table() { // Regression (PR #266 review): `forCurrentTenant` is a local scope, not a diff --git a/laravel-lsp/src/query_chain/extractor.rs b/laravel-lsp/src/query_chain/extractor.rs index 9677a469..dbe125c3 100644 --- a/laravel-lsp/src/query_chain/extractor.rs +++ b/laravel-lsp/src/query_chain/extractor.rs @@ -634,8 +634,10 @@ fn member_chain_receiver(node: Node, bytes: &[u8], aliases: &UseAliases) -> Chai // the base model, with the relation queued as a pending hop for // the async finalize step. `from_call: true` — the claimed name // is a method call, so a finalize miss may still be a local - // scope (see [`RelationHopKind::CallClaim`]). - if let Some((base_type, relation)) = assignment.and_then(|(rhs, start)| { + // scope (see [`RelationHopKind::CallClaim`]). `call_hops` carries + // the chain's unrecognised middle calls (scopes, unmodeled + // builder methods, further relation hops) as heuristic hops. + if let Some((base_type, relation, call_hops)) = assignment.and_then(|(rhs, start)| { super::flow::resolve_collection_relation(rhs, start, bytes, aliases) }) { return ChainReceiver::Eloquent(EloquentReceiver::RelationProperty { @@ -643,6 +645,7 @@ fn member_chain_receiver(node: Node, bytes: &[u8], aliases: &UseAliases) -> Chai base_type: Some(base_type), relation, from_call: true, + call_hops, }); } // Phase 9: try to resolve `$var`'s declared class via either a @@ -720,6 +723,7 @@ fn member_access_receiver(node: Node, bytes: &[u8], aliases: &UseAliases) -> Cha base_type, relation, from_call: false, + call_hops: Vec::new(), }) } diff --git a/laravel-lsp/src/query_chain/extractor/tests.rs b/laravel-lsp/src/query_chain/extractor/tests.rs index aaea7176..477923fa 100644 --- a/laravel-lsp/src/query_chain/extractor/tests.rs +++ b/laravel-lsp/src/query_chain/extractor/tests.rs @@ -1381,10 +1381,15 @@ function run(User $user) { base_type, from_call: _, relation, + call_hops, }) => { assert_eq!(var, "regs"); assert_eq!(base_type.as_deref(), Some("App\\Models\\User")); assert_eq!(relation, "competitions"); + assert!( + call_hops.is_empty(), + "select() is a recognised builder method, not a heuristic hop" + ); } _ => unreachable!(), } diff --git a/laravel-lsp/src/query_chain/flow.rs b/laravel-lsp/src/query_chain/flow.rs index 8b52f893..9bc81285 100644 --- a/laravel-lsp/src/query_chain/flow.rs +++ b/laravel-lsp/src/query_chain/flow.rs @@ -150,25 +150,35 @@ pub fn latest_assignment_before<'tree>( /// passes the already-fetched latest assignment (`rhs` + `assignment_start`, /// from [`latest_assignment_before`]). The shape is: the RHS is a member-call /// chain whose deepest call is NOT a recognised builder method (a potential -/// relation hop), every call in between is a recognised builder method that -/// *keeps the chain an Eloquent builder* ([`ChainEffect::None`] — a mid-chain -/// terminator or `toBase()` means the tail of the chain no longer types the -/// relation query, so the shape doesn't apply), and the outermost call is a -/// [`COLLECTION_TERMINATORS`] entry (`get`, `pluck`, …). The chain root is a -/// variable resolvable to a model (via the normal flow walk), an Eloquent -/// static starter (`User::query()`), or a construction (`(new User)`). +/// relation hop), no call in between is a recognised builder method that +/// *changes the chain's mode* (a mid-chain terminator or `toBase()` means the +/// tail of the chain no longer types the relation query, so the shape doesn't +/// apply — only [`ChainEffect::None`] builder methods pass through), and the +/// outermost call is a [`COLLECTION_TERMINATORS`] entry (`get`, `pluck`, …). +/// The chain root is a variable resolvable to a model (via the normal flow +/// walk), an Eloquent static starter (`User::query()`), or a construction +/// (`(new User)`). /// -/// Returns `(base_model_fqcn, relation_name)` so the extractor can emit a -/// collection-mode receiver with the relation queued as a pending hop — the -/// relation→related-model resolution needs a model-file read, so it stays -/// deferred to the async finalize step. `None` means the assignment doesn't -/// match the shape (callers fall back to plain flow typing). +/// *Unrecognised* middle calls don't reject the shape — mirroring the inline +/// cursor collector ([`crate::query_chain::cursor`]), each is a heuristic +/// relation-hop candidate: a scope on the running model (`->approved()`), a +/// builder method we don't model (`->wherePivot(…)`), or a further relation +/// hop (`->organizer()`). They're returned (source order — innermost first) +/// so the finalize step resolves them after the receiver's claim, with a +/// miss keeping the running model unchanged. +/// +/// Returns `(base_model_fqcn, relation_name, heuristic_hops)` so the +/// extractor can emit a collection-mode receiver with the relation (and any +/// heuristic candidates) queued as pending hops — the relation→related-model +/// resolution needs a model-file read, so it stays deferred to the async +/// finalize step. `None` means the assignment doesn't match the shape +/// (callers fall back to plain flow typing). pub fn resolve_collection_relation( rhs: Node, assignment_start: usize, bytes: &[u8], aliases: &UseAliases, -) -> Option<(String, String)> { +) -> Option<(String, String, Vec)> { let outer = unwrap_parens(rhs); if outer.kind() != "member_call_expression" { return None; @@ -190,9 +200,8 @@ pub fn resolve_collection_relation( }; // Shape gate: a collection terminator on top, the relation candidate at - // the bottom, and in between only recognised builder methods whose - // [`ChainEffect`] is `None` — calls that keep the chain a builder on the - // relation query. A mid-chain `get()` (FlipToCollection), `toBase()` + // the bottom, and no *recognised* middle method whose [`ChainEffect`] + // isn't `None`. A mid-chain `get()` (FlipToCollection), `toBase()` // (FlipToBase), or `first()` (Terminate) means everything after it // operates on a collection / base builder / single model, not the // relation query, so typing the variable to the related model would be @@ -202,13 +211,23 @@ pub fn resolve_collection_relation( let (&relation, middle) = rest.split_last()?; if !COLLECTION_TERMINATORS.contains(&terminator) || is_known_builder_method(relation) - || !middle + || middle .iter() - .all(|m| is_known_builder_method(m) && chain_effect(m) == ChainEffect::None) + .any(|m| is_known_builder_method(m) && chain_effect(m) != ChainEffect::None) { return None; } + // Unrecognised middle calls are heuristic hop candidates (see the doc + // comment) — collected innermost-first so the finalize step applies them + // in source order after the receiver's relation claim. + let heuristic_hops: Vec = middle + .iter() + .rev() + .filter(|m| !is_known_builder_method(m)) + .map(|m| m.to_string()) + .collect(); + // Type the chain root: `$base` via the normal flow walk (bounded at this // assignment, so we don't re-find the assignment we're inside), a static // Eloquent starter (`User::query()->relation()->get()`), or a @@ -224,7 +243,7 @@ pub fn resolve_collection_relation( "object_creation_expression" => extract_new_class(root, bytes, aliases)?, _ => return None, }; - Some((base, relation.to_string())) + Some((base, relation.to_string(), heuristic_hops)) } /// Like [`resolve`], but also reports the [`Confidence`] tier of the diff --git a/laravel-lsp/src/query_chain/flow/tests.rs b/laravel-lsp/src/query_chain/flow/tests.rs index 83260d42..0e62fdc1 100644 --- a/laravel-lsp/src/query_chain/flow/tests.rs +++ b/laravel-lsp/src/query_chain/flow/tests.rs @@ -557,7 +557,7 @@ function search() { /// Run [`super::resolve_collection_relation`] against the Nth occurrence of /// `$var` in the snippet. -fn resolve_collection_at(src: &str, var: &str, n: usize) -> Option<(String, String)> { +fn resolve_collection_at(src: &str, var: &str, n: usize) -> Option<(String, String, Vec)> { let wrapped = format!(" Option<(String, Stri super::resolve_collection_relation(rhs, start, bytes, &aliases) } +/// `resolve_collection_at` with no heuristic middle hops expected — the +/// common assertion shape. +fn resolve_collection_simple(src: &str, var: &str, n: usize) -> Option<(String, String)> { + resolve_collection_at(src, var, n).map(|(base, relation, hops)| { + assert!(hops.is_empty(), "expected no heuristic hops, got {hops:?}"); + (base, relation) + }) +} + #[test] fn collection_relation_detects_instance_rooted_chain() { // The exact issue #246 shape: an executed relation query (with @@ -580,7 +589,7 @@ function run(User $user) { } "#; assert_eq!( - resolve_collection_at(src, "regs", 1), + resolve_collection_simple(src, "regs", 1), Some(("App\\Models\\User".to_string(), "competitions".to_string())) ); } @@ -594,7 +603,7 @@ function run() { } "#; assert_eq!( - resolve_collection_at(src, "regs", 1), + resolve_collection_simple(src, "regs", 1), Some(("User".to_string(), "competitions".to_string())) ); } @@ -608,7 +617,7 @@ function run(User $user) { } "#; assert_eq!( - resolve_collection_at(src, "ids", 1), + resolve_collection_simple(src, "ids", 1), Some(("User".to_string(), "competitions".to_string())) ); } @@ -652,17 +661,63 @@ function run(User $user) { } #[test] -fn collection_relation_rejects_unrecognised_middle_call() { - // A second unrecognised call mid-chain could be another relation hop — - // we only model a single hop, so stay out entirely (fall back to the - // plain flow path) rather than resolve to the wrong model. +fn collection_relation_collects_unrecognised_middle_calls_as_hops() { + // PR #266 round 4 (proactive): unrecognised middle calls no longer + // reject the shape — mirroring the inline cursor collector, each is a + // heuristic hop candidate (a scope on the running model, a pivot builder + // method we don't model, or a further relation hop), returned in source + // order after the relation claim. let src = r#" function run(User $user) { - $x = $user->competitions()->organizer()->get(); + $x = $user->competitions()->approved()->organizer()->get(); $x->pluck('id'); } "#; - assert_eq!(resolve_collection_at(src, "x", 1), None); + assert_eq!( + resolve_collection_at(src, "x", 1), + Some(( + "User".to_string(), + "competitions".to_string(), + vec!["approved".to_string(), "organizer".to_string()] + )) + ); +} + +#[test] +fn collection_relation_collects_pivot_method_as_hop() { + // BelongsToMany pivot builder methods (`wherePivot`, `withPivot`, …) + // aren't in the recognised-builder catalog — they must ride the + // heuristic-hop path rather than reject the shape, or the exact issue + // #246 false positive returns for pivot-filtered chains. + let src = r#" +use App\Models\User; +function run(User $user) { + $regs = $user->competitions()->wherePivot('active', 1)->get(); + $regs->whereIn('type', ['league']); +} +"#; + assert_eq!( + resolve_collection_at(src, "regs", 1), + Some(( + "App\\Models\\User".to_string(), + "competitions".to_string(), + vec!["wherePivot".to_string()] + )) + ); +} + +#[test] +fn collection_relation_rejects_mode_flip_with_unrecognised_middle_present() { + // The mode-flip gate (round 3) survives the heuristic-middle loosening: + // a mid-chain `toBase()` still rejects even when an unrecognised call + // also sits mid-chain. + let src = r#" +function run(User $user) { + $rows = $user->competitions()->approved()->toBase()->get(); + $rows->filter(); +} +"#; + assert_eq!(resolve_collection_at(src, "rows", 1), None); } #[test] @@ -718,7 +773,7 @@ function run() { } "#; assert_eq!( - resolve_collection_at(src, "regs", 1), + resolve_collection_simple(src, "regs", 1), Some(("App\\Models\\User".to_string(), "competitions".to_string())) ); }