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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

check generic mixed type based on config #2809

Closed

Conversation

schlndh
Copy link
Contributor

@schlndh schlndh commented Dec 8, 2023

I discovered this issue: https://phpstan.org/r/4ef0bc61-b8b3-4db0-8b03-d862c15cd37d (for @template T and @template T of mixed it reports wrong class name for the missing method). That lead me to find that @template T and @template T of mixed can hide issues via TemplateMixedType even when checkImplicitMixed and checkCheckMixed are enabled.

For now, I just created a quick and dirty attempt to fix it via RuleLevelHelper and I'd like your feedback on whether this is something that was just overlooked/postponed/..., or whether I again ran into something more complex than I realize. 馃槄 I tried to scroll through the open issues related to generics/mixed, but nothing stood out to me (I'm hoping that the issue bot action may provide further clues).

@schlndh
Copy link
Contributor Author

schlndh commented Dec 8, 2023

I checked why the the sources fail to transform for PHP <8 and it seems to be because InternalScopeFactory is transformed first (the union type for $function is removed) and then LazyInternalScopeFactory fails to transform because at that point the class is not a valid implementation of the interface.

@schlndh
Copy link
Contributor Author

schlndh commented Dec 8, 2023

It seems that one issue is that strict mixed type is not a straightforward upgrade from normal mixed type:

  • It's not subtractable.
  • describe methods only handle the normal mixed type (IDK if this is a feature or a bug).
  • ...

@ondrejmirtes
Copy link
Member

I'm sorry, you're going to have to explain it to me better :) This https://phpstan.org/r/4ef0bc61-b8b3-4db0-8b03-d862c15cd37d just shows a cosmetic issue in an error message, but your PR description tells me about "hiding issues" - meaning there are errors that should be reported but aren't. Can you provide a better code sample where this is obvious? Thanks :)

@schlndh
Copy link
Contributor Author

schlndh commented Dec 8, 2023

Ok, sorry. How about this: https://phpstan.org/r/5e3fed61-e2c2-4c48-ac7f-e2c6d4e972dc ? IMO every line inside those two functions should have an error on level 9 (i.e. "the only allowed operation you can do with it is to pass it to another mixed").

This is the result from my branch (I used a bare-bones config file with just level 9 set and nothing else):

  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 of mixed given.  
  13     Cannot use ++ on T of mixed.                                               
  18     Cannot call method foo() on mixed.                                         
  19     Cannot call static method bar() on mixed.                                  
  20     Cannot access offset 5 on mixed.                                           
  21     Parameter #1 $string of function strlen expects string, mixed given.       
  22     Cannot use ++ on mixed. 

@ondrejmirtes
Copy link
Member

And is the reason it doesn't get reported because TemplateMixedType does not get converted into StrictMixedType?

@schlndh
Copy link
Contributor Author

schlndh commented Dec 8, 2023

I think so (to TemplateStrictMixedType to be exact). I checked $a::foo() (it's caught for regular mixed) and $a++ (it's not caught for either) in the debugger:

For $a::foo() there is this code in StaticMethodCallCheck:

$classTypeResult = $this->ruleLevelHelper->findTypeToCheck(
	$scope,
	NullsafeOperatorHelper::getNullsafeShortcircuitedExprRespectingScope($scope, $class),
	sprintf('Call to static method %s() on an unknown class %%s.', SprintfHelper::escapeFormatString($methodName)),
	static fn (Type $type): bool => $type->canCallMethods()->yes() && $type->hasMethod($methodName)->yes(),
);
$classType = $classTypeResult->getType();
if ($classType instanceof ErrorType) {
	return [$classTypeResult->getUnknownClassErrors(), null];
}
  • The generic version returns ErrorType from findTypeToCheck (getUnknownClassErrors is empty array).
  • The specific version works because findTypeToCheck returns StrictMixedType.

For $a++ there is this code in InvalidIncDecOperationRule:

$varType = $scope->getType($node->var);
if (!$varType->toString() instanceof ErrorType) {
	return [];
}
if (!$varType->toNumber() instanceof ErrorType) {
	return [];
}

Both TemplateMixedType and MixedType always allow toString, so no error is reported.

In general it seems that various rules have ways of dealing with mixed (i.e. using RuleLevelHelper in some ways), but it's not consistent. That's why I thought it might be a good idea to try converting the mixed types to strict versions directly in the scope (it will probably need to happen in more places - e.g. values returned by functions, not just the parameters), so that the rules don't have to think about it individually.

@ondrejmirtes
Copy link
Member

It's all supposed to happen in RuleLevelHelper... I'll have to look into this when I get back in front of my computer 馃槉

@schlndh schlndh force-pushed the fix-checkGenericMixedTypeBasedOnConfig branch from 0c08e27 to 3a03551 Compare January 13, 2024 13:43
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.

And I don't like the changes in PHPStanTestCase either :) I think this needs a completely different direction.

@@ -2772,7 +2774,7 @@ private function enterFunctionLike(
}
}
$parameterNode = new Variable($parameter->getName());
$expressionTypes[$paramExprString] = ExpressionTypeHolder::createYes($parameterNode, $parameterType);
$expressionTypes[$paramExprString] = ExpressionTypeHolder::createYes($parameterNode, $this->ruleLevelHelper->transformCommonType($parameterType));
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this because Scope shouldn't depend on rules. Type inference is rules and rule level-agnostic. Rules already depend on Scope. There shouldn't be a dependency in the other direction.

@schlndh
Copy link
Contributor Author

schlndh commented Jan 21, 2024

@ondrejmirtes Thank you for the feedback (and sorry for delayed reply). You are right. I created #2885 as a replacement.

@schlndh schlndh closed this Jan 21, 2024
@schlndh schlndh deleted the fix-checkGenericMixedTypeBasedOnConfig branch January 21, 2024 16:02
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