Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support removing class-strings from GenericClassStringType #1590

Merged
merged 6 commits into from Aug 5, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Aug 4, 2022

@ondrejmirtes
Copy link
Member

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

@Dgame
Copy link

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

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
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
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
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

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
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

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
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
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
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
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
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
Member

Thank you.

@Dgame
Copy link

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
3 participants