Fix chaining nullsafe operator

phpstan · Nov 6, 2021 · 6bb0fd6 · 6bb0fd6
1 parent e3a0e0d
commit 6bb0fd6
Show file tree

Hide file tree

Showing 16 changed files with 147 additions and 11 deletions.
diff --git a/src/Analyser/NullsafeOperatorHelper.php b/src/Analyser/NullsafeOperatorHelper.php
@@ -3,10 +3,25 @@
 namespace PHPStan\Analyser;
 
 use PhpParser\Node\Expr;
+use PHPStan\Type\TypeCombinator;
 
 class NullsafeOperatorHelper
 {
 
+	public static function getNullsafeShortcircuitedExprRespectingScope(Scope $scope, Expr $expr): Expr
+	{
+		if (!TypeCombinator::containsNull($scope->getType($expr))) {
+			// We're in most likely in context of a null-safe operator ($scope->moreSpecificType is defined for $expr)
+			// Modifying the expression would not bring any value or worse ruin the context information
+			return $expr;
+		}
+
+		return self::getNullsafeShortcircuitedExpr($expr);
+	}
+
+	/**
+	 * @internal Use NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope
+	 */
 	public static function getNullsafeShortcircuitedExpr(Expr $expr): Expr
 	{
 		if ($expr instanceof Expr\NullsafeMethodCall) {

diff --git a/src/Rules/Arrays/NonexistentOffsetInArrayDimFetchCheck.php b/src/Rules/Arrays/NonexistentOffsetInArrayDimFetchCheck.php
@@ -44,7 +44,7 @@ public function check(
 	{
 		$typeResult = $this->ruleLevelHelper->findTypeToCheck(
 			$scope,
-			NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($var),
+			NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $var),
 			$unknownClassPattern,
 			static function (Type $type) use ($dimType): bool {
 				return $type->hasOffsetValueType($dimType)->yes();

diff --git a/src/Rules/Arrays/NonexistentOffsetInArrayDimFetchRule.php b/src/Rules/Arrays/NonexistentOffsetInArrayDimFetchRule.php
@@ -51,7 +51,7 @@ public function processNode(\PhpParser\Node $node, Scope $scope): array
 
 		$isOffsetAccessibleTypeResult = $this->ruleLevelHelper->findTypeToCheck(
 			$scope,
-			NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($node->var),
+			NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $node->var),
 			$unknownClassPattern,
 			static function (Type $type): bool {
 				return $type->isOffsetAccessible()->yes();

diff --git a/src/Rules/Arrays/OffsetAccessAssignmentRule.php b/src/Rules/Arrays/OffsetAccessAssignmentRule.php
@@ -42,7 +42,7 @@ public function processNode(\PhpParser\Node $node, Scope $scope): array
 
 		$varTypeResult = $this->ruleLevelHelper->findTypeToCheck(
 			$scope,
-			NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($node->var),
+			NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $node->var),
 			'',
 			static function (Type $varType) use ($potentialDimType): bool {
 				$arrayDimType = $varType->setOffsetValueType($potentialDimType, new MixedType());

diff --git a/src/Rules/Classes/ClassConstantRule.php b/src/Rules/Classes/ClassConstantRule.php
@@ -119,7 +119,7 @@ public function processNode(Node $node, Scope $scope): array
 		} else {
 			$classTypeResult = $this->ruleLevelHelper->findTypeToCheck(
 				$scope,
-				NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($class),
+				NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $class),
 				sprintf('Access to constant %s on an unknown class %%s.', SprintfHelper::escapeFormatString($constantName)),
 				static function (Type $type) use ($constantName): bool {
 					return $type->canAccessConstants()->yes() && $type->hasConstant($constantName)->yes();

diff --git a/src/Rules/Functions/CallCallablesRule.php b/src/Rules/Functions/CallCallablesRule.php
@@ -54,7 +54,7 @@ public function processNode(
 
 		$typeResult = $this->ruleLevelHelper->findTypeToCheck(
 			$scope,
-			NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($node->name),
+			NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $node->name),
 			'Invoking callable on an unknown class %s.',
 			static function (Type $type): bool {
 				return $type->isCallable()->yes();

diff --git a/src/Rules/Methods/CallMethodsRule.php b/src/Rules/Methods/CallMethodsRule.php
@@ -62,12 +62,13 @@ public function processNode(Node $node, Scope $scope): array
 
 		$typeResult = $this->ruleLevelHelper->findTypeToCheck(
 			$scope,
-			NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($node->var),
+			NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $node->var),
 			sprintf('Call to method %s() on an unknown class %%s.', SprintfHelper::escapeFormatString($name)),
 			static function (Type $type) use ($name): bool {
 				return $type->canCallMethods()->yes() && $type->hasMethod($name)->yes();
 			}
 		);
+
 		$type = $typeResult->getType();
 		if ($type instanceof ErrorType) {
 			return $typeResult->getUnknownClassErrors();

diff --git a/src/Rules/Methods/CallStaticMethodsRule.php b/src/Rules/Methods/CallStaticMethodsRule.php
@@ -151,7 +151,7 @@ public function processNode(Node $node, Scope $scope): array
 		} else {
 			$classTypeResult = $this->ruleLevelHelper->findTypeToCheck(
 				$scope,
-				NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($class),
+				NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $class),
 				sprintf('Call to static method %s() on an unknown class %%s.', SprintfHelper::escapeFormatString($methodName)),
 				static function (Type $type) use ($methodName): bool {
 					return $type->canCallMethods()->yes() && $type->hasMethod($methodName)->yes();

diff --git a/src/Rules/Methods/CallToMethodStatementWithoutSideEffectsRule.php b/src/Rules/Methods/CallToMethodStatementWithoutSideEffectsRule.php
@@ -47,7 +47,7 @@ public function processNode(Node $node, Scope $scope): array
 
 		$typeResult = $this->ruleLevelHelper->findTypeToCheck(
 			$scope,
-			NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($methodCall->var),
+			NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $methodCall->var),
 			'',
 			static function (Type $type) use ($methodName): bool {
 				return $type->canCallMethods()->yes() && $type->hasMethod($methodName)->yes();

diff --git a/src/Rules/Methods/CallToStaticMethodStatementWithoutSideEffectsRule.php b/src/Rules/Methods/CallToStaticMethodStatementWithoutSideEffectsRule.php
@@ -61,7 +61,7 @@ public function processNode(Node $node, Scope $scope): array
 		} else {
 			$typeResult = $this->ruleLevelHelper->findTypeToCheck(
 				$scope,
-				NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($staticCall->class),
+				NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $staticCall->class),
 				'',
 				static function (Type $type) use ($methodName): bool {
 					return $type->canCallMethods()->yes() && $type->hasMethod($methodName)->yes();

diff --git a/src/Rules/Properties/AccessPropertiesRule.php b/src/Rules/Properties/AccessPropertiesRule.php
@@ -73,7 +73,7 @@ private function processSingleProperty(Scope $scope, PropertyFetch $node, string
 	{
 		$typeResult = $this->ruleLevelHelper->findTypeToCheck(
 			$scope,
-			NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($node->var),
+			NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $node->var),
 			sprintf('Access to property $%s on an unknown class %%s.', SprintfHelper::escapeFormatString($name)),
 			static function (Type $type) use ($name): bool {
 				return $type->canAccessProperties()->yes() && $type->hasProperty($name)->yes();

diff --git a/src/Rules/Properties/AccessStaticPropertiesRule.php b/src/Rules/Properties/AccessStaticPropertiesRule.php
@@ -147,7 +147,7 @@ private function processSingleProperty(Scope $scope, StaticPropertyFetch $node,
 		} else {
 			$classTypeResult = $this->ruleLevelHelper->findTypeToCheck(
 				$scope,
-				NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($node->class),
+				NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $node->class),
 				sprintf('Access to static property $%s on an unknown class %%s.', SprintfHelper::escapeFormatString($name)),
 				static function (Type $type) use ($name): bool {
 					return $type->canAccessProperties()->yes() && $type->hasProperty($name)->yes();

diff --git a/tests/PHPStan/Rules/Methods/CallMethodsRuleTest.php b/tests/PHPStan/Rules/Methods/CallMethodsRuleTest.php
@@ -2193,4 +2193,29 @@ public function testBug3465(): void
 		$this->analyse([__DIR__ . '/data/bug-3465.php'], []);
 	}
 
+	public function testBug5868(): void
+	{
+		if (PHP_VERSION_ID < 80000) {
+			$this->markTestSkipped('Test requires PHP 8.0');
+		}
+
+		$this->checkThisOnly = false;
+		$this->checkNullables = true;
+		$this->checkUnionTypes = true;
+		$this->analyse([__DIR__ . '/data/bug-5868.php'], [
+			[
+				'Cannot call method nullable1() on Bug5868\HelloWorld|null.',
+				14,
+			],
+			[
+				'Cannot call method nullable2() on Bug5868\HelloWorld|null.',
+				15,
+			],
+			[
+				'Cannot call method nullable3() on Bug5868\HelloWorld|null.',
+				16,
+			],
+		]);
+	}
+
 }
diff --git a/tests/PHPStan/Rules/Methods/data/bug-5868.php b/tests/PHPStan/Rules/Methods/data/bug-5868.php
@@ -0,0 +1,30 @@
+<?php // lint >= 8.0
+
+namespace Bug5868;
+
+class HelloWorld
+{
+	public function nullable1(): ?self
+	{
+		// OK
+		$tmp = $this->nullable1()?->nullable1()?->nullable2();
+		$tmp = $this->nullable1()?->nullable3()->nullable2()?->nullable3()->nullable1();
+
+		// Error
+		$tmp = $this->nullable1()->nullable1()?->nullable2();
+		$tmp = $this->nullable1()?->nullable1()->nullable2();
+		$tmp = $this->nullable1()?->nullable3()->nullable2()->nullable3()->nullable1();
+
+		return $this->nullable1()?->nullable3();
+	}
+
+	public function nullable2(): ?self
+	{
+		return $this;
+	}
+
+	public function nullable3(): self
+	{
+		return $this;
+	}
+}
diff --git a/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php b/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php
@@ -491,4 +491,32 @@ public function testBug4808(): void
 		$this->analyse([__DIR__ . '/data/bug-4808.php'], []);
 	}
 
+
+	public function testBug5868(): void
+	{
+		if (PHP_VERSION_ID < 80000) {
+			$this->markTestSkipped('Test requires PHP 8.0');
+		}
+		$this->checkThisOnly = false;
+		$this->checkUnionTypes = true;
+		$this->analyse([__DIR__ . '/data/bug-5868.php'], [
+			[
+				'Cannot access property $child on Bug5868PropertyFetch\Foo|null.',
+				31,
+			],
+			[
+				'Cannot access property $child on Bug5868PropertyFetch\Child|null.',
+				32,
+			],
+			[
+				'Cannot access property $existingChild on Bug5868PropertyFetch\Child|null.',
+				33,
+			],
+			[
+				'Cannot access property $existingChild on Bug5868PropertyFetch\Child|null.',
+				34,
+			],
+		]);
+	}
+
 }
diff --git a/tests/PHPStan/Rules/Properties/data/bug-5868.php b/tests/PHPStan/Rules/Properties/data/bug-5868.php
@@ -0,0 +1,37 @@
+<?php // lint >= 8.0
+
+namespace Bug5868PropertyFetch;
+
+class Child
+{
+
+    public ?self $child;
+
+	public self $existingChild;
+
+}
+
+class Foo
+{
+	public ?Child $child;
+}
+
+class HelloWorld
+{
+
+	function getAttributeInNode(?Foo $node): ?Child
+	{
+		// Ok
+		$tmp = $node?->child;
+		$tmp = $node?->child?->child?->child;
+		$tmp = $node?->child?->existingChild->child;
+		$tmp = $node?->child?->existingChild->child?->existingChild;
+
+		// Errors
+		$tmp = $node->child;
+		$tmp = $node?->child->child;
+		$tmp = $node?->child->existingChild->child;
+		$tmp = $node?->child?->existingChild->child->existingChild;
+	}
+
+}