phpstan-src icon indicating copy to clipboard operation
phpstan-src copied to clipboard

Implement new InheritingClassMethodRule rule

Open canvural opened this issue 2 years ago • 3 comments

Implements https://github.com/phpstan/phpstan/issues/7388

Some parts needs a little bit of clean up, but I think the logic is correct. After review, I'll clean up the messy parts.

canvural avatar Jun 05 '22 11:06 canvural

An idea here - there are two rules for a similar purpose:

  1. OverridingMethodRule - this one checks native incompatibilities. All reported errors are nonIgnorable. This rule is registered in config and runs since level 0.
  2. MethodSignatureRule - this rule isn't registered on any level. It's only executed if OverridingMethodRule doesn't find any errors and checkPhpDocMethodSignatures is true. Its checks include PHPDoc incompatibilities as well.

I see this PR as an opportunity to replicate this structure for these scenarios, or maybe clean up the whole situation somehow too :)

ondrejmirtes avatar Jun 06 '22 08:06 ondrejmirtes

Sorry, I did not get what you were trying to say 😅 What do you mean by replicate this structure for these scenarios? I think the important logic on OverridingMethodRule is already extracted to MethodParameterComparisonHelper and is used by this new rule. Only MethodSignatureRule is not used by the new rule, cause I thought PHPDocs were not important to check in this case. But I can add if that's what you meant.

canvural avatar Jun 06 '22 20:06 canvural

It's difficult to isolate cases that are just coming from PHPDocs, and those that come from native types and cause PHP warnings/fatal errors (and should be non-ignorable). That's why I chose the two-rule approach to this - OverridingMethodRule only reports the native violations (as non-ignorable). MethodSignatureRule reports all violations (as ignorable), but is called only when OverridingMethodRule returns zero errors.

Can the same be said about this PR? That the errors from PHPDocs are ignorable, and the native errors are non-ignorable? Or is only one of these categories reported?

ondrejmirtes avatar Jun 07 '22 08:06 ondrejmirtes