Avoid warning about non-nullable class-property in traits.

[peter17]

Credit: made with the help of Opus 5.
Fixes phpstan/phpstan#14416
This supersedes #5479

[peter17]

Note: the failing tests seem unrelated to those changes

[peter17]

@ondrejmirtes any opinion about this? I will have time in the upcoming days to make adjustments if needed. Thanks! Regards

[peter17]

@SanderMuller any opinion about this PR? I will have time in the upcoming days to make adjustments if needed. Thanks in advance! Best regards

[SanderMuller]

I'm not a maintainer, so this isn't an approval — but you asked, and the PR deserved a proper look, so I checked it out and ran it. Hopefully this makes the decision cheap for @ondrejmirtes.

It does what it says. I ran the issue reproducer as a real analysis on 6cdbaa708 (level 9, trait and classes in separate files so the cross-file collector path is actually exercised):

// MyTrait.php
trait MyTrait {
	public function a(): bool { return isset($this->i); }
	public function b(): bool { return empty($this->i); }
	public function c(): int  { return $this->i ?? -1; }
}
// MyClass.php   class MyClass  { use MyTrait; public int $i = 10; }
// MyClass2.php  class MyClass2 { use MyTrait; }

Same input, PR's base commit (8e2efc0ce) vs the PR head:

# before
MyTrait.php (in context of class App\MyClass)
  4   Property App\MyClass::$i (int) in isset() is not nullable.
  6   Property App\MyClass::$i (int) on left side of ?? is not nullable.
[ERROR] Found 2 errors

# after
[OK] No errors

So #14416 is fixed. (empty($this->i) reports nothing either way here — int can be 0, so it is legitimately falsy-able; the empty() path is covered by the fixture instead.)

It doesn't over-suppress. Control case — same shape, but every using class declares the property non-nullable:

trait AllSetTrait { public function m(): bool { return isset($this->k); } }
class A1 { use AllSetTrait; public int $k = 1; }
class A2 { use AllSetTrait; public int $k = 2; }

→ still reported, once per context:

AllSet.php (in context of class App\A1)   Property App\A1::$k (int) in isset() is not nullable.
AllSet.php (in context of class App\A2)   Property App\A2::$k (int) in isset() is not nullable.

which is the behaviour you'd want, and matches the SamePropertyTypeTrait case in the new fixture.

Tests. The three touched suites pass (96 tests), and tests/PHPStan/Rules/Variables + tests/PHPStan/Rules/Comparison are green together (661 tests) locally.

The two red CI jobs are unrelated — I read the logs rather than assuming:

• Tests with old PHPUnit (7.4): IntersectionTypeTest::testIsAcceptedBy data set DOMNode child and sibling accessor properties should be nullable #7, non-empty-array&hasOffsetValue(0, int) -> isAcceptedBy(array{int, int}), expected Maybe got No. Type system, unrelated.
• Turbo Extension (macos-latest, 8.5, make phpstan): PHPStan process crashed because it reached configured PHP memory limit: 450M. Runner memory, not a rule failure.

So your "seem unrelated" note holds on both counts.

Re: the #5479 feedback — the tests now use CompositeRule + ConstantConditionInTraitRule and include both should-report and should-not-report cases, matching how the 18 existing tests/PHPStan/Rules/Comparison/* tests from #5309 are written (down to the same // @phpstan-ignore argument.type).

Two things worth an explicit decision from @ondrejmirtes — design calls, not defects:

1. $value is hardcoded true. In the Reduce false positives about constant conditions in traits #5309 rules the 4th argument is the real constant value ($leftType->getValue() etc.), so contexts disagreeing true-vs-false collapse to "report nothing". Here every error emits true, so the discriminator is effectively error vs. no error — which the added comments do call out. I tried to break it and couldn't; with one user having the offset and one not:

   trait T { public function f(): void { var_dump(isset($this->arr['k'])); } }
   class HasOffset   { use T; /** @var array{k: int} */ public array $arr = ['k' => 1]; }
   class LacksOffset { use T; /** @var array{} */      public array $arr = []; }

   both contexts error and both are reported in context — identical to what the PR's base commit already does with the same fixture. So it looks sound to me, but it is a deliberate divergence from the helper's original contract, so probably worth an explicit ok rather than leaving it implicit.

2. Where the logic lives. The hint on the issue was "could probably be done in a single place in IssetCheck"; this version instead repeats the same ~8-line emit/suppress block in each of the three rules. For what it's worth that mirrors Reduce false positives about constant conditions in traits #5309's own rules (policy in the rule, not in the shared check) and keeps IssetCheck free of trait policy. If you'd prefer it centralised, IssetCheck::check() is invoked only from these three rules (the other IssetCheck mentions under src/Analyser/ are comments), so it could take the rule class-string and do it once.

Two cosmetic notes, take or leave: the #5309 rules cache $isInTrait = $scope->isInTrait() once at the top; and testBug14416 only exists in IssetRuleTest — the empty() / ?? equivalents are already covered by MaybeDeclaredPropertyTrait in isset-in-trait.php, so that one is purely about symmetry.

Nothing I found looks like a blocker. Thanks for sticking with this one since April — the fixture in particular is a nice piece of work.

@staabm what do you think?