Skip to content

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

Open
jrmajor wants to merge 1 commit into
phpstan:2.3.xfrom
jrmajor:fix-class-string-supertype
Open

jrmajor wants to merge 1 commit into
phpstan:2.3.xfrom
jrmajor:fix-class-string-supertype

Conversation

@jrmajor

@jrmajor jrmajor commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

@phpstan-bot

Copy link
Copy Markdown
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
@jrmajor
jrmajor force-pushed the fix-class-string-supertype branch from c0c711b to 428ba86 Compare October 31, 2023 15:22
@ondrejmirtes

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

@jrmajor

jrmajor commented Oct 31, 2023

Copy link
Copy Markdown
Contributor Author

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?

@ondrejmirtes ondrejmirtes left a comment

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.

After these changes it will be good to merge 👍

)),
],
ConstantStringType::class,
"'Exception'",

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.

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

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.

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
jrmajor force-pushed the fix-class-string-supertype branch from c038852 to a293912 Compare October 31, 2023 19:30
@jrmajor

jrmajor commented Oct 31, 2023

Copy link
Copy Markdown
Contributor Author

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

jrmajor commented Dec 15, 2023

Copy link
Copy Markdown
Contributor Author

@ondrejmirtes Friendly ping, I need your decision here.

@ondrejmirtes
ondrejmirtes force-pushed the fix-class-string-supertype branch from 218633a to 1a693bb Compare October 31, 2026 13:29
@ondrejmirtes
ondrejmirtes changed the base branch from 1.10.x to 2.3.x October 31, 2026 13:29
@ondrejmirtes
ondrejmirtes force-pushed the fix-class-string-supertype branch from 1a693bb to 7ed5126 Compare October 31, 2026 13:48
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.

Union of string literal and class-string<T> does not accept that string

3 participants