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

Accept ::class as literal string #1692

Merged
merged 14 commits into from Sep 20, 2022
Merged

Accept ::class as literal string #1692

merged 14 commits into from Sep 20, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 4, 2022

@staabm staabm marked this pull request as ready for review Sep 4, 2022
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Sep 4, 2022

I disagree with this fix. class-string isn't always literal string.
Rather than that, $x::class should be an intersection with literal-string. You can implement that in MutatingScope::resolveType() and also verify that in NodeScopeResolverTest.

@VincentLanglet
Copy link
Contributor

VincentLanglet commented Sep 5, 2022

I disagree with this fix. class-string isn't always literal string.

@craigfrancis didn't give an example of class-string which are not literal-string in phpstan/phpstan-doctrine#332 (comment)

But I think we should follow the way it's handled by psalm
https://psalm.dev/r/16085bfd30

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Sep 5, 2022

// $s is string
if (class_exists($s)) {
    // should $s suddenly be a literal-string?
}

@craigfrancis
Copy link
Contributor

craigfrancis commented Sep 5, 2022

[I’m out atm, and using phone, will try to check properly later]

The source of the variable has to be a literal, so if you got it from example::class, that’s fine… but if you’re checking the value contains a string that happens to match a class name (e.g. ‘class_exists()’), that doesn’t flip the variable to being a literal.

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Sep 5, 2022

That's what I thought, so this PR has to be fixed with my suggestion. Thank you.

@staabm staabm marked this pull request as draft Sep 5, 2022
@clxmstaab clxmstaab force-pushed the bug7823 branch 3 times, most recently from 26d06aa to e71a0dc Compare Sep 5, 2022
@staabm
Copy link
Contributor Author

staabm commented Sep 5, 2022

thanks for the input guys. I just committed an update.

@staabm staabm marked this pull request as ready for review Sep 5, 2022
@staabm
Copy link
Contributor Author

staabm commented Sep 5, 2022

The spurious out of memory error could also be observed on other PRs. I don‘t think it is related to the actual change

$reflection = $type->getClassReflection();
if ($reflection !== null && $reflection->isFinalByKeyword()) {
return new ConstantStringType($reflection->getName(), true);
$stringType = new ConstantStringType($reflection->getName(), true);
Copy link
Contributor

@rvanvelzen rvanvelzen Sep 6, 2022

Choose a reason for hiding this comment

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

ConstantStringType is already considered to be a literal string, so intersecting with AccessoryLiteralStringType will just remove the AccessoryLiteralStringType again.

Copy link
Contributor Author

@staabm staabm Sep 6, 2022

Choose a reason for hiding this comment

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

great catch! fixed

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

I'd say that the ifs above should also be covered by this PR. Basically any $obj::class is a literal-string.

So with $obj::class and $obj being EnumCaseObjectType, it should also be literal-string. And with $type instanceof TemplateType && !$type instanceof TypeWithClassName that should work too.

@staabm
Copy link
Contributor Author

staabm commented Sep 7, 2022

adjusted per feedback.

I am thinking whether a object typed parameter on which ::class is appended should also be considered literal?

/**
 * @param object $o
 */
function a($o): void
{
	assertType('class-string&literal-string', $o::class);
	sayHello($o::class);
}

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Sep 7, 2022

Of course :)

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Sep 20, 2022

Please rebase and solve the conflict, I'll merge it then.

@ondrejmirtes ondrejmirtes changed the title accept generic class string as literal string Accept ::class as literal string Sep 20, 2022
@ondrejmirtes ondrejmirtes merged commit d61b9f9 into phpstan:1.8.x Sep 20, 2022
47 checks passed
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Sep 20, 2022

Thank you!

@staabm staabm deleted the bug7823 branch Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants