Report a private or protected member reached from outside its scope

[petrovo-as]

Note

I am not a Rust developer. This came up while using PHPantom on my own PHP
CMS, and I fixed it with AI assistance — written with Claude Opus 5, reviewed
by GPT-5.6 Codex, which sent the design back three times before this shape
survived. I would be glad if you read through the proposed changes, even if
you end up not using the code.

Adds invalid_member_access (Error): a private or protected property,
method, class constant, or static property reached from a scope that cannot see
it. This and unreachable code were the only two checks I found missing against
Intelephense in day-to-day use.

The problem

member_exists() matched on name alone. Visibility is already on
MethodInfo, PropertyInfo, and ConstantInfo, and completion already filters
on it, but no diagnostic read it.

A parent's private member was worse than unreported. The merge drops it, since
PHP does not inherit private members, so this was reported as unknown_member:

class Base { private string $secret = 'x'; }
class Child extends Base {
    public function run(): void { echo $this->secret; }  // was: not found
}

The change

Runs inside the unknown-member walk, which has already resolved the subject to a
class. No second resolution pass, no second walk over the symbol map.

Visibility is read from the assembled class. Trait adaptations are therefore
not re-derived — use T { run as private; }, aliases, and insteadof are
already applied by inheritance/traits.rs.

The declaring class is computed (declaring_class()), because the assembled
class does not carry it: merge_traits_into() receives the host FQN and drops
it. inheritance/mod.rs merges an ancestor's traits into the descendant, so a
private member on a merged class may have been declared in a trait a parent
uses. Consequences:

• protected on a shared parent — reachable from every branch below it;
• protected declared on a sibling — not reachable;
• private in a parent — reported as an access violation, not a missing member.

The walk runs only for a non-public member reached from inside some class.
Public members and everything reached from outside every class are settled by
the merged lookup the surrounding pass already performs.

Suppressed: a magic handler on the class or inherited from a parent; a trait
body (host class unknown); @see tags; a union with one permitting branch; an
ancestor the loader cannot produce; a provenance walk that finds nothing.

$this, self, and static name the class the code is bound to, which
differs from the enclosing class for a Macroable callback or any
Closure::bind, so those three add the receiver as a scope.
self_and_static_in_macro_closure_resolve_to_macro_target fails without this.
parent:: does not count — it names the class to look the member up in, not the
scope doing the looking.

What the review rejected

1. Walk the raw hierarchy for everything, read visibility off the raw
   declaration. Ignores trait adaptations: run as private invisible,
   insteadof can pick the losing declaration. False positive.
2. A private member on a merged class belongs to that class, no walk needed.
   Disproved by inheritance/mod.rs merging a parent's traits into the child.
3. Check before the merge, on whatever classes the resolver had. A class
   inheriting __get while declaring a private property was flagged for an
   access PHP dispatches to the handler. The pre-merge shortcut now confirms
   only public members.

(1) and (2) are the same mistake in opposite directions: inferring provenance
that is not recorded anywhere. Hence declaring_class(), and hence D22.

Also in here

examples/php/inlay_hints.php used fn($u) => $u->name, where $name is
protected on Model and the closure lives in a class that does not extend it
— a runtime fatal, unnoticed because the method returns an empty array and the
closure is never called. Switched to the public accessor. The new check found
it.

Not included

Backlog entries this PR adds. All are missed reports, not wrong ones:

• D19 — the span does not record read vs. write vs. unset, so the exact
  magic handler cannot be required; any of the four suppresses.
• D20 — extraction strips the $ from Foo::$bar; a static property and a
  same-named constant are indistinguishable downstream.
• D21 — Known|Other with Known::$x private and no $x on Other is
  reported by neither check.
• D22 — provenance recomputed rather than recorded; the recomputation
  handles neither trait aliases nor nested traits.
• D23 — a rebound closure's scope is added to the lexical one rather than
  replacing it, though the resolver already knows the bound class.

D22 is the one I would like your read on. Recording the declaring class
during the merge deletes this check's provenance walk and closes D23, but adds a
field to every member across the index — worth measuring against current memory
first. Your call; happy to open an issue instead.

Testing

cargo test 7120 passed / 0 failed, cargo clippy --all-targets -- -D warnings
and cargo fmt --check clean. php -l clean, runDemoAssertions() passes,
phpantom_lsp analyze on examples/laravel still reports exactly three errors
— the new check fires nowhere in that project.

37 tests in tests/integration/diagnostics_member_visibility.rs. The sibling
pair is the one to look at: protected on a shared parent must stay reachable
from a cousin, protected on the sibling itself must not. They pass together only
if provenance is right; either alone passes against broken code.

examples/php/diagnostics.php gains MemberVisibilityDemo, with reflection
assertions in scaffolding/assertions.php.

Criterion, diagnostics group, base vs. head:

fixture	base	head	change
lots_of_missing_methods (~1175 lines, all unresolved members)	102.22 ms	103.48 ms	+1.23 %, within noise threshold
method_chain	1.6712 ms	1.6860 ms	+1.51 %, within noise threshold

lots_of_missing_methods is the worst case — every member in it takes the
lengthened path.

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, examples/)
• I have updated the config schema (config-schema.json)
• I have added/updated tests to cover my changes
• I fully understand the code that I am submitting (what it does,
  how it works, how it's organized), including any code drafted by an LLM.
• For any prose generated by an LLM, I have proof-read and copy-edited with
  an eye towards deleting anything that is irrelevant, clarifying anything
  that is confusing, and adding details that are relevant. This includes,
  for example, commit descriptions, PR descriptions, and code comments.

🤖 Generated with Claude Code

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 97.61388% with 11 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/diagnostics/member_visibility.rs	97.16%	10 Missing ⚠️
src/diagnostics/unknown_members/mod.rs	99.07%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[petrovo-as]

Thanks for the benchmark job — it made me go and measure properly, and I think
the picture is a bit different from what the alert says. Numbers below so you
can judge for yourself.

First, a small thing: the step also failed at the end trying to post its
comment, with 403 Resource not accessible by integration. I think that is
just a PR from a fork not having pull_requests=write, rather than anything to
do with the timings.

The alert does not reproduce here. Same two commits, criterion, my machine:

benchmark	7d8648c	05e63a8	change	criterion's verdict
completion_cross_file_type_hint	26.32 µs	26.87 µs	+2.2 %	no change detected (p = 0.06)
completion_short_file	28.98 µs	29.28 µs	+3.1 %	regressed (p = 0.00)

CI reported 1.46× and 1.32×. It also measured the baseline at 0.052 ms and
0.056 ms where I get 26 µs and 29 µs, so that runner is roughly half the speed,
and the ±0.009 it reports against a baseline ±0.003 probably says the rest.

I do not want to wave the remaining 3 % away, so: completion never runs any of
the new code. Everything this PR adds is reached only from
check_member_on_resolved_classes, and nothing outside src/diagnostics/
calls that, inaccessible_member_message, or member_is_public. My best
explanation for what is left is a bigger binary laying out differently, which
any addition to the crate pays on a benchmark this short.

There is one real cost, though, and I should have put it in the description.
The pre-merge shortcut used to return as soon as the member existed; it now
returns only for a public member, because a non-public one cannot be judged
before the merge. So $this->somePrivateProp inside its own class — very
common — now falls through to resolve_class_fully_cached() instead of
short-circuiting. That is a cache lookup rather than a fresh merge, but it is
more work than before, and the fixture I measured does not exercise it: the
members in lots_of_missing_methods do not exist at all. For the paths it does
cover:

fixture	7d8648c	05e63a8	change
lots_of_missing_methods (~1175 lines, all unresolved members)	102.22 ms	103.48 ms	+1.23 %, within noise threshold
method_chain	1.6712 ms	1.6860 ms	+1.51 %, within noise threshold

If the 1.30 threshold on microsecond benchmarks is something you want held, I am
happy to dig further — but the next step would mean reaching into parts of the
engine outside this change, which I deliberately stayed out of, so that is
really your call rather than mine. A fixture full of $this->private… accesses
would also be worth having either way, since nothing measures that shape today.

🤖 Generated with Claude Code