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

Fix class-string<*> being supertype of any string literal #2707

Open
wants to merge 8 commits into
base: 1.10.x
Choose a base branch
from

Conversation

jrmajor
Copy link
Contributor

@jrmajor jrmajor commented Oct 31, 2023

@phpstan-bot
Copy link
Collaborator

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

@jrmajor jrmajor changed the base branch from 1.11.x to 1.10.x October 31, 2023 15:13
@ondrejmirtes
Copy link
Member

Hi, changes to isSuperTypeOf need to be verified in TypeCombinatorTest::testUnion and testIntersect. So please add some cases to the data providers there.

You need to test expectations with different types that should hit different branches in that method.

Thank you.

@jrmajor jrmajor marked this pull request as ready for review October 31, 2023 15:40
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@jrmajor
Copy link
Contributor Author

jrmajor commented Oct 31, 2023

Hi, thank you for such a quick reply :)

You need to test expectations with different types that should hit different branches in that method.

Are the tests that I've added sufficient, or do I need to add some for the branches that I didn't touch too?

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

After these changes it will be good to merge 👍

tests/PHPStan/Analyser/AnalyserIntegrationTest.php Outdated Show resolved Hide resolved
tests/PHPStan/Analyser/data/bug-10076.php Outdated Show resolved Hide resolved
)),
],
ConstantStringType::class,
"'Exception'",
Copy link
Member

Choose a reason for hiding this comment

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

Please test intersection with 'array' too - should be NeverType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing the test for phpstan/phpstan#6697 would make it 'array'&class-string<T (function foo(), parameter)> instead of never, because we would need to assume that 'array' may be name of a class PHPStan doesn't know about. What should I do about that?

@jrmajor
Copy link
Contributor Author

jrmajor commented Oct 31, 2023

This broke the test for phpstan/phpstan#6697. Not sure what to do here. Fixing phpstan/phpstan#10076 requires assuming that constant string is not a class-string unless PHPStan knows about that class, while phpstan/phpstan#6697 relies on the assumption that constant string may be a name of a class that PHPStan doesn't know about.

@jrmajor
Copy link
Contributor Author

jrmajor commented Dec 15, 2023

@ondrejmirtes Friendly ping, I need your decision here.

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