Skip to content

Query-chain relation-collection detection: nullsafe (?->) and $this-rooted chains - #273

Merged
mikebronner merged 2 commits into
mainfrom
feature/272-query-chain-relation-collection-detection-doesnt-c
Jul 17, 2026
Merged

mikebronner merged 2 commits into
mainfrom
feature/272-query-chain-relation-collection-detection-doesnt-c

Conversation

@mikebronner

@mikebronner mikebronner commented Jul 17, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Implements #272 — extends the query-chain executed-relation-collection detection (issue #246's shape) to nullsafe (?->) and $this-rooted chains, and pins the root-scope-before-relation composition with a dedicated end-to-end test.

Changes

  • flow.rs — resolve_collection_relation now accepts nullsafe_member_call_expression links as chain structure (mixed ->/?-> chains included), resolves a bare-$this root via the enclosing class's FQCN, and handles a $this->prop root by making the property the relation claim (the deepest call rides the heuristic-hop queue). Returns a new CollectionRelation struct carrying a from_call flag, since the property claim must seed RelationHopKind::Claim (miss → clear) rather than CallClaim (miss → consult scopes).
  • extractor.rs — the bare-$var receiver path passes the struct's from_call through instead of hardcoding true.
  • member_resolver.rs — enclosing_class_fqcn promoted to pub(crate) for reuse by the flow walk.
  • flow/tests.rs — 8 new unit tests (nullsafe, mixed-operator, nullsafe mode-flip gate, $this call root, $this->prop root, $this outside a class, $this builder-call gate, non-$this property root); existing helper projects the new struct so all pre-existing assertions run unchanged.
  • diagnostics/tests.rs — 8 new end-to-end tests: paired positive/typo for the nullsafe, $this-rooted, and $this->prop-rooted shapes, plus the paired scope-before-relation pin.

Acceptance Criteria

  • $var = $user?->competitions()?->get() (nullsafe) detected as an executed-relation collection; downstream chain validates against the related table — paired positive/typo tests (nullsafe_relation_collection_*).
  • $var = $this->competitions()->get() and $var = $this->user->competitions()->get() detected likewise, relation resolved against the enclosing class's model — paired positive/typo tests (this_rooted_*, this_property_rooted_*).
  • Existing bare-$var, static-root, (new X)-root detection and the RelationProperty property-access path do not regress — full suite passes unchanged (2790 tests).
  • Dedicated end-to-end regression test for the root-scope-before-relation shape, mirroring scoped_relation_collection_flags_typo_on_related_table with a paired positive/typo pair (root_scope_before_relation_collection_*).

Test Plan

  • All existing tests pass (cargo test: 2241 + 469 + 80, 0 failures)
  • New tests cover the changes (16 added: 8 unit, 8 end-to-end)
  • cargo clippy --all-targets clean, cargo fmt applied

Fixes #272

Extends resolve_collection_relation (issue #246's shape detection) to two
idiomatic chain shapes it previously skipped:

- Nullsafe (?->) links are treated as chain structure like plain ->, so
  $var = $user?->competitions()?->get() types to the related collection.
- $this-rooted chains resolve against the enclosing class's model:
  $this->competitions()->get() (call claim) and
  $this->user->competitions()->get() (the property is the relation claim,
  from_call: false; the deepest call rides the heuristic-hop queue).

The detection now returns a CollectionRelation struct carrying a from_call
flag, since the $this->prop root seeds a property Claim whose finalize
miss must clear rather than consult scopes. enclosing_class_fqcn is made
pub(crate) for the flow walk. Adds paired positive/typo regression tests
for both shapes plus a dedicated end-to-end pin for the
root-scope-before-relation composition
($user->forCurrentTenant()->competitions()->get()).

Fixes #272
@mikebronner
mikebronner marked this pull request as ready for review July 17, 2026 10:56

@mr-sherlock-holmes mr-sherlock-holmes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved

Review Summary

  • Extends query-chain executed-relation-collection detection (issue #246's shape) to nullsafe (?->) chains and $this-rooted / $this->prop-rooted chains, plus a dedicated end-to-end pin for the root-scope-before-relation composition.
  • Every acceptance criterion is met:
    • AC #1 (nullsafe): is_member_call (flow.rs) matches both member_call_expression and nullsafe_member_call_expression, so $user?->competitions()?->get() walks identically to the plain--> form. Paired nullsafe_relation_collection_validates_against_related_table / _flags_typo_on_related_table assert against the related table. ✅
    • AC #2 ($this-rooted): the root-kind match resolves bare-$this via enclosing_class_fqcn (call claim, from_call: true) and $this->prop via the property claim (from_call: false); paired positive/typo tests for both this_rooted_* and this_property_rooted_*. ✅
    • AC #3 (no regression): member_access_receiver / RelationProperty path is byte-identical to main; the pre-existing shapes still return from_call: true. Full suite passes unchanged (2790 tests). ✅
    • AC #4 (scope-before-relation pin): root_scope_before_relation_collection_* uses $user->forCurrentTenant()->competitions()->get() and genuinely mirrors scoped_relation_collection_flags_typo_on_related_table, adding the positive counterpart the mirrored test lacked. ✅
  • Tests verified meaningful, not tautological: each typo test would legitimately produce zero diagnostics (not one) without the fix, because pre-fix resolve_collection_relation/classify_rhs strictly matched only member_call_expression and had no $this special-case — so the assert_eq!(diags.len(), 1) genuinely fails without the change. Message assertions substitute the resolved table name, so they can only pass if detection actually rerouted the type.
  • The Claim vs CallClaim asymmetry is correct-by-design: the $this->prop property claim seeds RelationHopKind::Claim (miss → clear effective_model, no false positive), while call claims seed CallClaim (miss → consult local scopes). Each branch's is_known_builder_method gating matches its miss-semantics — the property branch safely omits the gate the call branch requires. Documented in the enum and the PR body.
  • Security: clean — iterative chain walk (not recursion), no new unwraps/panics/unsafe, enclosing_class_fqcn → pub(crate) is a benign in-crate visibility promotion.
  • CI: all checks green (LSP test/fmt/clippy, extension wasm/fmt/clippy).

Nicely scoped work: the new CollectionRelation struct threads the from_call distinction cleanly instead of hardcoding it, the doc-comments carry the reasoning, and the 16 tests (8 unit + 8 e2e) pin every new shape with paired positive/negative coverage plus the deliberate negative-shape gates ($this outside a class, $this builder-call, non-$this property root).

📋 Non-blocking follow-ups

  • None.

Ready for @mikebronner to merge.

@mikebronner
mikebronner merged commit 9eef6b9 into main Jul 17, 2026
5 checks passed
@mikebronner
mikebronner deleted the feature/272-query-chain-relation-collection-detection-doesnt-c branch July 17, 2026 14:11
mikebronner added a commit that referenced this pull request Jul 30, 2026
Merging origin/main brought in PR #273's new tests for relation-collection
diagnostics, written against the pre-rebrand API (code_of()/CODE_UNKNOWN_COLUMN,
both removed in e7ca2f7). Git auto-merged diagnostics/tests.rs without a
textual conflict — the new tests landed on different lines than anything
this branch touched — but the result didn't compile.

Nine assertions updated to the pattern this branch already established
for every other test in the file: assert on data_kind_of(), the field
the code-action handler actually branches on, not the removed code
field. All nine were confirmed to be column-kind diagnostics before the
swap, matching the existing "column" assertions around them.
mikebronner added a commit that referenced this pull request Jul 31, 2026
* chore: 🚚 Rebrand user-facing name to "Laravel (Community Edition)"

Laravel now ships an official Zed extension of its own
(zed-industries/extensions#6996). The agreed resolution with the Laravel
and Zed teams is two coexisting marketplace listings, so this extension
needs display text that identifies it as the community-maintained option.

Two-tier naming: long form "Laravel (Community Edition)" where there is
room to be unambiguous (marketplace name/description, README title,
CONTRIBUTING title), short form "Laravel CE" in Zed's compact
language-server slot.

Also adds the README comparison section promised on that thread: a
neutral architecture and feature-by-feature contrast between the static
tree-sitter approach here and the official server's boot-the-app
approach, carrying the same "snapshot as of, corrections welcome"
framing as the adjacent editors table. Neither approach is presented as
a superset of the other.

* chore: 💬 Brand LSP runtime output as "Laravel CE" / "laravel-ce"

Every string where the extension refers to itself now carries the
brand, so its output stays attributable when Laravel's official
extension is installed alongside.

Prose labels (Title Case "Laravel CE"): the three indexing/rename
progress titles, the reindex code-action label, four database-
connection toasts, and the WASM crate's two binary-provisioning
errors. The progress title was duplicated across three call sites;
it's now a single PROGRESS_TITLE constant in indexing_progress.

Diagnostic attribution (lowercase "laravel-ce"): every Diagnostic this
server publishes now carries a shared laravel_lsp::DIAGNOSTIC_SOURCE,
since Zed renders a diagnostic's source directly in its hover/problems
UI, the same way it shows "(eslint)". Lowercase because that's how
every other server's tag reads, distinct from the Title Case prose
labels above.

The four query-chain diagnostics (unknown column/relation/table,
ambiguous column) also carry a code; those are now bare kind names
with no brand prefix ("unknown-table", not "laravel-ce.unknown-table")
since Zed concatenates source and code without deduping — a repeated
prefix on both was redundant.

Unifying `source` into one brand string across every diagnostic
removed it as a usable discriminator between query-chain diagnostics
and path-based ones, which the code-action handler relied on. Extracted
is_chain_diagnostic() as the single source of truth: a chain diagnostic
is now identified by its structured `data` payload, which only those
four diagnostics carry.

Strings naming the PHP framework or the user's own app are deliberately
untouched: the "Laravel documentation" hover link, the "Laravel
dependencies not installed" diagnostic, the "Laravel config not
available" rename errors, and the source: "laravel" completion
provenance tags. Renaming those would misname the user's own framework
or falsely claim authorship of Laravel's own validation rules.

Tests added/updated at every touched call site, each mutation-checked
(revert the literal, confirm red, restore, confirm green) — including
one gap the mutation pass caught on its own: three of the four
diagnostic codes were only ever compared against their own constant,
which passes trivially through any relabelling.

* chore: ⚰️ Drop the unread code field from chain diagnostics

Zed renders a diagnostic's source and code concatenated, so the four
query-chain diagnostics showed as "(laravel-ce unknown-table)" in every
tooltip. Nothing in the server reads Diagnostic.code: the rename /
qualify / create-migration quick-fixes are routed by
is_chain_diagnostic(), which keys off source plus the structured data
payload, and there's no codeDescription URL or per-code suppression
setting behind it either. The field was decorative.

So the four CODE_* constants go with it, and these diagnostics now set
only source — matching the other ~29 diagnostics this server publishes.
The asymmetry was incidental history, not a design choice. This
supersedes the "those are now bare kind names" paragraph in a8f5657,
which kept the codes on the assumption they earned their place. If an
error knowledgebase is ever built (codeDescription links, a docs page
per diagnostic kind), codes come back then — with something reading them.

Every .code assertion is replaced by one on data.kind, the field the
code-action handler actually branches on, so the swap pins a live
contract where the old assertions pinned a dead one. It discriminates:
flipping make_diagnostic's Column arm to "relation" reddens five tests,
and renaming the ambiguous-column kind reddens its own. The brand-prefix
test is replaced by no_chain_diagnostic_carries_a_code, which drives one
source through each of the three construction sites and asserts code is
None; re-adding a code at any of them was mutation-checked red, one site
at a time. The code_actions fixture helper drops its code argument so the
fixture matches what the producer now builds.

* fix: 🔀 Repair silent semantic conflict from merging origin/main

Merging origin/main brought in PR #273's new tests for relation-collection
diagnostics, written against the pre-rebrand API (code_of()/CODE_UNKNOWN_COLUMN,
both removed in e7ca2f7). Git auto-merged diagnostics/tests.rs without a
textual conflict — the new tests landed on different lines than anything
this branch touched — but the result didn't compile.

Nine assertions updated to the pattern this branch already established
for every other test in the file: assert on data_kind_of(), the field
the code-action handler actually branches on, not the removed code
field. All nine were confirmed to be column-kind diagnostics before the
swap, matching the existing "column" assertions around them.

* docs: 📝 Slim the README into a scannable entry point

The README had regrown to 336 lines — a dense wall that buried the parts
readers actually need. Move the three largest blocks into docs/, mirroring
what #33 already did for per-feature detail:

- docs/configuration.md — the zero-config intro, the language_servers
  callout, the full JSONC settings reference, database connection setup,
  and the outline-panel requirements
- docs/troubleshooting.md — the five-step decision tree and the manual
  binary fallback
- docs/comparison.md — "Laravel across editors" plus the Community
  Edition vs. official extension tables

Each moved section leaves behind a teaser carrying its most load-bearing
fact — the Configuration teaser keeps the language_servers override gotcha
inline, since that is the #1 real-world support issue.

Adds a Contents table under the badges, and docs/README.md as an index for
the folder's 14 guides, grouped by task (features, configuration, choosing
this extension, help, design notes) rather than alphabetically.

Cross-links swept repo-wide: the five docs/*.md links pointing at README
anchors inside moved sections now target docs/configuration.md, anchored to
the right subsection. Subheadings in the new files drop their emoji to match
the existing docs/ convention, which also gives them stable anchors.

README: 336 -> 127 lines (3241 -> 1162 words). No content dropped — every
moved paragraph, table, and code block is preserved verbatim.

* chore: 📃 Updated documentation.

* fix: 🩹 Route chain quick-fixes on payload kind, not payload presence

is_chain_diagnostic() discriminates query-chain diagnostics from
path-based ones for main.rs's code_action handler, whose chain arm
unconditionally continues. It keyed on source + data.is_some(), which
is correct only because nothing else on this server attaches data to a
diagnostic. The day something does — for its own unrelated reason — it
would be swallowed into the chain arm and silently lose its own
create-view / create-component fixes: no compile error, no test
failure. That is the same failure class this branch already hit twice,
once when unifying source into one brand string and once when dropping
the code field.

The payload half is now matched positively against CHAIN_DIAG_KINDS —
the four kind values the constructors actually stamp — so an
unrecognised payload falls through to the path-based arm instead. The
whitelist lives beside the constructors in diagnostics.rs, since that
is where a new kind would be added.

Four tests, each mutation-checked. Reverting to data.is_some() reddens
the two foreign/kindless-payload cases; dropping a kind from the
whitelist reddens every_chain_diagnostic_is_recognised_by_the_gate,
which drives real PHP through all three construction sites and asserts
each product still passes; hardcoding the gate to one kind reddens both
that and every_producer_kind_is_routed_to_chain_actions.

Docs, same round: the decision table Mike added to the README was
duplicated almost verbatim in docs/comparison.md, along with the
"don't run both" callout. comparison.md now back-references the
README's copy and keeps only what is not duplicated — the data-gathering
paragraph, the static-resolution boundary, and the three detail tables.
docs/README.md's index line no longer advertises the trimmed content,
and PHPStorm is spelled PhpStorm.

* chore: 🚚 Rename cache dir, release binary, and PATH lookup to laravel-ce-lsp

Three of the four internal uses of "laravel-lsp" were sharing that literal
by coincidence rather than necessity. Split them onto the rebranded name,
each with a compatibility path so no existing user notices.

Cache directory — ProjectDirs' app name becomes "laravel-ce-lsp". All five
call sites (pattern, magic, command, and vendor-alias caches plus
CacheManager) now funnel through a new shared cache_root(), which runs a
one-shot migration behind a OnceLock: when the new directory is absent and
the pre-rebrand one exists, the whole tree is renamed across, so upgrading
users keep their warm index instead of paying for a cold re-scan. Both
directories present prefers the new one and leaves the old untouched
rather than attempting a destructive merge; a failed move is logged and
falls back to an empty cache, because one re-index beats a server that
won't start.

Release binary — the published asset names become laravel-ce-lsp-*, in
lockstep across release.yml's matrix, the archive contents, and the name
platform_binary_name expects after extraction. The Cargo package name
stays "laravel-lsp", so the local dev path in .zed/settings.json is
unaffected. Download URLs are version-pinned, so released versions keep
resolving their own assets.

PATH fallback — the manual-install lookup checks laravel-ce-lsp first and
falls back to laravel-lsp, keeping binaries placed per the previously
published troubleshooting doc working. The doc now describes only the new
name.

The Zed-facing language-server id is deliberately untouched: it is what
existing settings.json files key off, and renaming it would break explicit
language_servers lists.

* chore: 🔥 Remove build.sh and install.sh

Unreferenced from README, CONTRIBUTING.md, CLAUDE.md, docs/, and CI —
old local convenience scripts (cargo build + copy-to-PATH) that predate
the documented dev workflow (cargo build --release + "install dev
extension") and the "Manual binary fallback" section in
docs/troubleshooting.md, which already covers the same scenario the
scripts automated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Query-chain relation-collection detection doesn't cover nullsafe (?->) or $this-rooted chains

1 participant