Fix/issue 5351 2 #1091

rajyan · 2022-03-20T15:52:37Z

Issue was not fixed by #1063, becauseensureShallowNonNullability is not handling optional keys. This PR specifies type for ArrayDimFetch to make the nonNullable scope more precise.

ondrejmirtes · 2022-03-21T12:48:14Z

tests/PHPStan/Analyser/data/bug-4885.php

+
+class Foo
+{
+	/** @param array{word?: string} $data */


Please also test array{word?: string|null}.

ondrejmirtes · 2022-03-21T12:52:40Z

src/Analyser/NodeScopeResolver.php

-			);
+		if ($exprToSpecify instanceof ArrayDimFetch && $exprToSpecify->dim !== null) {
+			$dimFetchVarType = $scope->getType($exprToSpecify->var);
+			if ($dimFetchVarType instanceof ConstantArrayType) {


I worry this logic isn't really sufficient. You can also have a union of constant arrays. And I guess it should also do something aboout nested ArrayDimFetch - $a['a']['b']['c'] where the intermediate keys are also optional...

I feel like instead of reimplementing the logic here, this place should call TypeSpecifier because the place where it handles Isset_ looks like it has this covered:

phpstan-src/src/Analyser/TypeSpecifier.php

Lines 742 to 826 in 5c48a18

} elseif (

$expr instanceof Expr\Isset_

&& count($expr->vars) > 0

&& $context->true()

) {

$vars = [];

foreach ($expr->vars as $var) {

$tmpVars = [$var];

while (

$var instanceof ArrayDimFetch

|| $var instanceof PropertyFetch

|| (

$var instanceof StaticPropertyFetch

&& $var->class instanceof Expr

)

) {

if ($var instanceof StaticPropertyFetch) {

/** @var Expr $var */

$var = $var->class;

} else {

$var = $var->var;

}

$tmpVars[] = $var;

}

$vars = array_merge($vars, array_reverse($tmpVars));

}

if (count($vars) === 0) {

throw new ShouldNotHappenException();

}

$types = null;

foreach ($vars as $var) {

if ($var instanceof Expr\Variable && is_string($var->name)) {

if ($scope->hasVariableType($var->name)->no()) {

return new SpecifiedTypes([], []);

}

}

if (

$var instanceof ArrayDimFetch

&& $var->dim !== null

&& !$scope->getType($var->var) instanceof MixedType

) {

$type = $this->create(

$var->var,

new HasOffsetType($scope->getType($var->dim)),

$context,

false,

$scope,

)->unionWith(

$this->create($var, new NullType(), TypeSpecifierContext::createFalse(), false, $scope),

);

} else {

$type = $this->create($var, new NullType(), TypeSpecifierContext::createFalse(), false, $scope);

}

if (

$var instanceof PropertyFetch

&& $var->name instanceof Node\Identifier

) {

$type = $type->unionWith($this->create($var->var, new IntersectionType([

new ObjectWithoutClassType(),

new HasPropertyType($var->name->toString()),

]), TypeSpecifierContext::createTruthy(), false, $scope));

} elseif (

$var instanceof StaticPropertyFetch

&& $var->class instanceof Expr

&& $var->name instanceof Node\VarLikeIdentifier

) {

$type = $type->unionWith($this->create($var->class, new IntersectionType([

new ObjectWithoutClassType(),

new HasPropertyType($var->name->toString()),

]), TypeSpecifierContext::createTruthy(), false, $scope));

}

if ($types === null) {

$types = $type;

} else {

$types = $types->unionWith($type);

}

}

return $types;

So the scope after $x ?? throw should be something like $scope->filterByTruthyValue(new Isset_($expr->left)) (which delegates the call to TypeSpecifier internally).

rajyan · 2022-03-21T13:03:13Z

@ondrejmirtes
Thank you for the review.
I was having the same feeling and thought maybe the whole Coalesce process can be replaced with isset($leftExpr) ? $leftExpr : $rightExpr. With this, the scope should work perfect with NeverType and can filter by TruthyScope without any additional implementation (Also, #1063 can be reverted I think). Although, early termination for non Stmt is not working now, phpstan/phpstan#6870
so maybe I would fix that first.

ondrejmirtes · 2022-03-21T13:22:00Z

Try whatever you think should work 😊

rajyan · 2022-03-22T15:58:38Z

move to #1104

rajyan force-pushed the fix/issue-5351-2 branch from 6af6b8f to 5f74f45 Compare March 20, 2022 16:04

rajyan added 3 commits March 21, 2022 01:19

add failing test case for issue-4885

a6ab0c4

add non nullability for ArrayDimFetch optional keys

956171b

throw expression is allowed from PHP8.0

ba63d34

rajyan force-pushed the fix/issue-5351-2 branch from 5f74f45 to ba63d34 Compare March 20, 2022 16:19

ondrejmirtes requested changes Mar 21, 2022

View reviewed changes

rajyan marked this pull request as draft March 21, 2022 13:03

rajyan added a commit to rajyan/phpstan-src that referenced this pull request Mar 22, 2022

pick tests from phpstan#1091

e3fde53

rajyan closed this Mar 22, 2022

rajyan deleted the fix/issue-5351-2 branch March 24, 2022 02:27

rajyan added a commit to rajyan/phpstan-src that referenced this pull request Mar 24, 2022

pick tests from phpstan#1091

e4997e2

rajyan added a commit to rajyan/phpstan-src that referenced this pull request Apr 9, 2022

pick tests from phpstan#1091

2a8d228

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix/issue 5351 2 #1091

Fix/issue 5351 2 #1091

Uh oh!

rajyan commented Mar 20, 2022 •

edited

Loading

Uh oh!

ondrejmirtes Mar 21, 2022

Uh oh!

ondrejmirtes Mar 21, 2022

Uh oh!

rajyan commented Mar 21, 2022

Uh oh!

ondrejmirtes commented Mar 21, 2022

Uh oh!

rajyan commented Mar 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	} elseif (
	$expr instanceof Expr\Isset_
	&& count($expr->vars) > 0
	&& $context->true()
	) {
	$vars = [];
	foreach ($expr->vars as $var) {
	$tmpVars = [$var];

	while (
	$var instanceof ArrayDimFetch
	\|\| $var instanceof PropertyFetch
	\|\| (
	$var instanceof StaticPropertyFetch
	&& $var->class instanceof Expr
	)
	) {
	if ($var instanceof StaticPropertyFetch) {
	/** @var Expr $var */
	$var = $var->class;
	} else {
	$var = $var->var;
	}
	$tmpVars[] = $var;
	}

	$vars = array_merge($vars, array_reverse($tmpVars));
	}

	if (count($vars) === 0) {
	throw new ShouldNotHappenException();
	}

	$types = null;
	foreach ($vars as $var) {
	if ($var instanceof Expr\Variable && is_string($var->name)) {
	if ($scope->hasVariableType($var->name)->no()) {
	return new SpecifiedTypes([], []);
	}
	}
	if (
	$var instanceof ArrayDimFetch
	&& $var->dim !== null
	&& !$scope->getType($var->var) instanceof MixedType
	) {
	$type = $this->create(
	$var->var,
	new HasOffsetType($scope->getType($var->dim)),
	$context,
	false,
	$scope,
	)->unionWith(
	$this->create($var, new NullType(), TypeSpecifierContext::createFalse(), false, $scope),
	);
	} else {
	$type = $this->create($var, new NullType(), TypeSpecifierContext::createFalse(), false, $scope);
	}

	if (
	$var instanceof PropertyFetch
	&& $var->name instanceof Node\Identifier
	) {
	$type = $type->unionWith($this->create($var->var, new IntersectionType([
	new ObjectWithoutClassType(),
	new HasPropertyType($var->name->toString()),
	]), TypeSpecifierContext::createTruthy(), false, $scope));
	} elseif (
	$var instanceof StaticPropertyFetch
	&& $var->class instanceof Expr
	&& $var->name instanceof Node\VarLikeIdentifier
	) {
	$type = $type->unionWith($this->create($var->class, new IntersectionType([
	new ObjectWithoutClassType(),
	new HasPropertyType($var->name->toString()),
	]), TypeSpecifierContext::createTruthy(), false, $scope));
	}

	if ($types === null) {
	$types = $type;
	} else {
	$types = $types->unionWith($type);
	}
	}

	return $types;

Fix/issue 5351 2 #1091

Fix/issue 5351 2 #1091

Uh oh!

Conversation

rajyan commented Mar 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

rajyan commented Mar 21, 2022

Uh oh!

ondrejmirtes commented Mar 21, 2022

Uh oh!

rajyan commented Mar 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rajyan commented Mar 20, 2022 •

edited

Loading