From 6f9986a1347dac041c42c1dc569f9cfc62d422de Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Tue, 28 Jul 2026 22:14:47 +0200 Subject: [PATCH] Resolve isset/empty/?? chains from handler-built descriptors Rules\IssetCheck was a hand-maintained mirror of MutatingScope::issetCheck's chain walk (variable / offset / property / leaf), re-walking the AST and re-resolving types and property reflections with its own drifting copy of the logic. The chain-link handlers (Variable / ArrayDimFetch / PropertyFetch / StaticPropertyFetch) now attach an IssetabilityDescriptor to their ExpressionResult, built from the child results they already have in hand. IssetHandler, EmptyHandler, CoalesceHandler and AssignOpHandler (??=) emit virtual nodes carrying the subject ExpressionResults; IssetRule, EmptyRule and NullCoalesceRule listen on those nodes, and Rules\IssetCheck folds the resolved links of an IssetabilityResolution instead of walking the AST. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01DaBZjgksga4c5s6Q9FniY7 --- .../ExprHandler/ArrayDimFetchHandler.php | 2 + src/Analyser/ExprHandler/AssignOpHandler.php | 19 ++ src/Analyser/ExprHandler/CoalesceHandler.php | 3 + src/Analyser/ExprHandler/EmptyHandler.php | 3 + src/Analyser/ExprHandler/IssetHandler.php | 5 + .../ExprHandler/PropertyFetchHandler.php | 3 + .../StaticPropertyFetchHandler.php | 4 + src/Analyser/ExprHandler/VariableHandler.php | 2 + src/Analyser/ExpressionResult.php | 27 ++ src/Analyser/ExpressionResultFactory.php | 1 + src/Analyser/IssetabilityDescriptor.php | 163 ++++++++++ src/Analyser/IssetabilityLinkInfo.php | 307 ++++++++++++++++++ src/Analyser/IssetabilityResolution.php | 154 +++++++++ src/Node/CoalesceExpressionNode.php | 59 ++++ src/Node/EmptyExpressionNode.php | 45 +++ src/Node/IssetExpressionNode.php | 51 +++ src/Rules/IssetCheck.php | 227 ++++++------- src/Rules/Variables/EmptyRule.php | 7 +- src/Rules/Variables/IssetRule.php | 9 +- src/Rules/Variables/NullCoalesceRule.php | 66 ++-- .../PHPStan/Rules/Variables/EmptyRuleTest.php | 2 - .../PHPStan/Rules/Variables/IssetRuleTest.php | 2 - .../Rules/Variables/NullCoalesceRuleTest.php | 2 - 23 files changed, 988 insertions(+), 175 deletions(-) create mode 100644 src/Analyser/IssetabilityDescriptor.php create mode 100644 src/Analyser/IssetabilityLinkInfo.php create mode 100644 src/Analyser/IssetabilityResolution.php create mode 100644 src/Node/CoalesceExpressionNode.php create mode 100644 src/Node/EmptyExpressionNode.php create mode 100644 src/Node/IssetExpressionNode.php diff --git a/src/Analyser/ExprHandler/ArrayDimFetchHandler.php b/src/Analyser/ExprHandler/ArrayDimFetchHandler.php index b52587be917..210e1318d6d 100644 --- a/src/Analyser/ExprHandler/ArrayDimFetchHandler.php +++ b/src/Analyser/ExprHandler/ArrayDimFetchHandler.php @@ -15,6 +15,7 @@ use PHPStan\Analyser\ExpressionResultStorage; use PHPStan\Analyser\ExprHandler; use PHPStan\Analyser\ExprHandler\Helper\NullsafeShortCircuitingHelper; +use PHPStan\Analyser\IssetabilityDescriptor; use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\NodeScopeResolver; use PHPStan\Analyser\NoopNodeCallback; @@ -125,6 +126,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex throwPoints: $throwPoints, impurePoints: $impurePoints, containsNullsafe: $varResult->containsNullsafe(), + issetabilityDescriptor: IssetabilityDescriptor::offset($varResult, $dimResult), ); } diff --git a/src/Analyser/ExprHandler/AssignOpHandler.php b/src/Analyser/ExprHandler/AssignOpHandler.php index fb25464e4b5..5484969f1b7 100644 --- a/src/Analyser/ExprHandler/AssignOpHandler.php +++ b/src/Analyser/ExprHandler/AssignOpHandler.php @@ -15,14 +15,17 @@ use PHPStan\Analyser\ExpressionResultStorage; use PHPStan\Analyser\ExprHandler; use PHPStan\Analyser\ExprHandler\Helper\ImplicitToStringCallHelper; +use PHPStan\Analyser\ExprHandler\Helper\NonNullabilityHelper; use PHPStan\Analyser\InternalThrowPoint; use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\NodeScopeResolver; +use PHPStan\Analyser\NoopNodeCallback; use PHPStan\Analyser\Scope; use PHPStan\Analyser\SpecifiedTypes; use PHPStan\Analyser\TypeSpecifier; use PHPStan\Analyser\TypeSpecifierContext; use PHPStan\DependencyInjection\AutowiredService; +use PHPStan\Node\CoalesceExpressionNode; use PHPStan\Reflection\InitializerExprTypeResolver; use PHPStan\ShouldNotHappenException; use PHPStan\Type\Constant\ConstantIntegerType; @@ -45,6 +48,7 @@ public function __construct( private InitializerExprTypeResolver $initializerExprTypeResolver, private ImplicitToStringCallHelper $implicitToStringCallHelper, private ExpressionResultFactory $expressionResultFactory, + private NonNullabilityHelper $nonNullabilityHelper, ) { } @@ -57,6 +61,17 @@ public function supports(Expr $expr): bool public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Expr $expr, MutatingScope $scope, ExpressionResultStorage $storage, callable $nodeCallback, ExpressionContext $context): ExpressionResult { $beforeScope = $scope; + $condResult = null; + if ($expr instanceof Expr\AssignOp\Coalesce) { + // `$lvalue ??= ...` is `$lvalue = $lvalue ?? ...`: price the left side + // once as a read (mirroring CoalesceHandler's left-side processing) so + // its result carries the isset descriptor for NullCoalesceRule. The + // NoopNodeCallback avoids duplicate reports and the read's scope is + // discarded: processAssignVar() walks the target itself. + $nonNullabilityResult = $this->nonNullabilityHelper->ensureNonNullability($scope, $expr->var); + $condScope = $nodeScopeResolver->lookForSetAllowedUndefinedExpressions($nonNullabilityResult->getScope(), $expr->var); + $condResult = $nodeScopeResolver->processExprNode($stmt, $expr->var, $condScope, $storage, new NoopNodeCallback(), $context->enterDeep()); + } $assignResult = $this->assignHandler->processAssignVar( $nodeScopeResolver, $scope, @@ -114,6 +129,10 @@ function (MutatingScope $scope) use ($stmt, $expr, $nodeCallback, $context, $sto $impurePoints = array_merge($impurePoints, $toStringResult->getImpurePoints()); } + if ($expr instanceof Expr\AssignOp\Coalesce) { + $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, 'on left side of ??='), $beforeScope, $storage, $context); + } + return $this->expressionResultFactory->create( $scope, beforeScope: $beforeScope, diff --git a/src/Analyser/ExprHandler/CoalesceHandler.php b/src/Analyser/ExprHandler/CoalesceHandler.php index d7b0fa53571..c0a54c5bee3 100644 --- a/src/Analyser/ExprHandler/CoalesceHandler.php +++ b/src/Analyser/ExprHandler/CoalesceHandler.php @@ -18,6 +18,7 @@ use PHPStan\Analyser\TypeSpecifier; use PHPStan\Analyser\TypeSpecifierContext; use PHPStan\DependencyInjection\AutowiredService; +use PHPStan\Node\CoalesceExpressionNode; use PHPStan\ShouldNotHappenException; use PHPStan\Type\Constant\ConstantBooleanType; use PHPStan\Type\NeverType; @@ -135,6 +136,8 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $scope = $scope->filterByTruthyValue(new Expr\Isset_([$expr->left]))->mergeWith($rightResult->getScope()); } + $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, 'on left side of ??'), $beforeScope, $storage, $context); + return $this->expressionResultFactory->create( $scope, beforeScope: $beforeScope, diff --git a/src/Analyser/ExprHandler/EmptyHandler.php b/src/Analyser/ExprHandler/EmptyHandler.php index 54e9c397fa1..45e30427624 100644 --- a/src/Analyser/ExprHandler/EmptyHandler.php +++ b/src/Analyser/ExprHandler/EmptyHandler.php @@ -19,6 +19,7 @@ use PHPStan\Analyser\TypeSpecifier; use PHPStan\Analyser\TypeSpecifierContext; use PHPStan\DependencyInjection\AutowiredService; +use PHPStan\Node\EmptyExpressionNode; use PHPStan\ShouldNotHappenException; use PHPStan\Type\BooleanType; use PHPStan\Type\Constant\ConstantBooleanType; @@ -95,6 +96,8 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $scope = $this->nonNullabilityHelper->revertNonNullability($scope, $nonNullabilityResult->getSpecifiedExpressions()); $scope = $nodeScopeResolver->lookForUnsetAllowedUndefinedExpressions($scope, $expr->expr); + $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new EmptyExpressionNode($expr, $exprResult), $beforeScope, $storage, $context); + return $this->expressionResultFactory->create( $scope, beforeScope: $beforeScope, diff --git a/src/Analyser/ExprHandler/IssetHandler.php b/src/Analyser/ExprHandler/IssetHandler.php index 9fc6c135a1c..62d9abadf04 100644 --- a/src/Analyser/ExprHandler/IssetHandler.php +++ b/src/Analyser/ExprHandler/IssetHandler.php @@ -29,6 +29,7 @@ use PHPStan\DependencyInjection\AutowiredService; use PHPStan\Node\Expr\TypeExpr; use PHPStan\Node\IssetExpr; +use PHPStan\Node\IssetExpressionNode; use PHPStan\Rules\Arrays\AllowedArrayKeysTypes; use PHPStan\ShouldNotHappenException; use PHPStan\Type\Accessory\HasOffsetType; @@ -352,10 +353,12 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $impurePoints = []; $nonNullabilityResults = []; $isAlwaysTerminating = false; + $varResults = []; foreach ($expr->vars as $var) { $nonNullabilityResult = $this->nonNullabilityHelper->ensureNonNullability($scope, $var); $scope = $nodeScopeResolver->lookForSetAllowedUndefinedExpressions($nonNullabilityResult->getScope(), $var); $varResult = $nodeScopeResolver->processExprNode($stmt, $var, $scope, $storage, $nodeCallback, $context->enterDeep()); + $varResults[] = $varResult; $scope = $varResult->getScope(); $hasYield = $hasYield || $varResult->hasYield(); $throwPoints = array_merge($throwPoints, $varResult->getThrowPoints()); @@ -388,6 +391,8 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $scope = $this->nonNullabilityHelper->revertNonNullability($scope, $nonNullabilityResult->getSpecifiedExpressions()); } + $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new IssetExpressionNode($expr, $varResults), $beforeScope, $storage, $context); + return $this->expressionResultFactory->create( $scope, beforeScope: $beforeScope, diff --git a/src/Analyser/ExprHandler/PropertyFetchHandler.php b/src/Analyser/ExprHandler/PropertyFetchHandler.php index 805b2b190d5..95f1e1c72d9 100644 --- a/src/Analyser/ExprHandler/PropertyFetchHandler.php +++ b/src/Analyser/ExprHandler/PropertyFetchHandler.php @@ -14,6 +14,7 @@ use PHPStan\Analyser\ExprHandler; use PHPStan\Analyser\ExprHandler\Helper\NullsafeShortCircuitingHelper; use PHPStan\Analyser\InternalThrowPoint; +use PHPStan\Analyser\IssetabilityDescriptor; use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\NodeScopeResolver; use PHPStan\Analyser\Scope; @@ -22,6 +23,7 @@ use PHPStan\Analyser\TypeSpecifierContext; use PHPStan\DependencyInjection\AutowiredService; use PHPStan\Php\PhpVersion; +use PHPStan\Rules\Properties\FoundPropertyReflection; use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Type\ErrorType; use PHPStan\Type\MixedType; @@ -95,6 +97,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex throwPoints: $throwPoints, impurePoints: $impurePoints, containsNullsafe: $varResult->containsNullsafe(), + issetabilityDescriptor: IssetabilityDescriptor::property($varResult, fn (MutatingScope $s): ?FoundPropertyReflection => $this->propertyReflectionFinder->findPropertyReflectionFromNode($expr, $s), $expr), ); } diff --git a/src/Analyser/ExprHandler/StaticPropertyFetchHandler.php b/src/Analyser/ExprHandler/StaticPropertyFetchHandler.php index dd15b5a6eb4..0d964e7fafc 100644 --- a/src/Analyser/ExprHandler/StaticPropertyFetchHandler.php +++ b/src/Analyser/ExprHandler/StaticPropertyFetchHandler.php @@ -16,6 +16,7 @@ use PHPStan\Analyser\ExprHandler; use PHPStan\Analyser\ExprHandler\Helper\NullsafeShortCircuitingHelper; use PHPStan\Analyser\ImpurePoint; +use PHPStan\Analyser\IssetabilityDescriptor; use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\NodeScopeResolver; use PHPStan\Analyser\Scope; @@ -23,6 +24,7 @@ use PHPStan\Analyser\TypeSpecifier; use PHPStan\Analyser\TypeSpecifierContext; use PHPStan\DependencyInjection\AutowiredService; +use PHPStan\Rules\Properties\FoundPropertyReflection; use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Type\ErrorType; use PHPStan\Type\MixedType; @@ -67,6 +69,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex ]; $isAlwaysTerminating = false; $containsNullsafe = false; + $classResult = null; if ($expr->class instanceof Expr) { $classResult = $nodeScopeResolver->processExprNode($stmt, $expr->class, $scope, $storage, $nodeCallback, $context->enterDeep()); $hasYield = $classResult->hasYield(); @@ -94,6 +97,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex throwPoints: $throwPoints, impurePoints: $impurePoints, containsNullsafe: $containsNullsafe, + issetabilityDescriptor: IssetabilityDescriptor::property($classResult, fn (MutatingScope $s): ?FoundPropertyReflection => $this->propertyReflectionFinder->findPropertyReflectionFromNode($expr, $s), $expr), ); } diff --git a/src/Analyser/ExprHandler/VariableHandler.php b/src/Analyser/ExprHandler/VariableHandler.php index eff0cee6c9a..c34c281ffad 100644 --- a/src/Analyser/ExprHandler/VariableHandler.php +++ b/src/Analyser/ExprHandler/VariableHandler.php @@ -13,6 +13,7 @@ use PHPStan\Analyser\ExpressionResultStorage; use PHPStan\Analyser\ExprHandler; use PHPStan\Analyser\ImpurePoint; +use PHPStan\Analyser\IssetabilityDescriptor; use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\NodeScopeResolver; use PHPStan\Analyser\Scope; @@ -103,6 +104,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $isAlwaysTerminating, $throwPoints, $impurePoints, + issetabilityDescriptor: is_string($expr->name) ? IssetabilityDescriptor::variable($expr->name) : null, truthyScopeCallback: static fn (): MutatingScope => $scope->filterByTruthyValue($expr), falseyScopeCallback: static fn (): MutatingScope => $scope->filterByFalseyValue($expr), ); diff --git a/src/Analyser/ExpressionResult.php b/src/Analyser/ExpressionResult.php index 91a801351b7..3a937d61903 100644 --- a/src/Analyser/ExpressionResult.php +++ b/src/Analyser/ExpressionResult.php @@ -35,6 +35,7 @@ public function __construct( private array $throwPoints, private array $impurePoints, private bool $containsNullsafe = false, + private ?IssetabilityDescriptor $issetabilityDescriptor = null, ?callable $truthyScopeCallback = null, ?callable $falseyScopeCallback = null, ) @@ -69,6 +70,32 @@ public function containsNullsafe(): bool return $this->containsNullsafe; } + /** + * The isset/empty/?? view of this expression evaluated at the given + * scope: folds the chain descriptor, or builds a leaf resolution from the + * expression's own type when it is not a chain link (e.g. a method-call-rooted + * base like $this->getFoo()['x']). $useNativeTypes selects native vs phpdoc. + */ + public function getIssetabilityResolution(MutatingScope $scope, bool $useNativeTypes): IssetabilityResolution + { + if ($this->issetabilityDescriptor !== null) { + return $this->issetabilityDescriptor->resolve($scope, $useNativeTypes, $this->expr); + } + + $type = $this->getTypeOnScope($scope, $useNativeTypes); + + return new IssetabilityResolution( + IssetabilityLinkInfo::leaf($type, $this->expr, $this->expr instanceof Expr\NullsafePropertyFetch), + null, + ); + } + + /** Prices this result's expression on the given scope in the requested flavour. */ + public function getTypeOnScope(MutatingScope $scope, bool $useNativeTypes): Type + { + return $useNativeTypes ? $scope->getNativeType($this->expr) : $scope->getType($this->expr); + } + /** * @return InternalThrowPoint[] */ diff --git a/src/Analyser/ExpressionResultFactory.php b/src/Analyser/ExpressionResultFactory.php index 8044067a87f..e49481036da 100644 --- a/src/Analyser/ExpressionResultFactory.php +++ b/src/Analyser/ExpressionResultFactory.php @@ -22,6 +22,7 @@ public function create( array $throwPoints, array $impurePoints, bool $containsNullsafe = false, + ?IssetabilityDescriptor $issetabilityDescriptor = null, ?callable $truthyScopeCallback = null, ?callable $falseyScopeCallback = null, ): ExpressionResult; diff --git a/src/Analyser/IssetabilityDescriptor.php b/src/Analyser/IssetabilityDescriptor.php new file mode 100644 index 00000000000..3ac913dc4a2 --- /dev/null +++ b/src/Analyser/IssetabilityDescriptor.php @@ -0,0 +1,163 @@ +kind === self::KIND_VARIABLE) { + $variableName = $this->variableName; + if ($variableName === null) { + throw new ShouldNotHappenException(); + } + + $hasVariable = $scope->hasVariableType($variableName); + $valueType = $hasVariable->yes() + ? ($useNativeTypes ? $scope->doNotTreatPhpDocTypesAsCertain()->getVariableType($variableName) : $scope->getVariableType($variableName)) + : new NeverType(); + + return new IssetabilityResolution(IssetabilityLinkInfo::variable($variableName, $hasVariable, $valueType), null); + } + + if ($this->kind === self::KIND_OFFSET) { + $varResult = $this->varResult; + $dimResult = $this->dimResult; + if ($varResult === null || $dimResult === null) { + throw new ShouldNotHappenException(); + } + + $varType = $varResult->getTypeOnScope($scope, $useNativeTypes); + $dimType = $dimResult->getTypeOnScope($scope, $useNativeTypes); + $hasOffsetValue = $varType->hasOffsetValueType($dimType); + $valueType = $hasOffsetValue->no() ? new NeverType() : $varType->getOffsetValueType($dimType); + + return new IssetabilityResolution( + IssetabilityLinkInfo::offset( + $varType->isOffsetAccessible(), + $hasOffsetValue, + $scope->hasExpressionType($expr)->yes(), + $varType, + $dimType, + $valueType, + ), + $varResult->getIssetabilityResolution($scope, $useNativeTypes), + ); + } + + $reflectionResolver = $this->reflectionResolver; + $propertyFetch = $this->propertyFetch; + if ($reflectionResolver === null || $propertyFetch === null) { + throw new ShouldNotHappenException(); + } + + $inner = $this->innerResult !== null ? $this->innerResult->getIssetabilityResolution($scope, $useNativeTypes) : null; + + $propertyReflection = $reflectionResolver($scope); + if ($propertyReflection === null) { + return new IssetabilityResolution( + IssetabilityLinkInfo::property(null, $propertyFetch, false, false, TrinaryLogic::createNo(), new NeverType(), new NeverType(), false, false, false, false, false, false, false, false), + $inner, + ); + } + + $hasNativeType = $propertyReflection->hasNativeType(); + $nativeReflection = $propertyReflection->getNativeReflection(); + $initializedThisProperty = $propertyFetch instanceof PropertyFetch + && $propertyFetch->name instanceof Identifier + && $propertyFetch->var instanceof Variable + && $propertyFetch->var->name === 'this' + && $scope->hasExpressionType(new PropertyInitializationExpr($propertyReflection->getName()))->yes(); + + return new IssetabilityResolution( + IssetabilityLinkInfo::property( + $propertyReflection, + $propertyFetch, + $propertyReflection->isNative(), + $hasNativeType, + $propertyReflection->isVirtual(), + $propertyReflection->getWritableType(), + $hasNativeType ? $propertyReflection->getNativeType() : new NeverType(), + $scope->hasExpressionType($propertyFetch)->yes(), + isset($scope->getConditionalExpressions()[$scope->getNodeKey($propertyFetch)]), + $initializedThisProperty, + $nativeReflection !== null, + $nativeReflection !== null && $nativeReflection->isPromoted(), + $nativeReflection !== null && $nativeReflection->isReadOnly(), + $nativeReflection !== null && $nativeReflection->isHooked(), + $nativeReflection !== null && $nativeReflection->getNativeReflection()->hasDefaultValue(), + ), + $inner, + ); + } + +} diff --git a/src/Analyser/IssetabilityLinkInfo.php b/src/Analyser/IssetabilityLinkInfo.php new file mode 100644 index 00000000000..a59416d6b99 --- /dev/null +++ b/src/Analyser/IssetabilityLinkInfo.php @@ -0,0 +1,307 @@ +kind === self::KIND_VARIABLE; + } + + public function isOffset(): bool + { + return $this->kind === self::KIND_OFFSET; + } + + public function isProperty(): bool + { + return $this->kind === self::KIND_PROPERTY; + } + + public function getVariableName(): string + { + if ($this->variableName === null) { + throw new ShouldNotHappenException(); + } + + return $this->variableName; + } + + public function getHasVariable(): TrinaryLogic + { + if ($this->hasVariable === null) { + throw new ShouldNotHappenException(); + } + + return $this->hasVariable; + } + + /** The type the operator's callback inspects: variable type, offset value type, property writable type, or leaf type. */ + public function getValueType(): Type + { + if ($this->valueType === null) { + throw new ShouldNotHappenException(); + } + + return $this->valueType; + } + + public function getIsOffsetAccessible(): TrinaryLogic + { + if ($this->isOffsetAccessible === null) { + throw new ShouldNotHappenException(); + } + + return $this->isOffsetAccessible; + } + + public function getHasOffsetValue(): TrinaryLogic + { + if ($this->hasOffsetValue === null) { + throw new ShouldNotHappenException(); + } + + return $this->hasOffsetValue; + } + + public function hasExpressionTypeOfExpr(): bool + { + return $this->hasExpressionTypeOfExpr; + } + + public function getVarType(): Type + { + if ($this->varType === null) { + throw new ShouldNotHappenException(); + } + + return $this->varType; + } + + public function getDimType(): Type + { + if ($this->dimType === null) { + throw new ShouldNotHappenException(); + } + + return $this->dimType; + } + + public function getPropertyReflection(): ?FoundPropertyReflection + { + return $this->propertyReflection; + } + + /** + * @return Expr\PropertyFetch|Expr\StaticPropertyFetch + */ + public function getPropertyFetch(): Expr + { + if (!$this->propertyFetch instanceof Expr\PropertyFetch && !$this->propertyFetch instanceof Expr\StaticPropertyFetch) { + throw new ShouldNotHappenException(); + } + + return $this->propertyFetch; + } + + public function isReflectionNative(): bool + { + return $this->reflectionNative; + } + + public function hasNativeType(): bool + { + return $this->hasNativeType; + } + + public function isVirtual(): TrinaryLogic + { + if ($this->isVirtual === null) { + throw new ShouldNotHappenException(); + } + + return $this->isVirtual; + } + + public function getNativeType(): Type + { + if ($this->nativeType === null) { + throw new ShouldNotHappenException(); + } + + return $this->nativeType; + } + + public function hasExpressionTypeOfFetch(): bool + { + return $this->hasExpressionTypeOfFetch; + } + + /** + * Whether the scope holds conditional-expression entries about the fetch. + * Such entries exist only when the fetch was narrowed in an evaluated + * condition - and evaluating a condition READS the fetch, which would have + * thrown on an uninitialized typed property. A typed-property read + * witnesses initialization. + */ + public function hasConditionalExpressionsOfFetch(): bool + { + return $this->hasConditionalExpressionsOfFetch; + } + + public function isInitializedThisProperty(): bool + { + return $this->initializedThisProperty; + } + + public function nativeReflectionExists(): bool + { + return $this->nativeReflectionExists; + } + + public function nativeIsPromoted(): bool + { + return $this->nativeIsPromoted; + } + + public function nativeIsReadOnly(): bool + { + return $this->nativeIsReadOnly; + } + + public function nativeIsHooked(): bool + { + return $this->nativeIsHooked; + } + + public function nativeHasDefaultValue(): bool + { + return $this->nativeHasDefaultValue; + } + + public function getLeafExpr(): Expr + { + if ($this->leafExpr === null) { + throw new ShouldNotHappenException(); + } + + return $this->leafExpr; + } + + public function leafIsNullsafePropertyFetch(): bool + { + return $this->leafIsNullsafePropertyFetch; + } + +} diff --git a/src/Analyser/IssetabilityResolution.php b/src/Analyser/IssetabilityResolution.php new file mode 100644 index 00000000000..9695f977ecd --- /dev/null +++ b/src/Analyser/IssetabilityResolution.php @@ -0,0 +1,154 @@ +link; + } + + public function getInner(): ?IssetabilityResolution + { + return $this->inner; + } + + /** + * Whether isset() of the whole chain holds: null = maybe (resolves to bool), + * true/false = the typeCallback's verdict on the leaf type threaded outward + * over the chain's set-ness. Mirrors the former MutatingScope::issetCheck(). + * + * @param callable(Type): ?bool $typeCallback + */ + public function isSet(callable $typeCallback, ?bool $result = null): ?bool + { + $link = $this->link; + + if ($link->isVariable()) { + $hasVariable = $link->getHasVariable(); + if ($hasVariable->maybe()) { + return null; + } + + if ($result === null) { + if ($hasVariable->yes()) { + if ($link->getVariableName() === '_SESSION') { + return null; + } + + return $typeCallback($link->getValueType()); + } + + return false; + } + + return $result; + } + + if ($link->isOffset()) { + if (!$link->getIsOffsetAccessible()->yes()) { + return $result ?? ($this->inner !== null ? $this->inner->isSetUndefined() : null); + } + + $hasOffsetValue = $link->getHasOffsetValue(); + if ($hasOffsetValue->no()) { + return false; + } + + // If offset cannot be null, store this verdict and see if one of the earlier + // offsets is. E.g. $array['a']['b']['c'] ?? null; is a valid coalesce if a OR + // b OR c might be null. + if ($hasOffsetValue->yes()) { + $result = $typeCallback($link->getValueType()); + + if ($result !== null) { + return $this->inner !== null ? $this->inner->isSet($typeCallback, $result) : $result; + } + } + + // Has offset, it is nullable + return null; + } + + if ($link->isProperty()) { + if ($link->getPropertyReflection() === null || !$link->isReflectionNative()) { + return $this->inner !== null ? $this->inner->isSetUndefined() : null; + } + + if ( + $link->hasNativeType() + && !$link->isVirtual()->yes() + && !$link->hasExpressionTypeOfFetch() + && !$link->hasConditionalExpressionsOfFetch() + && !$link->nativeHasDefaultValue() + && (!$link->nativeReflectionExists() || !$link->nativeIsPromoted() || (!$link->nativeIsReadOnly() && !$link->nativeIsHooked())) + ) { + return $this->inner !== null ? $this->inner->isSetUndefined() : null; + } + + if ($result !== null) { + return $this->inner !== null ? $this->inner->isSet($typeCallback, $result) : $result; + } + + $result = $typeCallback($link->getValueType()); + if ($result !== null && $this->inner !== null) { + return $this->inner->isSet($typeCallback, $result); + } + + return $result; + } + + // leaf + return $result ?? $typeCallback($link->getValueType()); + } + + private function isSetUndefined(): ?bool + { + $link = $this->link; + + if ($link->isVariable()) { + if (!$link->getHasVariable()->no()) { + return null; + } + + return false; + } + + if ($link->isOffset()) { + if (!$link->getIsOffsetAccessible()->yes()) { + return $this->inner !== null ? $this->inner->isSetUndefined() : null; + } + + if (!$link->getHasOffsetValue()->no()) { + return $this->inner !== null ? $this->inner->isSetUndefined() : null; + } + + return false; + } + + if ($link->isProperty()) { + return $this->inner !== null ? $this->inner->isSetUndefined() : null; + } + + return null; + } + +} diff --git a/src/Node/CoalesceExpressionNode.php b/src/Node/CoalesceExpressionNode.php new file mode 100644 index 00000000000..e8653bbc426 --- /dev/null +++ b/src/Node/CoalesceExpressionNode.php @@ -0,0 +1,59 @@ +getAttributes()); + } + + public function getOriginalExpr(): Expr + { + return $this->originalExpr; + } + + public function getSubjectResult(): ExpressionResult + { + return $this->subjectResult; + } + + public function getOperatorDescription(): string + { + return $this->operatorDescription; + } + + #[Override] + public function getType(): string + { + return 'PHPStan_Node_CoalesceExpressionNode'; + } + + /** + * @return string[] + */ + #[Override] + public function getSubNodeNames(): array + { + return []; + } + +} diff --git a/src/Node/EmptyExpressionNode.php b/src/Node/EmptyExpressionNode.php new file mode 100644 index 00000000000..e6685101a76 --- /dev/null +++ b/src/Node/EmptyExpressionNode.php @@ -0,0 +1,45 @@ +getAttributes()); + } + + public function getExprResult(): ExpressionResult + { + return $this->exprResult; + } + + #[Override] + public function getType(): string + { + return 'PHPStan_Node_EmptyExpressionNode'; + } + + /** + * @return string[] + */ + #[Override] + public function getSubNodeNames(): array + { + return []; + } + +} diff --git a/src/Node/IssetExpressionNode.php b/src/Node/IssetExpressionNode.php new file mode 100644 index 00000000000..6c8bc870198 --- /dev/null +++ b/src/Node/IssetExpressionNode.php @@ -0,0 +1,51 @@ +getAttributes()); + } + + /** + * @return ExpressionResult[] + */ + public function getVarResults(): array + { + return $this->varResults; + } + + #[Override] + public function getType(): string + { + return 'PHPStan_Node_IssetExpressionNode'; + } + + /** + * @return string[] + */ + #[Override] + public function getSubNodeNames(): array + { + return []; + } + +} diff --git a/src/Rules/IssetCheck.php b/src/Rules/IssetCheck.php index 35195e7ea52..eff4ed98840 100644 --- a/src/Rules/IssetCheck.php +++ b/src/Rules/IssetCheck.php @@ -2,22 +2,27 @@ namespace PHPStan\Rules; -use PhpParser\Node; -use PhpParser\Node\Expr; +use PhpParser\Node\Expr\NullsafePropertyFetch; +use PhpParser\Node\Identifier; +use PHPStan\Analyser\ExpressionResult; +use PHPStan\Analyser\IssetabilityResolution; +use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\Scope; use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\AutowiredService; -use PHPStan\Node\Expr\PropertyInitializationExpr; use PHPStan\Rules\Properties\PropertyDescriptor; -use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Type\NeverType; use PHPStan\Type\Type; use PHPStan\Type\VerbosityLevel; -use function is_string; use function sprintf; use function str_starts_with; /** + * Renders the isset/empty/?? "does it make sense" errors from the single + * IssetabilityResolution the engine already computed. The chain is walked and + * resolved once (IssetabilityDescriptor::resolve); this only projects the + * resolved facts into messages - it never re-walks the AST nor re-resolves types. + * * @phpstan-type ErrorIdentifier = 'empty'|'isset'|'nullCoalesce' */ #[AutowiredService] @@ -26,7 +31,6 @@ final class IssetCheck public function __construct( private PropertyDescriptor $propertyDescriptor, - private PropertyReflectionFinder $propertyReflectionFinder, #[AutowiredParameter] private bool $checkAdvancedIsset, #[AutowiredParameter] @@ -39,26 +43,40 @@ public function __construct( * @param ErrorIdentifier $identifier * @param callable(Type): ?string $typeMessageCallback */ - public function check(Expr $expr, Scope $scope, string $operatorDescription, string $identifier, callable $typeMessageCallback, ?IdentifierRuleError $error = null): ?IdentifierRuleError + public function check(ExpressionResult $exprResult, Scope $scope, string $operatorDescription, string $identifier, callable $typeMessageCallback): ?IdentifierRuleError { - // mirrored in PHPStan\Analyser\MutatingScope::issetCheck() - if ($expr instanceof Node\Expr\Variable && is_string($expr->name)) { - $hasVariable = $scope->hasVariableType($expr->name); + $mutatingScope = $scope->toMutatingScope(); + $resolution = $exprResult->getIssetabilityResolution($mutatingScope, !$this->treatPhpDocTypesAsCertain); + + return $this->doCheck($resolution, $mutatingScope, $operatorDescription, $identifier, $typeMessageCallback, null); + } + + /** + * @param ErrorIdentifier $identifier + * @param callable(Type): ?string $typeMessageCallback + */ + private function doCheck(IssetabilityResolution $resolution, MutatingScope $scope, string $operatorDescription, string $identifier, callable $typeMessageCallback, ?IdentifierRuleError $error): ?IdentifierRuleError + { + $link = $resolution->getLink(); + $inner = $resolution->getInner(); + + if ($link->isVariable()) { + $hasVariable = $link->getHasVariable(); if ($hasVariable->maybe()) { return null; } if ($error === null) { if ($hasVariable->yes()) { - if ($expr->name === '_SESSION') { + if ($link->getVariableName() === '_SESSION') { return null; } - $type = $this->treatPhpDocTypesAsCertain ? $scope->getScopeType($expr) : $scope->getScopeNativeType($expr); + $type = $link->getValueType(); if (!$type instanceof NeverType) { return $this->generateError( $type, - sprintf('Variable $%s %s always exists and', $expr->name, $operatorDescription), + sprintf('Variable $%s %s always exists and', $link->getVariableName(), $operatorDescription), $typeMessageCallback, $identifier, 'variable', @@ -66,24 +84,22 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, str } } - return RuleErrorBuilder::message(sprintf('Variable $%s %s is never defined.', $expr->name, $operatorDescription)) + return RuleErrorBuilder::message(sprintf('Variable $%s %s is never defined.', $link->getVariableName(), $operatorDescription)) ->identifier(sprintf('%s.variable', $identifier)) ->build(); } return $error; - } elseif ($expr instanceof Node\Expr\ArrayDimFetch && $expr->dim !== null) { - $type = $this->treatPhpDocTypesAsCertain - ? $scope->getScopeType($expr->var) - : $scope->getScopeNativeType($expr->var); - if (!$type->isOffsetAccessible()->yes()) { - return $error ?? $this->checkUndefined($expr->var, $scope, $operatorDescription, $identifier); + } + + if ($link->isOffset()) { + $type = $link->getVarType(); + if (!$link->getIsOffsetAccessible()->yes()) { + return $error ?? $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier); } - $dimType = $this->treatPhpDocTypesAsCertain - ? $scope->getScopeType($expr->dim) - : $scope->getScopeNativeType($expr->dim); - $hasOffsetValue = $type->hasOffsetValueType($dimType); + $dimType = $link->getDimType(); + $hasOffsetValue = $link->getHasOffsetValue(); if ($hasOffsetValue->no()) { if (!$this->checkAdvancedIsset) { return null; @@ -101,12 +117,12 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, str // If offset cannot be null, store this error message and see if one of the earlier offsets is. // E.g. $array['a']['b']['c'] ?? null; is a valid coalesce if a OR b or C might be null. - if ($hasOffsetValue->yes() || $scope->hasExpressionType($expr)->yes()) { + if ($hasOffsetValue->yes() || $link->hasExpressionTypeOfExpr()) { if (!$this->checkAdvancedIsset) { return null; } - $error ??= $this->generateError($type->getOffsetValueType($dimType), sprintf( + $error ??= $this->generateError($link->getValueType(), sprintf( 'Offset %s on %s %s always exists and', $dimType->describe(VerbosityLevel::value()), $type->describe(VerbosityLevel::value()), @@ -114,56 +130,27 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, str ), $typeMessageCallback, $identifier, 'offset'); if ($error !== null) { - return $this->check($expr->var, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error); + return $inner !== null ? $this->doCheck($inner, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error) : $error; } } // Has offset, it is nullable return null; + } - } elseif ($expr instanceof Node\Expr\PropertyFetch || $expr instanceof Node\Expr\StaticPropertyFetch) { - - $propertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($expr, $scope->toMutatingScope()); - - if ($propertyReflection === null) { - if ($expr instanceof Node\Expr\PropertyFetch) { - return $this->checkUndefined($expr->var, $scope, $operatorDescription, $identifier); - } - - if ($expr->class instanceof Expr) { - return $this->checkUndefined($expr->class, $scope, $operatorDescription, $identifier); - } - - return null; - } - - if (!$propertyReflection->isNative()) { - if ($expr instanceof Node\Expr\PropertyFetch) { - return $this->checkUndefined($expr->var, $scope, $operatorDescription, $identifier); - } - - if ($expr->class instanceof Expr) { - return $this->checkUndefined($expr->class, $scope, $operatorDescription, $identifier); - } + if ($link->isProperty()) { + $reflection = $link->getPropertyReflection(); + $propertyFetch = $link->getPropertyFetch(); - return null; + if ($reflection === null || !$link->isReflectionNative()) { + return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier); } - if ($propertyReflection->hasNativeType() && !$propertyReflection->isVirtual()->yes()) { - if ( - $expr instanceof Node\Expr\PropertyFetch - && $expr->name instanceof Node\Identifier - && $expr->var instanceof Expr\Variable - && $expr->var->name === 'this' - && $scope->hasExpressionType(new PropertyInitializationExpr($propertyReflection->getName()))->yes() - ) { + if ($link->hasNativeType() && !$link->isVirtual()->yes()) { + if ($link->isInitializedThisProperty()) { return $this->generateError( - $propertyReflection->getNativeType(), - sprintf( - '%s %s', - $this->propertyDescriptor->describeProperty($propertyReflection, $scope, $expr), - $operatorDescription, - ), + $link->getNativeType(), + sprintf('%s %s', $this->propertyDescriptor->describeProperty($reflection, $scope, $propertyFetch), $operatorDescription), static function (Type $type) use ($typeMessageCallback): ?string { $originalMessage = $typeMessageCallback($type); if ($originalMessage === null) { @@ -181,64 +168,43 @@ static function (Type $type) use ($typeMessageCallback): ?string { ); } - if (!$scope->hasExpressionType($expr)->yes()) { - $nativeReflection = $propertyReflection->getNativeReflection(); - if ( - $nativeReflection !== null - && !$nativeReflection->getNativeReflection()->hasDefaultValue() - && (!$nativeReflection->isPromoted() || (!$nativeReflection->isReadOnly() && !$nativeReflection->isHooked())) - ) { - return null; - } + if ( + !$link->hasExpressionTypeOfFetch() + && $link->nativeReflectionExists() + && !$link->nativeHasDefaultValue() + && (!$link->nativeIsPromoted() || (!$link->nativeIsReadOnly() && !$link->nativeIsHooked())) + ) { + return null; } } - $propertyDescription = $this->propertyDescriptor->describeProperty($propertyReflection, $scope, $expr); - $propertyType = $propertyReflection->getWritableType(); + $propertyDescription = $this->propertyDescriptor->describeProperty($reflection, $scope, $propertyFetch); + $propertyType = $reflection->getWritableType(); if ($error !== null) { - if ($expr instanceof Node\Expr\PropertyFetch) { - return $this->check($expr->var, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error); - } - - if ($expr->class instanceof Expr) { - return $this->check($expr->class, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error); - } - - return $error; + return $inner !== null + ? $this->doCheck($inner, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error) + : $error; } if (!$this->checkAdvancedIsset) { - if ($expr instanceof Node\Expr\PropertyFetch) { - return $this->checkUndefined($expr->var, $scope, $operatorDescription, $identifier); - } - - if ($expr->class instanceof Expr) { - return $this->checkUndefined($expr->class, $scope, $operatorDescription, $identifier); - } - - return null; + return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier); } $error = $this->generateError( - $propertyReflection->getWritableType(), + $propertyType, sprintf('%s (%s) %s', $propertyDescription, $propertyType->describe(VerbosityLevel::typeOnly()), $operatorDescription), $typeMessageCallback, $identifier, 'property', ); - if ($error !== null) { - if ($expr instanceof Node\Expr\PropertyFetch) { - return $this->check($expr->var, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error); - } - - if ($expr->class instanceof Expr) { - return $this->check($expr->class, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error); - } + if ($error !== null && $inner !== null) { + return $this->doCheck($inner, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error); } return $error; } + // leaf - an arbitrary base expression that is not a chain link if ($error !== null) { return $error; } @@ -248,7 +214,7 @@ static function (Type $type) use ($typeMessageCallback): ?string { } $error = $this->generateError( - $this->treatPhpDocTypesAsCertain ? $scope->getScopeType($expr) : $scope->getScopeNativeType($expr), + $link->getValueType(), sprintf('Expression %s', $operatorDescription), $typeMessageCallback, $identifier, @@ -258,9 +224,10 @@ static function (Type $type) use ($typeMessageCallback): ?string { return $error; } - if ($expr instanceof Expr\NullsafePropertyFetch) { - if ($expr->name instanceof Node\Identifier) { - return RuleErrorBuilder::message(sprintf('Using nullsafe property access "?->%s" %s is unnecessary. Use -> instead.', $expr->name->name, $operatorDescription)) + if ($link->leafIsNullsafePropertyFetch()) { + $leafExpr = $link->getLeafExpr(); + if ($leafExpr instanceof NullsafePropertyFetch && $leafExpr->name instanceof Identifier) { + return RuleErrorBuilder::message(sprintf('Using nullsafe property access "?->%s" %s is unnecessary. Use -> instead.', $leafExpr->name->name, $operatorDescription)) ->identifier('nullsafe.neverNull') ->build(); } @@ -273,50 +240,46 @@ static function (Type $type) use ($typeMessageCallback): ?string { return null; } - /** - * @param ErrorIdentifier $identifier - */ - private function checkUndefined(Expr $expr, Scope $scope, string $operatorDescription, string $identifier): ?IdentifierRuleError + private function checkUndefinedInner(?IssetabilityResolution $resolution, MutatingScope $scope, string $operatorDescription, string $identifier): ?IdentifierRuleError { - if ($expr instanceof Node\Expr\Variable && is_string($expr->name)) { - $hasVariable = $scope->hasVariableType($expr->name); - if (!$hasVariable->no()) { + if ($resolution === null) { + return null; + } + + $link = $resolution->getLink(); + $inner = $resolution->getInner(); + + if ($link->isVariable()) { + if (!$link->getHasVariable()->no()) { return null; } - return RuleErrorBuilder::message(sprintf('Variable $%s %s is never defined.', $expr->name, $operatorDescription)) + return RuleErrorBuilder::message(sprintf('Variable $%s %s is never defined.', $link->getVariableName(), $operatorDescription)) ->identifier(sprintf('%s.variable', $identifier)) ->build(); } - if ($expr instanceof Node\Expr\ArrayDimFetch && $expr->dim !== null) { - $type = $this->treatPhpDocTypesAsCertain ? $scope->getScopeType($expr->var) : $scope->getScopeNativeType($expr->var); - $dimType = $this->treatPhpDocTypesAsCertain ? $scope->getScopeType($expr->dim) : $scope->getScopeNativeType($expr->dim); - $hasOffsetValue = $type->hasOffsetValueType($dimType); - if (!$type->isOffsetAccessible()->yes()) { - return $this->checkUndefined($expr->var, $scope, $operatorDescription, $identifier); + if ($link->isOffset()) { + if (!$link->getIsOffsetAccessible()->yes()) { + return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier); } - if (!$hasOffsetValue->no()) { - return $this->checkUndefined($expr->var, $scope, $operatorDescription, $identifier); + if (!$link->getHasOffsetValue()->no()) { + return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier); } return RuleErrorBuilder::message( sprintf( 'Offset %s on %s %s does not exist.', - $dimType->describe(VerbosityLevel::value()), - $type->describe(VerbosityLevel::value()), + $link->getDimType()->describe(VerbosityLevel::value()), + $link->getVarType()->describe(VerbosityLevel::value()), $operatorDescription, ), )->identifier(sprintf('%s.offset', $identifier))->build(); } - if ($expr instanceof Expr\PropertyFetch) { - return $this->checkUndefined($expr->var, $scope, $operatorDescription, $identifier); - } - - if ($expr instanceof Expr\StaticPropertyFetch && $expr->class instanceof Expr) { - return $this->checkUndefined($expr->class, $scope, $operatorDescription, $identifier); + if ($link->isProperty()) { + return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier); } return null; diff --git a/src/Rules/Variables/EmptyRule.php b/src/Rules/Variables/EmptyRule.php index d1656a3be27..b884cebddc3 100644 --- a/src/Rules/Variables/EmptyRule.php +++ b/src/Rules/Variables/EmptyRule.php @@ -5,12 +5,13 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; use PHPStan\DependencyInjection\RegisteredRule; +use PHPStan\Node\EmptyExpressionNode; use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Rule; use PHPStan\Type\Type; /** - * @implements Rule + * @implements Rule */ #[RegisteredRule(level: 1)] final class EmptyRule implements Rule @@ -22,12 +23,12 @@ public function __construct(private IssetCheck $issetCheck) public function getNodeType(): string { - return Node\Expr\Empty_::class; + return EmptyExpressionNode::class; } public function processNode(Node $node, Scope $scope): array { - $error = $this->issetCheck->check($node->expr, $scope, 'in empty()', 'empty', static function (Type $type): ?string { + $error = $this->issetCheck->check($node->getExprResult(), $scope, 'in empty()', 'empty', static function (Type $type): ?string { $isNull = $type->isNull(); if ($isNull->maybe()) { return null; diff --git a/src/Rules/Variables/IssetRule.php b/src/Rules/Variables/IssetRule.php index 839241a169b..cd9300c2dc5 100644 --- a/src/Rules/Variables/IssetRule.php +++ b/src/Rules/Variables/IssetRule.php @@ -5,12 +5,13 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; use PHPStan\DependencyInjection\RegisteredRule; +use PHPStan\Node\IssetExpressionNode; use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Rule; use PHPStan\Type\Type; /** - * @implements Rule + * @implements Rule */ #[RegisteredRule(level: 1)] final class IssetRule implements Rule @@ -22,14 +23,14 @@ public function __construct(private IssetCheck $issetCheck) public function getNodeType(): string { - return Node\Expr\Isset_::class; + return IssetExpressionNode::class; } public function processNode(Node $node, Scope $scope): array { $messages = []; - foreach ($node->vars as $var) { - $error = $this->issetCheck->check($var, $scope, 'in isset()', 'isset', static function (Type $type): ?string { + foreach ($node->getVarResults() as $varResult) { + $error = $this->issetCheck->check($varResult, $scope, 'in isset()', 'isset', static function (Type $type): ?string { $isNull = $type->isNull(); if ($isNull->maybe()) { return null; diff --git a/src/Rules/Variables/NullCoalesceRule.php b/src/Rules/Variables/NullCoalesceRule.php index 6250b59b906..409ae4e431d 100644 --- a/src/Rules/Variables/NullCoalesceRule.php +++ b/src/Rules/Variables/NullCoalesceRule.php @@ -6,6 +6,7 @@ use PHPStan\Analyser\Scope; use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\RegisteredRule; +use PHPStan\Node\CoalesceExpressionNode; use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Rule; @@ -14,7 +15,7 @@ use function sprintf; /** - * @implements Rule + * @implements Rule */ #[RegisteredRule(level: 1)] final class NullCoalesceRule implements Rule @@ -30,42 +31,35 @@ public function __construct( public function getNodeType(): string { - return Node\Expr::class; + return CoalesceExpressionNode::class; } public function processNode(Node $node, Scope $scope): array { - $typeMessageCallback = static function (Type $type): ?string { - $isNull = $type->isNull(); - if ($isNull->maybe()) { - return null; - } - - if ($isNull->yes()) { - return 'is always null'; - } - - return 'is not nullable'; - }; - - if ($node instanceof Node\Expr\BinaryOp\Coalesce) { - $left = $node->left; - $right = $node->right; - $operator = '??'; - } elseif ($node instanceof Node\Expr\AssignOp\Coalesce) { - $left = $node->var; - $right = $node->expr; - $operator = '??='; - } else { - return []; - } + $error = $this->issetCheck->check( + $node->getSubjectResult(), + $scope, + $node->getOperatorDescription(), + 'nullCoalesce', + static function (Type $type): ?string { + $isNull = $type->isNull(); + if ($isNull->maybe()) { + return null; + } + + if ($isNull->yes()) { + return 'is always null'; + } + + return 'is not nullable'; + }, + ); - $error = $this->issetCheck->check($left, $scope, sprintf('on left side of %s', $operator), 'nullCoalesce', $typeMessageCallback); if ($error !== null) { return [$error]; } - $unnecessaryError = $this->checkUnnecessaryNullCoalesce($left, $right, $operator, $scope); + $unnecessaryError = $this->checkUnnecessaryNullCoalesce($node, $scope); if ($unnecessaryError !== null) { return [$unnecessaryError]; } @@ -73,12 +67,23 @@ public function processNode(Node $node, Scope $scope): array return []; } - private function checkUnnecessaryNullCoalesce(Node\Expr $left, Node\Expr $right, string $operator, Scope $scope): ?IdentifierRuleError + private function checkUnnecessaryNullCoalesce(CoalesceExpressionNode $node, Scope $scope): ?IdentifierRuleError { if (!$this->unnecessaryNullCoalesce) { return null; } + $originalExpr = $node->getOriginalExpr(); + if ($originalExpr instanceof Node\Expr\BinaryOp\Coalesce) { + $right = $originalExpr->right; + $operator = '??'; + } elseif ($originalExpr instanceof Node\Expr\AssignOp\Coalesce) { + $right = $originalExpr->expr; + $operator = '??='; + } else { + return null; + } + if (!$scope->getType($right)->isNull()->yes()) { return null; } @@ -86,7 +91,8 @@ private function checkUnnecessaryNullCoalesce(Node\Expr $left, Node\Expr $right, // The coalesce only changes the result when the left side is undefined. // If the left side is always set, `?? null` (or `??= null`) never changes // anything, so the whole coalesce is redundant. - if ($scope->toMutatingScope()->issetCheck($left, static fn (): bool => true) !== true) { + $resolution = $node->getSubjectResult()->getIssetabilityResolution($scope->toMutatingScope(), false); + if ($resolution->isSet(static fn (): bool => true) !== true) { return null; } diff --git a/tests/PHPStan/Rules/Variables/EmptyRuleTest.php b/tests/PHPStan/Rules/Variables/EmptyRuleTest.php index 582fdeb1076..079b032e8ee 100644 --- a/tests/PHPStan/Rules/Variables/EmptyRuleTest.php +++ b/tests/PHPStan/Rules/Variables/EmptyRuleTest.php @@ -4,7 +4,6 @@ use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Properties\PropertyDescriptor; -use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; use PHPUnit\Framework\Attributes\DataProvider; @@ -22,7 +21,6 @@ protected function getRule(): Rule { return new EmptyRule(new IssetCheck( new PropertyDescriptor(), - new PropertyReflectionFinder(), true, $this->treatPhpDocTypesAsCertain, )); diff --git a/tests/PHPStan/Rules/Variables/IssetRuleTest.php b/tests/PHPStan/Rules/Variables/IssetRuleTest.php index 7e83c53117b..af8f1542e04 100644 --- a/tests/PHPStan/Rules/Variables/IssetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/IssetRuleTest.php @@ -4,7 +4,6 @@ use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Properties\PropertyDescriptor; -use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; use PHPUnit\Framework\Attributes\RequiresPhp; @@ -21,7 +20,6 @@ protected function getRule(): Rule { return new IssetRule(new IssetCheck( new PropertyDescriptor(), - new PropertyReflectionFinder(), true, $this->treatPhpDocTypesAsCertain, )); diff --git a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php index fc6c577822d..17d1dd85949 100644 --- a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php +++ b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php @@ -4,7 +4,6 @@ use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Properties\PropertyDescriptor; -use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; use PHPUnit\Framework\Attributes\RequiresPhp; @@ -20,7 +19,6 @@ protected function getRule(): Rule { return new NullCoalesceRule(new IssetCheck( new PropertyDescriptor(), - new PropertyReflectionFinder(), true, $this->shouldTreatPhpDocTypesAsCertain(), ), true);