Skip to content

Fix ClassConstFetch Identical specification with Yoda conditions#2498

Merged
ondrejmirtes merged 3 commits intophpstan:1.10.xfrom
herndlm:fix-yoda-class-const-fetch-identical
Jun 30, 2023
Merged

Fix ClassConstFetch Identical specification with Yoda conditions#2498
ondrejmirtes merged 3 commits intophpstan:1.10.xfrom
herndlm:fix-yoda-class-const-fetch-identical

Conversation

@herndlm
Copy link
Copy Markdown
Contributor

@herndlm herndlm commented Jun 30, 2023 •
edited
Loading

Closes phpstan/phpstan#9542

Is this how you would have done it? Might be the simplest way I guess. It's just an adapted version to handle the left/right mixed variant of the block directly before it.

UPDATE: I had the urge to somehow simplify/cleanup things, like the way left and right types are fetched from the scope, assigned to variables and used, in Identical in a follow-up but I failed. Better not touch it too much 😅

@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Jun 30, 2023

Hi, I'd be happier with NodeScopeResolverTest. We can leave the rule test but it'd be best to check the type inference (truthy/falsy context too). Thanks!

@herndlm herndlm force-pushed the fix-yoda-class-const-fetch-identical branch from ac0b40b to 1250516 Compare June 30, 2023 12:16
@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Jun 30, 2023 •
edited
Loading

sure. added it.
UPDATE: missed the falsy context assertions. addem them too now..

@herndlm herndlm force-pushed the fix-yoda-class-const-fetch-identical branch from 1250516 to 7ee61dd Compare June 30, 2023 12:26
ondrejmirtes
ondrejmirtes requested changes Jun 30, 2023
View reviewed changes
tests/PHPStan/Rules/Methods/data/bug-9542.php Outdated
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes Jun 30, 2023

Choose a reason for hiding this comment

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

There's a bad bug here. So you've eliminated that $translatable isn't TranslatableMessage. But it can still be a subclass of TranslatableMessage.

Most likely we should only specify the type for $context->true().

Same thing is done for get_class:

phpstan-src/src/Analyser/TypeSpecifier.php

Lines 194 to 210 in 2df1a59

if (
$context->true()
&& $unwrappedLeftExpr instanceof FuncCall
&& $unwrappedLeftExpr->name instanceof Name
&& strtolower($unwrappedLeftExpr->name->toString()) === 'get_class'
&& isset($unwrappedLeftExpr->getArgs()[0])
) {
if ($rightType->getClassStringObjectType()->isObject()->yes()) {
return $this->create(
$unwrappedLeftExpr->getArgs()[0]->value,
$rightType->getClassStringObjectType(),
$context,
false,
$scope,
$rootExpr,
)->unionWith($this->create($leftExpr, $rightType, $context, false, $scope, $rootExpr));
}

Copy link
Copy Markdown
Contributor

@staabm staabm Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

could we be "smarter" if TranslatableMessage is a reference to a final class (or a interface)?

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes Jun 30, 2023

Choose a reason for hiding this comment

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

We could, but first we need to get rid of the bug, then we can improve it :)

Copy link
Copy Markdown
Contributor Author

@herndlm herndlm Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

adapted. better? I didn't even notice this.. :)

Copy link
Copy Markdown
Contributor Author

@herndlm herndlm Jun 30, 2023

Choose a reason for hiding this comment

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

oh sorry, I was lazy and didn't run the full tests :/

Copy link
Copy Markdown
Contributor Author

@herndlm herndlm Jun 30, 2023

Choose a reason for hiding this comment

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

thx for the fix!

@herndlm herndlm requested a review from ondrejmirtes June 30, 2023 19:02
@herndlm herndlm force-pushed the fix-yoda-class-const-fetch-identical branch from fe58422 to 446ee63 Compare June 30, 2023 19:09
@ondrejmirtes ondrejmirtes merged commit 1b47baf into phpstan:1.10.x Jun 30, 2023
@ondrejmirtes ondrejmirtes mentioned this pull request Jun 30, 2023
Closed
@herndlm herndlm deleted the fix-yoda-class-const-fetch-identical branch June 30, 2023 20:21
@Seb33300 Seb33300 mentioned this pull request Jul 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@staabm staabm staabm left review comments

@ondrejmirtes ondrejmirtes Awaiting requested review from ondrejmirtes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@herndlm @ondrejmirtes @staabm