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 check generic mixed type based on config v2 #2885

Merged

Conversation

schlndh
Copy link
Contributor

@schlndh schlndh commented Jan 21, 2024

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. 

@@ -62,7 +62,7 @@ private function transformCommonType(Type $type): Type
}

return TypeTraverser::map($type, function (Type $type, callable $traverse) {
if ($type instanceof TemplateMixedType) {
if ($type instanceof TemplateMixedType && $this->checkExplicitMixed) {
Copy link
Contributor Author

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.

{

/**
* @param callable(mixed): void $cb
Copy link
Contributor Author

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.

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).

@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

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

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
70 checks passed
@ondrejmirtes
Copy link
Member

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
Labels
None yet
Projects
None yet
2 participants