Skip to content

Collapse ExpressionResolver member lookup to one helper - #478

Merged
Firehed merged 3 commits into
mainfrom
step/24
Sep 1, 2026
Merged

Collapse ExpressionResolver member lookup to one helper#478
Firehed merged 3 commits into
mainfrom
step/24

Conversation

@Firehed

@Firehed Firehed commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Retargets step-24 to the structural M×N only — the "late-binding on properties as it does for methods" clause was unsatisfiable (PHP forbids static as a property type), so the row now covers the four near-identical resolvers only.

Done

  • The four methods share one member-lookup helper — ExpressionResolver::resolveMember owns the receiver → ClassName dance (Name via ScopeFinder::resolveClassNameInContext, expression via resolve()->getType()->getResolvableClassNames()), and each caller passes a first-class kind-specific finder (findMethod(...) / findProperty(...)).
  • Adding a fifth member-access node kind is one call site, not four — a new resolver is one guard, one resolveMember call, and one small find* finder (or reuse of an existing one).
  • Existing hover, definition, completion, and signature-help suites remain green — composer test passes locally.

Tightening

Locked in the boundary the refactor establishes:

  • Folded docblockForExpression's method/property branches and resolveConstructor through resolveMember too, so the file's two private find* finders are the sole direct callers of MemberResolver::findMethod/findProperty.
  • Added a disallowedMethodCalls entry in phpstan.neon for MemberResolver::findMethod() and MemberResolver::findProperty(), allowed only in src/Resolution/ExpressionResolver.php (plus MemberResolver.php itself for its trait-alias self-calls, and tests). A future sibling resolve* that reaches for MemberResolver::findMethod directly instead of going through resolveMember now fails PHPStan.

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.35%. Comparing base (80e706c) to head (179800f).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #478      +/-   ##
============================================
- Coverage     99.40%   99.35%   -0.05%     
+ Complexity     1875     1873       -2     
============================================
  Files           132      132              
  Lines          4838     4805      -33     
============================================
- Hits           4809     4774      -35     
- Misses           29       31       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Route the docblock and constructor lookups through resolveMember so
the two private find* finders in ExpressionResolver are the sole
direct callers of MemberResolver::findMethod/findProperty, then add
a disallowedMethodCalls entry that locks that boundary in.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@Firehed
Firehed marked this pull request as ready for review September 1, 2026 18:48
@Firehed

Firehed commented Sep 1, 2026

Copy link
Copy Markdown
Owner Author

All changed lines covered, project coverage drop not relevant.

@Firehed
Firehed merged commit fd82926 into main Sep 1, 2026
8 of 9 checks passed
@Firehed
Firehed deleted the step/24 branch September 1, 2026 18:51
Firehed added a commit that referenced this pull request Sep 1, 2026
Follow-up to #473. Scope grew after a second scan surfaced five more M×N
patterns not captured by the current manifest, and after the realisation
that step-21's baseline hard-lock made new tighten rules impractical.

## Tightens to steps 25 and 26

- **step-25** — enumerates the nine node kinds that must gain docblock
coverage; notes that if `docblockForExpression` becomes a one-liner it
is deleted and inlined rather than kept as a wrapper.
- **step-26** — names `Domain\LateBindingKeyword` as the sole home
(drops the "or `ScopeFinder::resolveClassName`, whichever" hedge); adds
a `tests/Architecture/` test that fails if the three keyword literals
appear in `src/` outside the enum.

## New steps 27-31 for uncaptured M×N findings

Each ships a `disallowedMethodCalls` tighten modelled on #478's pattern.

- **step-27** — union/intersection receivers: every member-lookup site
in `ExpressionResolver` and `MemberAccessDetector` picks
`getResolvableClassNames()[0]` while
`SymbolResolver::getAccessibleMembers` iterates. Failure: `function
f(A|B $x) { $x->onlyB(); }` — completion offers `onlyB`, hover returns
null. Tighten pins `getResolvableClassNames()` to the shared helper.
- **step-28** — `resolveConstFetch` uses one lookup, `resolveFuncCall`
iterates `NameContext::candidates`. Failure: `namespace App; const X =
1; echo X;` — hover on `X` returns null. Tighten pins
`SymbolSource::lookupConstant` to `ExpressionResolver`.
- **step-29** — `SymbolCandidates` reads `->docblock` +
`DocblockParser::extractDescription` directly, bypassing
`HasSymbolLocation::getDocumentation()`. Preventive today; a future
tag-strip in `getDocumentation()` would silently miss completion detail.
Tighten pins `DocblockParser::extractDescription` to
`HasSymbolLocation`.
- **step-30** — `HoverHandler`, `SignatureHelpHandler`, and
`CompletionItemFactory` each compose `format()` + `getDocumentation()`
themselves; a `ResolvedSymbolPresenter` consolidates the shape. Tighten
pins the two methods to the presenter.
- **step-31** — `$this` typing has an AST path in `ExpressionResolver`
and a text-fallback side-channel in `MemberAccessDetector` (via a
`resolvedType` node attribute). Deletes the attribute; routes both
through one helper. Tighten pins
`TextFallbackHelper::resolveEnclosingClassName` to that helper.

## Relaxes bin/check-baseline-shrink NOW

Step-21's script failed if either baseline file existed. That made new
tighten rules with residuals impossible to add. Restored the pre-step-21
shrink-only-when-present logic (comparing to `origin/main`). The hard
"must-not-exist" lock moves to the retire step, by which point every
interim tighten will have drained its residual.

## Beefed-up retire (step-32)

- Restores the hard lock on `bin/check-baseline-shrink`.
- Adds a `tests/Architecture/HandlerDependenciesRule.php` that fails if
any handler in `src/Handler/` names `ParserService`, `SymbolIndex`,
`MemberResolver`, or `SymbolSource` directly — code-enforcing the
"handlers are formatters, not resolvers" invariant that CLAUDE.md
documents but nothing enforces today.
- Updates `CLAUDE.md` Guardrails and Architecture Invariants to reflect
final state: baselines permanently gone, freeze paragraph deleted, the
running list of scoped `disallowedMethodCalls` tightens named as the
seams they pin.

## Considered and dropped from the scan

- Two regex classifiers for call kind
(`TextFallbackHelper::parseCallPattern` vs `CompletionClassifier`) —
verifier confirmed they answer different questions on the same input; no
feature asymmetry possible.
- Two text regex walkers (`DefaultTextSymbolExtractor` vs
`TextFallbackHelper::findClassLikeDeclarations`) —
`DefaultTextSymbolExtractor` is a consumer of the second; already
unified.

_PR body written by AI, reviewed by a human._
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.

1 participant