PHPDoc-based type narrowing

[rvanvelzen]

Implementation for phpstan/phpstan#7110. I already had a big chunk of this quite soon, so I finished most of it.

Just putting this out there, hopefully you didn't already start on this :smiling_face_with_tear:

[herndlm]

Oh, nice! Looks like we could get rid then of almost all of the custom expression building in the phpstan-assert* extensions :)

[ondrejmirtes]

Awesome, I haven't, I'm too deep in InitializerExprTypeResolver now (and also in my Verona phpDay talk) so great, I appreciate this :) I'm gonna go through the code soon, I just gonna list of some of my extra ideas for rules around this (you might have already done some of them, but I need to get them off my chest first :):

• Similarly to how subject/target is checked in conditional types, @phpstan-assert should also be checked for sanity.
  • So for example - @param int + @phpstan-assert int - doesn't narrow down the type
  • @param int + @phpstan-assert int|string - doesn't narrow down the type
  • @param string + @phpstan-assert int - can never happen
  • Also needs to work with generics @template T of int + @param T + @phpstan-assert positive-int is fine
  • But @template T of int + @param T + @phpstan-assert int doesn't narrow down the type
• The same rule should check if the referenced parameter in @phpstan-assert exists
• Check that the feature works with generics - e.g. @phpstan-assert T after we know what T is inferred as.
• Check that the @phpstan-assert can be put in stub files and that it works too.
• Check that ImpossibleCheckTypeFunctionCallRule automatically picks up that function is narrowing down something that's already narrowed. (Nothing special should be needed, everything should already work based on the code in TypeSpecifier and ImpossibleCheckTypeHelper).

Before I dive into the review, can you please check this list? :) Thanks.

[ondrejmirtes]

Just to be sure - if you're gonna work on these, please add them as new commits, it makes the review easier :)

To see how stubs-related functionality is tested, look at https://github.com/phpstan/phpstan-src/blob/1.7.x/tests/PHPStan/Analyser/ClassConstantStubFileTest.php.

[rvanvelzen]

None of that list is tested right now, just the very basic type narrowing. I'll look into that soon, thanks :)

[ondrejmirtes]

I'm glad that I hit the nail on the head :)

Also I'll probably question some of your reflection choices (where to put the new information), but I need to think about it and I'll shuffle things around myself later :)

[ondrejmirtes]

I really like how it's coming together :) After this one is merged, I also look forward to support for properties and methods besides parameter names :)

[rvanvelzen]

There's some work left to be done here, but it should be ready for review.

I'm not sure if the always true/false errors should pop up from these asserts - a function could very well narrow a type down without that being the only thing the function does. This could actually be handled by specifying types from conditional return types. I have a PR almost ready for that.

All that aside: I'll be on holiday for the next 2 weeks so I won't be able to spend much time on this. If you point out any issues I'll gladly work on them but I just don't know how quickly that will be. Thanks for understanding :)

[ondrejmirtes]

I'm not sure if the always true/false errors should pop up from these asserts - a function could very well narrow a type down without that being the only thing the function does.

That's my worry as well but I'd like to see in practice if it's a real concern or not. In theory someone could write @phpstan-assert above functions that do other things as side effects as well, but maybe it doesn't happen in practice and it's still useful to report things like "this int is already an int".

It'd be nice to have these "impossible or always-true type checks" reported only in bleeding edge, but I'm not sure we can easily isolate these cases from the already existing ones in rules like ImpossibleCheckTypeFunctionCallRule etc.

This could actually be handled by specifying types from conditional return types. I have a PR almost ready for that.

I have no idea what you mean by that, so I'm intrigued to see it :)

All that aside: I'll be on holiday for the next 2 weeks so I won't be able to spend much time on this.

I hope you won't mind if we're going to postpone this feature for PHPStan 1.8. There's already a lot of stuff ready in PHPStan 1.7 (and it doesn't mean we can't release 1.8 immediately after the PHPDoc-based type narrowing is merged too) and I'd like to gather real-world feedback on it soon. Thank you for understanding.

[rvanvelzen]

Sure, it's absolutely fine to let this sit until after 1.7 is released :)

[BackEndTea]

In theory someone could write @phpstan-assert above functions that do other things as side effects as well, but maybe it doesn't happen in practice and it's still useful to report things like "this int is already an int".

As far as i know this is the current behavior in psalm, which is why not all methods in webmozarts/assert have an @psalm-assert annotation. So it would probably be best to mimic this behavior in PHPStan. And maybe it helps promote people to write smaller functions that only do 1 thing, either a side effect or a validation.

[ondrejmirtes]

BTW Do you have plans to support this syntax? Thanks! @psalm-assert =ExpectedType $actual

[BackEndTea]

What would that do? Does that mean $actual is optional/nullable?

[rvanvelzen]

Not necessarily for the initial implementation, but I do plan on adding that as well.

[ondrejmirtes]

What would that do?

I have no idea, doesn't seem documented: https://psalm.dev/docs/annotating_code/adding_assertions/

/cc @orklah What does @psalm-assert =ExpectedType $actual do? And is there any other syntax to be aware of?

[rvanvelzen]

Those are equality assertions, though the documentation for it is hard to find: https://psalm.dev/annotating_code/assertion_syntax/

[rvanvelzen]

I believe this has become a bit too large and complex. I'd like to break it up into a few pieces to make it easier to reason about.

[ondrejmirtes]

Feel free to do what you think is best 😊 My idea for reflection is that I'm not sure if I want it on ParametersAcceptor vs. MethodReflection/FunctionReflection. I think it fits the latter better.

[mvorisek]

this PR seems to implement https://github.com/phpstan/phpstan/issues/7385, please include the demo https://phpstan.org/r/53ba9b04-685e-404c-ba77-a9616ca75ace code in the tests

[ondrejmirtes]

BTW @rvanvelzen This one would be a great thing to finish :) My idea is that instead of ParametersAcceptorWithAsserts / FunctionVariant etc., the getAsserts() could be on ExtendedMethodReflection + FunctionReflection.

[rvanvelzen]

@ondrejmirtes it's on my list to work on, but I need/want to finish phpstan/phpdoc-parser#139 first but I'm not really happy with the current version :/

[orklah]

/cc @orklah What does @psalm-assert =ExpectedType $actual do? And is there any other syntax to be aware of?

Wow, just saw this in my notifications, sorry I seem to have missed it completely before.

The = in assertions is used to express the IsIdentical assertion. You also have the ~ to express the IsLooselyEqual assertion And both of those can be negated (i.e. !~ is IsNotLooselyEqual). It allows to express every !=, !==, == or === we can encounter. I'm still unsure in which case we use our IsType/IsNotType assertion compared to those.

[ondrejmirtes]

A lot of people are really looking forward to this :) I thought I'd do this refactoring, but the branch isn't rebased and unfortunately has a lot of conflicts I don't really know how to solve :(

• Remove ParametersAcceptorWithAsserts
• Move public function getAsserts(): Assertions to FunctionReflection and ExtendedMethodReflection

Then I think I'd quite be happy with the API :)

[rvanvelzen]

I will try to get to this next week

[rvanvelzen]

This is rebased and functionally ready.

I've tried to refactor to your suggestion, but I can't figure out how to do it properly - you don't have easy access to resolved template types with just the reflection :/

[ondrejmirtes]

Alright, I'm gonna try it and see if I bump into a problem :) I'm really looking forward to this.

PHPDoc-based type narrowing and the list type will be the headlining features of PHPStan 1.9.0 :)

[ondrejmirtes]

Alright, i rewrote the reflection to my taste, with some help from commits already in 1.9.x:

• https://github.com/phpstan/phpstan-src/commit/3d42a8be55cc4031636757aceed45e4926e16c97
• https://github.com/phpstan/phpstan-src/commit/f94f5763f11ca6cd180bc5e5637cb25ab4929c27

And here I did these two commits:

• 0a4404f (#1317)
• 0de3f9b (#1317)

I think the 2nd one is the one you perhaps weren't willing to do, it's not that clean. Also I'm sure that if we test the same situation with instance method and static method, it will require the same fix too.

But if the build is green, I'm willing to merge this and continue the work on 1.9.x.

I'll create issues with some checklists for the list and @phpstan-assert features so that we can track the work in progress before PHPStan 1.9.0 is ready to be released.

[ondrejmirtes]

I'm quite happy with the CI results, I'll make Rector freak out less about it (it's one of the problems I already suspected and mentioned) and then we can improve it on top of 1.9.x branch :)

[ondrejmirtes]

I want to merge this because we're already stepping on each other's toes (I have some local commits but you force-pushed meanwhile :))

[ondrejmirtes]

We can do so much more stuff once this is merged!

• Get rid of many type-specifying extensions in https://github.com/phpstan/phpstan-src/tree/1.9.x/src/Type/Php
• Get rid of https://github.com/phpstan/phpstan-src/blob/1.9.x/src/Internal/ScopeIsInClassTypeSpecifyingExtension.php
• Simplify phpstan-phpunit, phpstan-beberlei-assert, phpstan-webmozart-assert

I can't wait :)

[ondrejmirtes]

Thank you very much!