search

Roave / BetterReflection

:crystal_ball: Better Reflection is a reflection API that aims to improve and provide more features than PHP's built-in reflection API.
MIT License
1.18k stars 131 forks source link

Last call for `5.0.0` BC breaks #904

Closed Ocramius closed 2 years ago

Ocramius commented 2 years ago

This is just a placeholder to make sure everyone is aware that 5.0.0 is around the corner: if nobody replies to this issue within a couple days, I will release 5.0.0 from 5.0.x (after attempting to put together some release notes @_@ )

If you need to break BC further, now is the time.

Ocramius commented 2 years ago

/cc @asgrim @ondrejmirtes @kukulich

kukulich commented 2 years ago

Ok, I suggest to remove the monkey patching feature :)

Ocramius commented 2 years ago

@asgrim the mutator API is your project: wdyt?

ondrejmirtes commented 2 years ago

Hi, I'm really enjoying. the 5.0.x. branch. Previously my fork has been based off BR 4.3. Now I successfully based my new fork on top of 5.0.x and established a downgrading process so that it works on PHP 7.1+. PHPStan's tests and PHPStan-on-PHPStan are now green with the new fork. But there's a bit more work to be done so a few more bugfixes might be submitted.

This is a list of what it does additionally/differently that might be interesting to the original version:

Additional things are not part of my fork, but are commited directly to phpstan-src:

Let me know if anything from this list is interesting to land in BetterReflection. Thanks.

Ocramius commented 2 years ago

Hi, I'm really enjoying. the 5.0.x. branch. Previously my fork has been based off BR 4.3. Now I successfully based my new fork on top of 5.0.x and established a downgrading process so that it works on PHP 7.1+. PHPStan's tests and PHPStan-on-PHPStan are now green with the new fork. But there's a bit more work to be done so a few more bugfixes might be submitted.

This is a list of what it does additionally/differently that might be interesting to the original version:

  • BetterReflection::populate() method so that I can provide my own instances of services to BR (ondrejmirtes@ad54d35) and that the whole reflection is consistent.

The root BetterReflection class is not supposed to be re-used in libraries: it's an "entry-point" for "doing it quickly, based on assumptions", but it is not an extension point.

For example, roave/backward-compatibility-check only uses it for getting an AstLocator: https://github.com/Roave/BackwardCompatibilityCheck/blob/7d37292fc3ab6a94682812f39a18860a13318cfd/bin/roave-backward-compatibility-check.php#L59

Consider BetterReflection (the class) a facade: as soon as you want more, get rid of it :)

  • More methods isPresentClass, isPresentFunction for PhpStormStubsSourceStubber (ondrejmirtes@09fd5c2) - so I can ask which symbols should not be present on the $phpVersion provided to the stubber's constructor. They return ?bool. In case of false PHPStan reports the symbol as undefined.

I'd say that the PhpStormStubsSourceStubber should not return any symbols that aren't declared in the specified version. Instantiating different PhpStormStubsSourceStubber with different versions would give you the possibility of comparing declared symbols.

  • PhpStormStubsSourceStubber reports filename (ondrejmirtes@e416d4d) - so PHPStan can read the PHPDocs resolved with the correct namespace and use statements. It might be weird for internal symbols to have filenames, but it works as long as isBuiltIn() still answers correctly.

Wouldn't it make more sense to read the docblock directly? The API for that is already provided...

  • ReflectionConstant::populateValue() (ondrejmirtes@2c2f032) - PHPStan has its own version of AutoloadSourceLocator, which can reflect on constants from an unknown file just based on defined() returning true. I need to provide the constant's value to BR.

This is a no-go: long-term, all symbols should move towards @psalm-immutable. The AutoloadSourceLocator (and the SourceLocator concept itself) probably needs tweaking to provide the constant value upfront, but populating the value post-definition is backwards, in this case.

This makes sense: some constants are just like that. Allowing resource would probably allow the previous point, but we can't design a native union type for resource, can we?

  • ReflectionClass::getParentClass() - be more resilient against errors (ondrejmirtes@1149f44) - I don't want to have reflection errors when someone extends an unknown class. I want to fail gracefully and have it reported with a PHPStan rule. Same should go for implemented interfaces but it's a much rarer problem.

This generally goes against the design of this library: undefined edges of the graph should lead to crashes. It is possible to build resiliency in userland (perhaps with an out-of-the-box implementation) by having a fallback "catch-all" source locator somewhere, registered at the end of all source locators, which will provide you with a symbol regardless, and mark it as unknown by adding some sort of signature or docblock definition to it.

  • Support old-style constructors - When class Foo implements method Foo, it still works as a constructor on PHP 8, it's just deprecated (ondrejmirtes@6f597e6)

Argh, I thought this was gone for good! This is not a BC break - please feel free to send a patch, and it will be included in 5.1.0

Also here, not a BC break: we can perhaps include this in 5.1.0, given patches and tests.

Additional things are not part of my fork, but are commited directly to phpstan-src:

Not sure this needs to be in core: it seems like an implementation of the abstraction provided by this library.

Totally worth using the classmap generator as input, and I even attempted doing that in the past, but remember that including anything is not really viable in core BetterReflection. Such a class would be interesting with an integration test, but would require composer/composer as dev dependency to guarantee compatibility. Perhaps separate package that just does that (with a massive disclaimer that it's including the analyzed sources)?

Ocramius commented 2 years ago

@ondrejmirtes of the above, the only one that we would need to do immediately, and that is a BC break, is this one:

asgrim commented 2 years ago

@asgrim the mutator API is your project: wdyt?

@kukulich I'm fine with removing the monkey patching feature if it isn't useful. It isn't the main point of the library indeed, but more of a "look what you can do with this". :+1:

At some point in the future I might make a new library that uses BR to do the monkey patching, just for fun :grin:

ondrejmirtes commented 2 years ago

The root BetterReflection class is not supposed to be re-used in libraries: it's an "entry-point" for "doing it quickly, based on assumptions", but it is not an extension point.

I'm not using it like that, the problem is that there are some places in BR that do this:

return (new BetterReflection())->reflector()->reflectClass($className);

So I need to provide the correct Reflector etc. for those places.

 Instantiating different PhpStormStubsSourceStubber with different versions would give you the possibility of comparing declared symbols.

That seems wasteful. I just need to know whether a Symbol is an internal symbol at all. If not, I can look for it in other source locators. If yes, there are two situations: it IS defined in the current analysis PHP version, or it IS NOT defined in the current analysis PHP version. Let's say PHPStan is running on PHP 7.4 and the user calls a function that was added in PHP 7.4, but they want to analyse their codebase as if they were running PHP 7.1. If PhpStormStubsSourceStubber tells me a simple yes/no (no in this case), I'd come to conclusion that the function exists but is defined in runtime in a different source locator. But the correct conclusion is that the function is NOT defined.

Wouldn't it make more sense to read the docblock directly? The API for that is already provided...

The PHPDoc string doesn't tell the whole story. It doesn't tell me the current namespace and use statements, so I can't resolve relative names in it correctly.

Constants from interface are considered final implicitly before PHP 8.1

This is a difficult problem to solve because ReflectionClassConstant doesn't have the PHP version it should consider for analysis. The only place that currently decides differently based on PHP version is PhpStormStubsSourceStubber. In my fork I solved it by adding public static $phpVersion = PHP_VERSION_ID; to BetterReflection class.

but remember that including anything is not really viable in core BetterReflection

I don't get it - the code doesn't include() anything, I just copy-pasted the relevant code from Composer (while giving credit in the docblock).

ondrejmirtes commented 2 years ago

Alright, I'm taking back the point about (new BetterReflection())->reflector(), looks like it's not in any path that would get executed by PHPStan. This is a different situation than it was on BR 4.x 👍

Ocramius commented 2 years ago

Instantiating different PhpStormStubsSourceStubber with different versions would give you the possibility of comparing declared symbols.

That seems wasteful. I just need to know whether a Symbol is an internal symbol at all. If not, I can look for it in other source locators. If yes, there are two situations: it IS defined in the current analysis PHP version, or it IS NOT defined in the current analysis PHP version. Let's say PHPStan is running on PHP 7.4 and the user calls a function that was added in PHP 7.4, but they want to analyse their codebase as if they were running PHP 7.1. If PhpStormStubsSourceStubber tells me a simple yes/no (no in this case), I'd come to conclusion that the function exists but is defined in runtime in a different source locator. But the correct conclusion is that the function is NOT defined.

Shouldn't a source locator for 7.1 be instantiated in that case? :thinking: Perhaps I'm not fully understanding the problem: can we perhaps talk about it in a call, next week?

I'd come to conclusion that the function exists but is defined in runtime in a different source locator.

FWIW, having completely different reflectors/locators is the primary/first use-case of this library: roave/backward-compatibility-check instantiates separate source locators for separate codebase versions ( https://github.com/Roave/BackwardCompatibilityCheck/blob/7d37292fc3ab6a94682812f39a18860a13318cfd/bin/roave-backward-compatibility-check.php#L64-L81 )

The PHPDoc string doesn't tell the whole story. It doesn't tell me the current namespace and use statements, so I can't resolve relative names in it correctly.

Constants from interface are considered final implicitly before PHP 8.1

The sources of that file should do that though, no? :thinking: EDIT: possibly other tools wanting a file, rather than the source string?

Constants from interface are considered final implicitly before PHP 8.1

This is a difficult problem to solve because ReflectionClassConstant doesn't have the PHP version it should consider for analysis. The only place that currently decides differently based on PHP version is PhpStormStubsSourceStubber. In my fork I solved it by adding public static $phpVersion = PHP_VERSION_ID; to BetterReflection class.

Ah, now I see the complexity: the behavior of the whole reflector changes there! I really wonder which injection point would be necessary, for constants to behave differently based on PHP version... :thinking:

but remember that including anything is not really viable in core BetterReflection

I don't get it - the code doesn't include() anything, I just copy-pasted the relevant code from Composer (while giving credit in the docblock).

Ah, I see from the linked code that you are actually processing the contents via regex: very fragile, but understandable. This sort of "weak contract" is precisely why I'd need composer/composer as a dependency, because the structure of those autoloader files is potentially subject to changes, hence the BC boundary being so thin. This really should live in its own package that uses roave/better-reflection and composer/composer in its dependencies, similar to how we pinned down roave/infection-static-analysis-plugin ( https://github.com/Roave/infection-static-analysis-plugin/tree/767ffca9f37bf18fce1031b723650e594bdbc7f6#stability )

kukulich commented 2 years ago

https://github.com/Roave/BetterReflection/pull/905 ✅

  • Support old-style constructors - When class Foo implements method Foo, it still works as a constructor on PHP 8, it's just deprecated (ondrejmirtes@6f597e6)

https://github.com/Roave/BetterReflection/pull/907 ✅

kukulich commented 2 years ago

Wouldn't it make more sense to read the docblock directly? The API for that is already provided...

The PHPDoc string doesn't tell the whole story. It doesn't tell me the current namespace and use statements, so I can't resolve relative names in it correctly.

I think there should be better solution but there's probably bug/unsolved use-cases in PhpStormSourceStubber - I have to think about it.

EDIT: https://github.com/Roave/BetterReflection/pull/910 ✅

ondrejmirtes commented 2 years ago

Perhaps I'm not fully understanding the problem: can we perhaps talk about it in a call, next week?

Yeah, sure :) To rephrase the problem one more time, I need to wrap PhpInternalSourceLocator (with ReflectionSourceStubber) in my own PhpVersionBlacklistSourceLocator (https://github.com/phpstan/phpstan-src/blob/master/src/Reflection/BetterReflection/SourceLocator/PhpVersionBlacklistSourceLocator.php) that calls these extra PhpStormStubsSourceStubber methods so that internal PHP functions from runtime version are not considered defined if they don't exist in the older PHP version set by the user.

The usefulness is demonstrated in the PHPStan playground: https://phpstan.org/r/656bc0ac-949b-45ef-919c-9365d4ebffac - it always runs on PHP 8.0 but sets different phpVersion for its PHPStan config when running analysis. If I didn't use PhpVersionBlacklistSourceLocator, the linked code snippet wouldn't report an error on PHP 7.x.

because the structure of those autoloader files is potentially subject to changes

It's not parsing Composer autoloader files, it's parsing the PHP files in the analysed codebase to look for defined classes, interfaces, traits, enums, functions...

Ocramius commented 2 years ago

Perhaps I'm not fully understanding the problem: can we perhaps talk about it in a call, next week?

Yeah, sure :) To rephrase the problem one more time, I need to wrap PhpInternalSourceLocator (with ReflectionSourceStubber) in my own PhpVersionBlacklistSourceLocator (https://github.com/phpstan/phpstan-src/blob/master/src/Reflection/BetterReflection/SourceLocator/PhpVersionBlacklistSourceLocator.php) that calls these extra PhpStormStubsSourceStubber methods so that internal PHP functions from runtime version are not considered defined if they don't exist in the older PHP version set by the user.

The usefulness is demonstrated in the PHPStan playground: https://phpstan.org/r/656bc0ac-949b-45ef-919c-9365d4ebffac - it always runs on PHP 8.0 but sets different phpVersion for its PHPStan config when running analysis. If I didn't use PhpVersionBlacklistSourceLocator, the linked code snippet wouldn't report an error on PHP 7.x.

Just to make sure: isn't this already covered by the internal source locator having the PHP version specified as a parameter? I would expect the example from the playground to be 3 different PHPStan processes, each started with a different input PHP version :thinking:

because the structure of those autoloader files is potentially subject to changes

It's not parsing Composer autoloader files, it's parsing the PHP files in the analysed codebase to look for defined classes, interfaces, traits, enums, functions...

Ah, that makes much more sense, yes. It's basically replicating https://github.com/composer/composer/blob/4f789a5f6d3bff8f3650202e875a069249b911dc/src/Composer/Autoload/ClassMapGenerator.php#L216-L285

We can probably include that in the repository, with some name like WonkyAssOptimisticFileStructureSourceLocator or such (I know composer does stop parsing symbols, in some cases, but I don't remember the scenarios). Testing that will not be a pleasure :P

Ocramius commented 2 years ago
  • PhpStormStubsSourceStubber reports filename (ondrejmirtes@e416d4d) - so PHPStan can read the PHPDocs resolved with the correct namespace and use statements. It might be weird for internal symbols to have filenames, but it works as long as isBuiltIn() still answers correctly.

Thanks to @kukulich's work in #910, this should now be possible without file location.

ondrejmirtes commented 2 years ago

isn't this already covered by the internal source locator having the PHP version specified as a parameter

I'm not sure, but we can set up a call to talk about this :)

I would expect the example from the playground to be 3 different PHPStan processes, each started with a different input PHP version

It starts different PHPStan processes, but all run on PHP 8.0, just with a different phpVersion in phpstan.neon :)

ondrejmirtes commented 2 years ago

BTW I just found an inconsistency between native PHP reflection and BR's Adapter regarding enum cases (BR doesn't do this correctly https://3v4l.org/4gqGD) which results in this (https://phpstan.org/r/5a32012d-ac0d-46c1-b5dd-aa83a3f42067) - an object should be inferred instead of integer 1. So please hold on with the release a bit longer, thanks :)

kukulich commented 2 years ago

BTW I just found an inconsistency between native PHP reflection and BR's Adapter regarding enum cases (BR doesn't do this correctly https://3v4l.org/4gqGD) which results in this (https://phpstan.org/r/5a32012d-ac0d-46c1-b5dd-aa83a3f42067) - an object should be inferred instead of integer 1. So please hold on with the release a bit longer, thanks :)

https://github.com/Roave/BetterReflection/pull/918

Ocramius commented 2 years ago

@ondrejmirtes I'd have some free time on Friday (I know, it's the 24th), but IMO we should at this point:

  1. release
  2. eventually release 6.0.0 if BC breaks are necessary afterwards

Would be a good xmas present :D

ondrejmirtes commented 2 years ago

I agree, release the kraken! 😊

I have 5.0.x integrated in PHPStan and it's super solid. I'll be able to release PHPStan with Enums support shortly after thanks to BR, I just need a few finishing touches that I hopefully find the time for during the holidays 😊

WyriHaximus commented 2 years ago

@Ocramius that be an amazing Christmas present, but don't feel bad to take longer if you have to.

asgrim commented 2 years ago

excited

Ocramius commented 2 years ago

Closing here: will focus on writing release notes and finishing up all pending details, but no more BEE SEE BREKS!