Skip to content

support removing class-strings from GenericClassStringType#1590

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

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

Conversation

@staabm

@staabm staabm commented Aug 4, 2022

Copy link
Copy Markdown
Contributor

@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

Dgame commented Aug 4, 2022

Copy link
Copy Markdown

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

Dgame commented Aug 4, 2022

Copy link
Copy Markdown

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

@staabm

staabm commented Aug 4, 2022

Copy link
Copy Markdown
Contributor Author

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

staabm commented Aug 5, 2022

Copy link
Copy Markdown
Contributor Author

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

staabm commented Aug 5, 2022

Copy link
Copy Markdown
Contributor Author

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

phpstan/phpstan#7746

@Dgame

Dgame commented Aug 5, 2022

Copy link
Copy Markdown

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

@staabm

staabm commented Aug 5, 2022

Copy link
Copy Markdown
Contributor Author

@Dgame

Dgame commented Aug 5, 2022

Copy link
Copy Markdown

@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

staabm commented Aug 5, 2022

Copy link
Copy Markdown
Contributor Author

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

@staabm staabm Aug 5, 2022

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.

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

Dgame commented Aug 5, 2022

Copy link
Copy Markdown

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