fix CI

[orklah]

This fixes a new error in CI that seem due to php/php-src#7196 where the isStatic method was moved from ReflectionMethod to ReflectionFunctionAbstract

[AndrolGenhald]

The PHP documentation is apparently not up-to-date with this. If this was an actual change in PHP shouldn't this go in the 8.1 callmap, or wherever the change was actually made? Or has the PHP documentation just always been wrong?

[orklah]

This is definitely a fix added in 8.1. I wondered if I should have removed the method and re-added it too, that's why I asked @weirdan to review. I'll probably make the change and see if it breaks something tomorrow

[weirdan]

I wondered if I should have removed the method and re-added it too

Yeah, I believe this is what we should do.

[weirdan]

This needs to be added to 8.1 delta

[orklah]

Great, when fixing properly, the error comes back. I'll try to debug that deeper to see where it goes wrong

[weirdan]

I think this could be caused by reflection / callmap mismatch. When running on 8.1, isStatic() is defined on ReflectionFunctionAbstract, but the resulting callmap for 7.1 (that we target in this job) does not have ReflectionFunctionAbstract::isStatic.

Idea for Psalm 6 (or 7): stop using reflection to infer types.

[orklah]

Yeah, that's probably it. I'm not sure of the interactions between stubs, callmap and reflection though...

[orklah]

Well adding the function to the stub works. I say we should merge like this.

Idea for Psalm 6 (or 7): stop using reflection to infer types.

Or use each PHP version to generate pseudo-stubs from reflection that we could load for each PHP version instead of relying to the Reflection module from the version we're in. I'm not even sure we would need to wait for a major version to do that...

We would immediately:

• improve our types
• get rid of some stubs that are not versioned (Reflection.phpstub describe ReflectionMethod as not extending ReflectionFunctionAbstract but we can't add it without breaking older versions)
• probably improve run time because Reflection would not be needed

I could try to work on that next week if you're okay with it

[AndrolGenhald]

@orklah I'd be interested in looking into this. I was planning to create a big issue listing all of the extension defined functions we have in callmap so we can pull them out into extension stub files after #7107 is merged. I was also planning something like improving @since support and maybe adding something like @until or @removed so we could just have one stub file for an extension that supports all PHP versions. This seems pretty closely related to what's going on here.

[orklah]

Well, things like changes in inheritance leave me dubious about what can be done in a single file.

Plus I think it would be cool to be able to just have a tool that generates the cache files as needed, that way, on a new PHP version, we just run the tool and saves the result. No need to think about how we should modify the existing stubs that dates back to PHP 7.1 in order to not break it

But if you have a consistent proposal, go ahead, if it's not enough, it will always be time to improve later

[AndrolGenhald]

That's a good point, if we have a tool to generate stubs automatically it's a lot less effort to maintain a stub for each version. How often would we need to make manual changes though? I imagine there are a good number of places where stubs would need to be changed to use eg numeric-string.

[orklah]

I'm not sure, I did not check this in details yet. If there are a lot, it could be an issue indeed. Maybe some "patch" system to the generation tool that takes the reflection result and apply patches to it?

[weirdan]

The only upside to using reflection I can think of is supporting unknown extensions. Replacing reflection fallback with predefined stubs would break this use case - so I think it's better done in a major version.

Building stubs would require having a runtime with all extensions enabled, it could be somewhat tricky to set up.

Note that we already have https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Stubs/Generator/StubsGenerator.php, which could be used as a starting point for this.

[orklah]

We wouldn't remove the Reflection module completely, only render it useless for loaded stubs. If the stub is missing, we fallback to Reflection. That would allow us to ask users to send us the files needed for specific version themselves if there is a specific config...

[orklah]

@weirdan can we merge this in the meantime? It's needed to start merging other PRs :)

[AndrolGenhald]

We wouldn't remove the Reflection module completely, only render it useless for loaded stubs. If the stub is missing, we fallback to Reflection. That would allow us to ask users to send us the files needed for specific version themselves if there is a specific config...

I like that idea, best of both worlds. Reflection keeps working for unknown extensions, but we modify it to print a notice that for full support for the unknown extension they should open an issue, and perhaps give an easy way for them to generate stubs that can be attached to the issue or used in a pull request.