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

Add extended reasons tips to param out errors #3258

Closed
wants to merge 1 commit into from

Conversation

thg2k
Copy link
Contributor

@thg2k thg2k commented Jul 22, 2024

This adds the very useful tips on complex types also to param-out.

Please carefully review the correctness, I assume that isSuperTypeOf and acceptsWithReason are equivalent.

Also not sure how to set the strictness parameter, as it is a param-out it should not be affected by the strict types declare.

Before:

 ------ ----------------------------------------------------------------------------------------------------------------------------
  Line   case2.php
 ------ ----------------------------------------------------------------------------------------------------------------------------
  8      Parameter &$foo by-ref type of function foo() expects array{a: int, b: int, c: string}, array{a: int, b: int, c: 5} given.
           parameterByRef.type
          You can change the parameter out type with @param-out PHPDoc tag.
 ------ ----------------------------------------------------------------------------------------------------------------------------

After:

 ------ ----------------------------------------------------------------------------------------------------------------------------
  Line   case2.php
 ------ ----------------------------------------------------------------------------------------------------------------------------
  8      Parameter &$foo by-ref type of function foo() expects array{a: int, b: int, c: string}, array{a: int, b: int, c: 5} given.
           parameterByRef.type
          Offset 'c' (string) does not accept type int.
          You can change the parameter out type with @param-out PHPDoc tag.
 ------ ----------------------------------------------------------------------------------------------------------------------------

@ondrejmirtes
Copy link
Member

accepts and isSuperTypeOf are not equivalent. The former is about "Does this type accept that type?" and the latter one is "Is this type part of that type?"

Some practical differences:

  • Float accepts any int, but float isn't an int
  • Without strict_types=1, object of class with __toString (Stringable) is accepted in a string.
  • Generics acceptance rules are stricter based on TemplateTypeStrategy

What we could do is implement isSuperTypeOfWithReason. We could use that in all the rules where isSuperTypeOf is used.

Another example besides @param-out where this would be useful is this issue: phpstan/phpstan#10421

@thg2k thg2k force-pushed the pr/param_out_tips branch from 6826152 to 3cdae2c Compare July 23, 2024 09:08
@thg2k
Copy link
Contributor Author

thg2k commented Jul 23, 2024

To be very honest I did not fully understand your answer 😄

Is it correct to assume that isSuperTypeOf()->yes() fully implies acceptance? Because in this case I can do it very dirty and attempt to pull the reasons from acceptsWithReason only if isSuperTypeOf check fails.

This way the semantic should be preserved and we still give the user some insights when available, worst case the message stays the same. Please check the updated PR.

@ondrejmirtes
Copy link
Member

One piece I forgot to mention: we're using isSuperTypeOf because no native type casting is involved, only PHPDocs. So the type must match otherwise you might end up with inconvenient surprise down the road.

I don't want a hack like that, that's not sustainable. It's worth investing in proper solutions.

@thg2k
Copy link
Contributor Author

thg2k commented Jul 23, 2024

It is a hack indeed, but the isSuperTypeWithReason implementation is too big of an endeavor for me, so feel free to close the PR.

@ondrejmirtes
Copy link
Member

I worry that the hack would show non-sensical tips we wouldn't have under control. A proper solution would also have other benefits.

@thg2k
Copy link
Contributor Author

thg2k commented Aug 15, 2024

Note to self:
See also phpstan/phpstan#11460

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.

2 participants