make superglobals more specific

[kkmuffme]

Make the types of superglobals like $_GET, $_POST,... more specific than "Mixed" to allow for simpler and stricter conditions and more effective psalm checks in many cases.

[kkmuffme]

Ready to be merged.

There are tons of reworks, as there is a bug in taint (#8477) that confused me, as I haven't used/worked with taint before and thought there's an issue in my code, while it was a bug in taint all along.

[AndrolGenhald]

We might have to think about this for the same reasons given in #1087. Mutating $_GET and other superglobals is a bad idea, but it's a bad idea that's still somewhat common unfortunately.

[kkmuffme]

You can still mutate $_GET just like before. The behavior for this isn't changed, is it?

Nevertheless, I think psalm should not account for niche, bad coding practices (since there's psalm-suppress and @var anyway) - there are very limited maintainer resources it being open source, so we shouldn't put too much thought into what is a 0.01% niche issue and a (very) bad coding practice in the first place to be honest. (but if anybody wants to fix it, we can still be open for it, I just don't think we should waste time with those things)

[staabm]

One way could be to make the more precise type inference a opt-in feature and also add a rule which reports errors when code modifies superglobals

[kkmuffme]

add a rule which reports errors when code modifies superglobals

Definitely, but not within this PR. Please feel free to create a separate PR for that.

One way could be to make the more precise type inference a opt-in feature

It already is. Psalm already supports declaring any globals in your config. However this PR is not about that - it's about declaring what those ~6 generally available superglobals can contain in every environment (if you didn't specify that yourself in your config already).

[AndrolGenhald]

This might be something we'd want to consider.

[kkmuffme]

This might be something we'd want to consider.

Actually, that's not the only issue. You can also have an auto_prepend_file in php.ini which changes ALL of them (including these), therefore nothing is safe here. Adding any conditions/checks for these few, would create a false sense of "security", as all of them can be changed in an auto_prepend_file or any file that runs before the current one in fact - which is an issue with all globals anyway.
Alas, don't need to consider this for this PR.

[AndrolGenhald]

which is an issue with all globals anyway

If we don't have one already, we should open an issue to make superglobals immutable.

[kkmuffme]

@orklah all changes done, branch rebased.

Shepherd has a bug it seems, bc I don't get an error when I do this here: https://psalm.dev/r/66e2a12517 (also this error makes no sense)

$this->argv with declared type 'list' cannot be assigned type 'non-empty-list'

I think this is caused by having "possibly_undefined" and "ignore_nullable_issues" added as you suggested.
I mean I can just fix this in the test by changing it to non-empty-list, but it will be a pain for everybody else.
What do you suggest?

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/66e2a12517

<?php

class Foo {
    /**
     * @var array<int, string>
     */
    private $argv = [];
    
    /**
     * @param non-empty-list<string> $arg
     */
    public function __construct($arg) {
    	$this->argv = $arg;
    }
}

Psalm output (using commit d957ff2):

No issues!

[orklah]

I think this is caused by having "possibly_undefined" and "ignore_nullable_issues" added as you suggested.

Yeah, it's probably due to the check for InvalidPropertyAssignmentValue considering that possibly_undefined could lead to null and not checking ignore_nullable. Can you coalesce with empty array here? It should solve the issue

[kkmuffme]

Yes, but that's what I had before with TNull already, however the error made much more sense then.

Because the error now doesn't make any sense:

$this->argv with declared type 'list' cannot be assigned type 'non-empty-list'

The reason why it reports this is because it's nullable, but psalm hides the fact that it could be "null".

Maybe putting back TNull and removing possibly_undefined make it better? let me try that. If not, I think I'll revert it as it was, bc at least the error made sense then.

[kkmuffme]

@orklah yeah it works how you want it when I do it with TNull and $argv_nullable->ignore_nullable_issues = true;

When using without TNull but doing:

$argv_nullable->possibly_undefined = true;
$argv_nullable->ignore_nullable_issues = true;

It reports those wrong errors.

Changed now.

Ready to be merged now.

[orklah]

Thanks!

[olleharstedt]

I think this PR broke our CI. See my latest bug here: #8559