Skip to content

support removing class-strings from GenericClassStringType#1590

Merged
ondrejmirtes merged 6 commits intophpstan:1.8.xfrom
staabm:class-string
Aug 5, 2022
Merged

support removing class-strings from GenericClassStringType#1590
ondrejmirtes merged 6 commits intophpstan:1.8.xfrom
staabm:class-string

Conversation

@staabm
Copy link
Copy Markdown
Contributor

@staabm staabm commented Aug 4, 2022

@ondrejmirtes
Copy link
Copy Markdown
Member

Please also add a regression test for the match expression, there's also some example with $obj::class I think.

@Dgame
Copy link
Copy Markdown

Dgame commented Aug 4, 2022

Here is a smaller example: https://phpstan.org/r/e4476215-99ea-434f-adae-3564ef93a5a5

If this works I really have to take a look how the semantics work, I'm not sure I understand the NeverType there.

@Dgame
Copy link
Copy Markdown

Dgame commented Aug 4, 2022

Not sure if this is related in this case, but have another test case there: phpstan/phpstan#7721

@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Aug 4, 2022

just added the match-test. its still failling though.

I think Dgame is right, that there is a missing piece in MatchExpressionRule. I need to invest more time, to get an idea how this match-arm analysis is working...
it seems MatchExpressionRule is successfully removing all match-arm expressions from the match condition, but instead of a NeverType it leads to a empty ClassStringType atm

@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Aug 5, 2022

ok, after sleeping a night over it, I know understood the basic building blocks of the arm analysis.

we more or less rewrite the AST in

$armConditionExpr = new Node\Expr\BinaryOp\Identical(
$matchCondition,
$armCondition->getCondition(),
);

which adds similar IF-identical expressions we already have in https://github.com/phpstan/phpstan-src/pull/1590/files#diff-9ca94a9a28f6d393c869ef34bde57e4bb0d53dcc9a8cb5fc865f074ef0df5348R30-R32

with this knowledge I was able to fix the test with commit 21e5bcf.


still, I think the match rule contains a bug which makes it only work when the match condition is a variable.
as soon as you put directly a expression into the match, it does not work atm:

works:

	$t = new Test(new A());
	$class = $t->value::class;
	echo match ($class) {
		A::class => 'A',
		B::class => 'B'
	};

doesn't work:

	$t = new Test(new A());
	echo match ($t->value::class) {
		A::class => 'A',
		B::class => 'B'
	};

I am still figuring out the details.

@staabm staabm marked this pull request as ready for review August 5, 2022 05:52
@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Aug 5, 2022

I put my finding into a new issue, as I think this one can be merged as is?

phpstan/phpstan#7746

@Dgame
Copy link
Copy Markdown

Dgame commented Aug 5, 2022

Thanks @staabm! That a good step forward. 🙂 Could we call tryRemove in the MatchExpressionRule in case the match-condition is an expression?

@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Aug 5, 2022

issue phpstan/phpstan#7746 is not related to the MatchRule. its a more general problem, see e.g. https://phpstan.org/r/be1b13ce-b8d9-445e-b6ed-dc9e87f2879b

@Dgame
Copy link
Copy Markdown

Dgame commented Aug 5, 2022

@staabm Here is a more reduced example which does not work: https://phpstan.org/r/0a06314d-d25e-4435-8aa1-d60bb41b8fda
It does not seems to work if I store the class-name in a variable.

@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Aug 5, 2022

phpstan.org/try does not yet contain the patch, as the PR is not yet merged.
taking your example and running it against phpstan with the PR applied works for me as expected locally (no errors)

if ($typeToRemove instanceof ConstantStringType && $typeToRemove->isClassString()) {
$generic = $this->getGenericType();

if ($generic instanceof ObjectType) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeWithClassName pls

Copy link
Copy Markdown
Contributor Author

@staabm staabm Aug 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that while PR creation and it made some tests fail.

Therefore I went back to ObjectType

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a red flag, we should investigate that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - seems to work now. maybe it was some intermediate step which made it fail…

@ondrejmirtes ondrejmirtes merged commit 2ea6efe into phpstan:1.8.x Aug 5, 2022
@ondrejmirtes
Copy link
Copy Markdown
Member

Thank you.

@Dgame
Copy link
Copy Markdown

Dgame commented Aug 5, 2022

Thanks a lot @staabm !

@staabm staabm deleted the class-string branch June 21, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants