Skip to content

Conversation

ondrejmirtes
Copy link
Member

No description provided.

@ondrejmirtes
Copy link
Member Author

ondrejmirtes commented May 3, 2022

Alright, so this one is for you @rvanvelzen to finish if you're interested :) The original reasoning is here but since then I came up with different names so I'm gonna reiterate all of that below again.

There are several problems we're solving here. The main one is that right now in the stable version, some types are resolved through this path: Expr -> runtime value -> Type. That's because the reflection API works like that - for example ReflectionParameter::getDefaultValue() and many other methods. And that's problematic because obtaining runtime values is an unnecessary and sometimes even an impossible step. Take the "new in initializers" feature (https://wiki.php.net/rfc/new_in_initializers). This path requires to create the object, which means it has to be autoloadable. Which it sometimes isn't. Or that "Foo::BAR" as of PHP 8.1 isn't always a scalar (or array) constant, but an object of an enum case.

So I decided we should take a direct "Expr -> Type" path. First step I took is that for PHPStan 1.7.0, there's going to be fully static reflection (#1265). That allows us to typehint PHPStan\BetterReflection\Reflection\Adapter\Reflection* classes everywhere and rely on extra methods they offer.

Then I added the extra methods that allow us to access the various Expr objects: ondrejmirtes/BetterReflection@9f92669

The "Expr -> runtime value" part in BetterReflection is done with the "CompileNodeToValue" class: https://github.com/ondrejmirtes/BetterReflection/blob/5.3.x/src/NodeCompiler/CompileNodeToValue.php

In PHPStan the equivalent code will live in "InitializerExprTypeResolver" class that I'm adding here in this PR.

"CompileNodeToValue" works with the help of "CompilerContext" class: https://github.com/ondrejmirtes/BetterReflection/blob/5.3.x/src/NodeCompiler/CompilerContext.php

In PHPStan the equivalent code will live in "InitializerExprContext" class that I'm adding here in this PR. Its job is to tell InitializerExprTypeResolver how to resolve various things like __FILE__, __FUNCTION__ etc. It's not finished - there will definitely be more constructor parameters and maybe more/different static create* methods.

The "runtime value -> Type" part in PHPStan was done by ConstantTypeHelper which I'm deprecating in this PR.

So all places should now do direct "Expr -> Type" resolution. What I want you to do here @rvanvelzen is to finish implementing InitializerExprTypeResolver and InitializerExprContext. There's a comment with the remaining expr types to resolve in InitializerExprTypeResolver.

There's going to be some overlap with MutatingScope::getType() and in some cases MutatingScope::getType() can call InitializerExprTypeResolver::getType() (but not vice versa). The difference is that 5 + 5 can be resolved by InitializerExprTypeResolver::getType() but $this->doFoo() + $this->doBar() can't. So simple things like __DIR__ ConstFetch can be delegated to InitializerExprTypeResolver but anything more complicated like BinaryOp\Concat can't.

Why aren't we always using MutatingScope? Because it brings some extra baggage and it brings unwanted dependencies in the wrong direction in the architecture. I don't want Reflection to rely on Scope.

Once this PR is done, we can use it for various nice things like:

  • Your PR: Expose default values for native functions/methods #1261 - use InitializerExprTypeResolver instead of MutatingScope to resolve the types of default expressions.
  • We also need to resolve the defaults for parameters not just from php-8-stubs but also when running on PHP 7.x. That's what @staabm attempted in Read parameter default values from php8-stubs #763 (but his version of InitializerExprTypeResolver was done in ParserNodeTypeToPHPStanType and I didn't particularly like it). This should be done in NativeFunctionReflectionProvider by looking at $reflectionFunction.
  • Once all of that is done, we should focus on WIP: Support named arguments #750 which is about normalizing named arguments when calling various extensions. It's done so that extensions don't have to care about named arguments at all and basically that $funcCall->getArgs() and similar are always indexed as if the function is called without named arguments at all.

I think that these items are also good pointers on how to test that InitializerExprTypeResolver is correctly implemented. I'm worried about correct values for things like __METHOD__ and __NAMESPACE__ the most. Looking at the tests for CompileNodeToValue (https://github.com/ondrejmirtes/BetterReflection/blob/5.3.x/test/unit/NodeCompiler/CompileNodeToValueTest.php) might be a good inspiration. But I'd like to have more type-inference-focused tests than that. So for example for parameter default value, the implementation should be tested through how generics behave when the default value is used - let's say we have foo($param = __FILE__) and the function is called like foo() - we should check what default value is used in @return T when calling the function by doing assertType() in NodeScopeResolverTest.

Or in case of class constants it's really easy. You can have public const FOO = 1 and then do assertType() on self::FOO.

In case of BinaryOps here are all the expr types that are supported in initializers and that should be resolved in InitializerExprTypeResolver correctly: https://github.com/nikic/PHP-Parser/blob/a6e34665fd6a2bcefc924cb5fd73788767ae510e/lib/PhpParser/ConstExprEvaluator.php#L173-L217 (ConstExprEvaluator is used internally in CompileNodeToValue for some simple expressions so we need to replicate that too).

Please let me know @rvanvelzen if you're going to tackle this or if I should do it myself :) Thank you very much.

@rvanvelzen
Copy link
Contributor

Very, very nice. While I'm definitely interested in working on it I'm not sure if I have enough time available.

I'll tinker a bit in the coming days and will get back to you :)

@ondrejmirtes
Copy link
Member Author

Alright, I'm looking forward to that! :) And hopefully meanwhile I'm gonna look at your other PRs :)

@rvanvelzen
Copy link
Contributor

@ondrejmirtes I managed to get the tests passing, but unfortunately I won't be able to invest much more time into it this week: https://github.com/rvanvelzen/phpstan-src/tree/initializer-expr

At some points I wondered whether the approach I was taking made sense. If I can help with anything else please let me know :)

@ondrejmirtes
Copy link
Member Author

Awesome, I can take over from here, I'm gonna push your commit in this PR and continue :) Thank you very much!

@ondrejmirtes ondrejmirtes marked this pull request as ready for review May 10, 2022 16:00
@ondrejmirtes ondrejmirtes merged commit c8b3926 into 1.7.x May 10, 2022
@ondrejmirtes ondrejmirtes deleted the initializer-expr branch May 10, 2022 16:01
@ondrejmirtes
Copy link
Member Author

It's not finished by any means, but it's green enough for now :)

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