Skip to content

Fix check generic mixed type based on config v2#2885

Merged
ondrejmirtes merged 2 commits intophpstan:1.10.xfrom
schlndh:fix-checkGenericMixedTypeBasedOnConfigV2
Jan 22, 2024
Merged

Fix check generic mixed type based on config v2#2885
ondrejmirtes merged 2 commits intophpstan:1.10.xfrom
schlndh:fix-checkGenericMixedTypeBasedOnConfigV2

Conversation

@schlndh
Copy link
Contributor

@schlndh schlndh commented Jan 21, 2024 •
edited
Loading

This is an alternative to #2809 . This time I was able to limit the changes to RuleLevelHelper (it kind of makes me wonder how I missed this option in the first place).

The goal is to handle TemplateMixedType similarly to explicit MixedType with respect to checkExplicitMixed. Here is an example of the issues in as many cases as I can think of https://phpstan.org/r/0b58afa8-fd01-4991-922e-d46037106135 . As you can see, even the regular mixed type is not covered completely, but that's a matter for another PR(s).

These are the results for the linked playground code in this branch:

  9      Cannot call method foo() on T of mixed.                                          
  10     Cannot call static method bar() on T of mixed.                                   
  11     Cannot access offset 5 on T of mixed.                                            
  12     Parameter #1 $string of function strlen expects string, T given.                 
  16     Cannot cast T to int.                                                            
  17     Anonymous function should return int but returns T.                              
  18     Only iterables can be unpacked, T of mixed given in argument #1.                 
  19     Only iterables can be unpacked, T of mixed given.                                
  21     Argument of an invalid type T of mixed supplied for foreach, only iterables are  
         supported.                                                                       
  36     Cannot call method foo() on mixed.                                               
  37     Cannot call static method bar() on mixed.                                        
  38     Cannot access offset 5 on mixed.                                                 
  39     Parameter #1 $string of function strlen expects string, mixed given.             
  43     Cannot cast mixed to int.                                                        
  44     Anonymous function should return int but returns mixed.                          
  45     Only iterables can be unpacked, mixed given in argument #1.                      
  46     Only iterables can be unpacked, mixed given.                                     
  48     Argument of an invalid type mixed supplied for foreach, only iterables are       
         supported.

@schlndh schlndh mentioned this pull request Jan 21, 2024
Closed
schlndh
schlndh commented Jan 21, 2024
View reviewed changes
src/Rules/RuleLevelHelper.php Outdated
Copy link
Contributor Author

@schlndh schlndh Jan 21, 2024

Choose a reason for hiding this comment

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

TemplateMixedType extends explicit MixedType, so I made its behavior consistent with that.

I wonder whether @template T and @template T of mixed should be implicit/explicit respectively. But I assume that the implicit mixed exists mainly for untyped legacy code. So in that sense it seems right to me for both of them to be considered explicit, even though it feels slightly inconsistent with how it works for non-generic parameters. Especially, considering that checkImplicitMixed is off even with level 9.

schlndh
schlndh commented Jan 21, 2024
View reviewed changes
tests/PHPStan/Rules/Methods/data/call-static-method-mixed.php Outdated
Copy link
Contributor Author

@schlndh schlndh Jan 21, 2024

Choose a reason for hiding this comment

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

Unfortunately, I wasn't able to figure out how to make a callable with implicit mixed parameter. But if it's impossible to make one, then it doesn't matter how it behaves and so there's no need to test it.

@schlndh schlndh mentioned this pull request Jan 21, 2024
Merged
@ondrejmirtes ondrejmirtes force-pushed the fix-checkGenericMixedTypeBasedOnConfigV2 branch from cece3bf to 4321f19 Compare January 21, 2024 22:13
ondrejmirtes
ondrejmirtes requested changes Jan 21, 2024
View reviewed changes
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.

Hi, I mostly like this, but I'd prefer the new behaviour only for bleeding edge, because this leads to more errors being eeported.

Please lead to the new behaviour only when newRuleLevelHelper is enabled, otherwise use the old behaviour. You don't need to test it, it should be obvious from the diff (that you're adding new branches to the code and otherwise not changing anything else).

@schlndh schlndh requested a review from ondrejmirtes January 22, 2024 15:12
@ondrejmirtes ondrejmirtes force-pushed the fix-checkGenericMixedTypeBasedOnConfigV2 branch 2 times, most recently from b58c879 to c4f1d74 Compare January 22, 2024 19:17
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Jan 22, 2024

I've reworked the code to show you what I meant - the diff is now obvious, and removing the feature flags when working on 2.0 will be trivial.

Screenshot 2024-01-22 at 20 18 20
Screenshot 2024-01-22 at 20 18 10

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Jan 22, 2024

If the build passes I'm gonna merge this :)

@ondrejmirtes ondrejmirtes force-pushed the fix-checkGenericMixedTypeBasedOnConfigV2 branch from c4f1d74 to f4b1b48 Compare January 22, 2024 19:29
@ondrejmirtes ondrejmirtes merged commit a3d0910 into phpstan:1.10.x Jan 22, 2024
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Jan 22, 2024

Thank you!

@schlndh schlndh deleted the fix-checkGenericMixedTypeBasedOnConfigV2 branch January 22, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@schlndh @ondrejmirtes