search

vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.57k stars 660 forks source link

Infer alternate PHP versions #8130

Open S1SYPHOS opened 2 years ago

S1SYPHOS commented 2 years ago

Hey there, I only started using psalm, but one thing I think could be improved: When depending on PHP like ^7.4|^8.0, the inferred version is 7.4, while both versions should be checked.

Cheers, S1SYPHOS

psalm-github-bot[bot] commented 2 years ago

Hey @S1SYPHOS, can you reproduce the issue on https://psalm.dev ?

S1SYPHOS commented 2 years ago

Hey there little fellow, that's not possible, sorry! :fox_face:

AndrolGenhald commented 2 years ago

Can you be more specific? What is it that you want checked for PHP 8 that the checks for 7.4 don't provide?

S1SYPHOS commented 2 years ago

It just occurred to me that if checks are different between versions, they should all be inferred - and if it doesn't make any difference, why chose the first one detected, not the second? We might as well chose the most current one, being v8.0 in my example 😀

AndrolGenhald commented 2 years ago

Because if your project supports PHP 7.4 we want to make sure you don't use features introduced in PHP 8.0. The only time this gets complicated is when PHP deprecates something, but I think we generally try to show deprecation warnings for earlier versions to make sure those sorts of things can still get caught.

It's entirely possible there are deprecations in 8.0 or 8.1 that aren't being warned about in 7.4, and if that's the case please do open an issue about it.

@orklah I think we generally want to warn about deprecations in all versions, even if the deprecation happens in a later version right? Otherwise you could have a case where you support 7.4|8.0|8.1 and something was deprecated in 8.0, removed in 8.1, but there's no warning from the 7.4 analysis.

S1SYPHOS commented 2 years ago

Thanks for explaining it in detail, that was helpful!

orklah commented 2 years ago

I'm not even sure we warn about deprecated functions though. Your point stays the same with removed functions however.

We could probably document that people could (should?) run at least in the oldest and newest version supported, but it's tiresome to maintain two baselines and two configs...

AndrolGenhald commented 2 years ago

I was thinking there was something we warned about, but maybe I'm misremembering. I know we discussed some ideas around this in #7512, but truly supporting full checks for multiple versions at the same time is something that's probably not going to happen any time soon.

dereuromark commented 2 years ago

I also dont like that one is forced to remove >= php 7.x from composer json to avoid

run in compatibility mode for older PHP version even when using PHP 7.4. It shows "Target PHP version: 7.2 (inferred from composer.json)".

Ideally we have a flag to control this. Either we want or dont want the compatibility mode to be enabled based on inferred composer.json

But now people seem to be forced to drop the PHP info in there only to please the tool here.

AndrolGenhald commented 2 years ago

I also dont like that one is forced to remove >= php 7.x from composer json to avoid

run in compatibility mode for older PHP version even when using PHP 7.4. It shows "Target PHP version: 7.2 (inferred from composer.json)".

Ideally we have a flag to control this. Either we want or dont want the compatibility mode to be enabled based on inferred composer.json

But now people seem to be forced to drop the PHP info in there only to please the tool here.

What does "run in compatibility mode for older PHP version" mean? You can override the PHP version in Psalm's config, but it's unclear to me why you'd want to declare support for PHP 7.2 but then not have Psalm check that everything actually works with PHP 7.2.

dereuromark commented 2 years ago

We are running both 7.4 and 8.0 in our CI, So we want to use respective versions in the CI, not just the min The min is covered already by one of them. Having this hardcoded from composer reading seems not logical to me.

AndrolGenhald commented 2 years ago

We are running both 7.4 and 8.0 in our CI, So we want to use respective versions in the CI

That's something we could definitely improve support for, by adding better documentation on setting that up if nothing else.

The min is covered already by one of them.

Is it? If you're trying to have it run with 7.4 and 8.0 then it's never going to be run for 7.2 right? What happens if you use an arrow function or something? Maybe other tests will catch it, but Psalm won't be able to.

Having this hardcoded from composer reading seems not logical to me.

That's an oxymoron, it's not hardcoded if it's coming from a config (and it's also possible to override it with Psalm's config). It also seems totally natural to me, wouldn't most people want to make sure their code works with the minimum supported version?

dereuromark commented 2 years ago

Sry, that was another repo, the one I am now mentioning was 7.4 + 8.0 The other 7.2 + 8.x So either way, we would like to run the tool on the language itself we are testing without it using this derived resolution Is there a config to switch this off?

Yes minimal version is tested, always. With one of those CI runs.

AndrolGenhald commented 2 years ago

Sry, that was another repo, the one I am now mentioning was 7.4 + 8.0

Ok, that makes more sense.

So either way, we would like to run the tool on the language itself we are testing without it using this derived resolution Is there a config to switch this off?

Right now I think there's only the phpVersion in the config that overrides it and the --php-version flag for the CLI, so if you want to run in multiple versions on CI I'd recommend using the --php-version flag. I don't think there's currently a convenient way to automatically run with the same version as the PHP installation.