Add configuration option to disable @var parsing everywhere except for properties.

[danog]

Ondrej is already planning to remove @var support in phpstan, and I also propose disabling parsing by default in Psalm v5.

[AndrolGenhald]

If we're going to add this, I think it would be nice to add an issue whenever inline @var tags are encountered when that option is set, that way they can be removed instead of hanging around for no reason. On the other hand, maybe they're still needed for PHPStorm or other static analyzers, so perhaps that should be a separate option? Should probably go with #7369.

[orklah]

I agree with Ondrej on this but his plans are way more sensible than just dropping the feature.

@var are sometimes needed to refine types from third party libs, for IDEs or even to circumvent bugs from static tools themselves. It would not be reasonable to just disable this

This is an interesting topic but we're gonna have to take this slowly to change habits of users.

Here's a few ideas:

• creating a @psalm-upcast and @psalm-downcast that would allow refining (or widening a given type) when used. For example, @psalm-downcast would be accept to turn object into myObject but it would fail to turn object into string or to turn myObject into mixed
• Emitting an error when the type in @var is not contained in the current inferred type. (which would limit it to only downcasting)
• allowing docblock assertions on variables. For example, if you retrieve a string from somewhere, instead of overriding the type with @var into non-empty-string, you would @psalm-assert !empty and Psalm would change it into non-empty-string. That way, when the library change the API, the assertion will fail and not hide the issue

Ultimately, we will possibly want to drop support for @var to prevent cross-usage between IDEs and static tools who use it in different context(autocomplete vs analysis), but not before we have tools to help users first

[danog]

@var are sometimes needed to refine types from third party libs, for IDEs or even to circumvent bugs from static tools themselves.

Disabling @var support in Psalm caused virtually no additional issues at work.
IDEs can still independently handle @vars just fine, while bugs from static tools should be reported and fixed, not hidden away with a @var.
Types from third-party libraries can also be fixed upstream with a MR, or refined using assertions and/or wrapper functions.

@psalm-upcast ... @psalm-downcast

No offense, but this seems like a horrible feature to implement into a static analysis tool like Psalm.
We're basically saying that we're OK with the fact that Psalm isn't capable of inferring the correct type, and we're forcefully specifying a custom type, overriding the actual type which may actually change with time, leading to undiscovered bugs.

allowing docblock assertions on variables

This is already possible using runtime assertions, which are actually way safer than an unchecked @psalm-assert for types coming from third-party code.

[orklah]

Disabling @var support in Psalm caused virtually no additional issues at work.

Then I'm pretty happy your job managed to keep things clean, but a lot of users have big hairy legacy codebase to take care of, including me :)

Types from third-party libraries can also be fixed upstream with a MR, or refined using assertions and/or wrapper functions.

Again, if you actually have time to wrap a whole library for static analysis purposes and if every dependency you have is up to date, this is cool!

At my work we have a 2009 Html2Pdf manually patched to run on PHP 8.0. We don't have time or resources to update it, without even starting to talk about patching it upstream or wrapping it for Psalm to understand it all

We're basically saying that we're OK with the fact that Psalm isn't capable of inferring the correct type

Psalm capabilities are not the whole issue here. There will always be new tools that don't use Psalm at all (starting from PHP-Parser, the thing that makes Psalm and PHPStan runs)

You have to remember that not all users have the same use-case as yours. The goal of Psalm is to help find bugs in application before running it, not to annoy users for the sake of quality.

Disabling widely used features is the opposite of helping people. They may be dangerous in some cases, but if you take into account that those using the features may have thousand of issues left in their baseline, it kinda make sense to give them power tools, not toys

[danog]

but a lot of users have big hairy legacy codebase to take care of, including me
They may be dangerous in some cases, but if you take into account that those using the features may have thousand of issues left in their baseline, it kinda make sense to give them power tools, not toys.

I agree with your power tool argument, however I really don't feel like @var is a useful tool at all: @psalm-suppress and runtime assertions can just as easily replace it, and runtime assertions are even safer.

I currently have 122k issues in my baseline :)
Our codebase is huge and only recently it's begun to clear up a bit.
I guess we're luckier because we mostly use in-house libraries which can be easily updated, and we also have no problem with forking a 3rd party library to our github org when needed.
We used to use @var a lot at work, mostly for typing foreach variables, before I added generics to all of our iterators.
Once that was done, virtually no valid usecase was left in our case, and I feel that most codebases are like this: they just need a bit of love and typed iterators, and @var becomes useless very quickly.

Anyway, this PR is just about introducing a flag; while I still strongly feel about making this a default in v5, I'd be more than glad to hear more arguments for possible usecases and solutions.

[pilif]

To give some perspective based on a 18 year old code-base (300k lines) which we started using psalm with 3 years ago, I can say that most of our @var usages are actually to work around bugs in psalm itself (annoyingly in some cases a reduced test case is fine, but the same code wrapped in 🍝 causes psalm to infer wrongly) and to help with wrong docblock annotations in third party libraries.

In both cases, the @var annotation is better than a suppression because suppressions stay forever and can hide future issues due to a lack in granularity of where they can be applied. They also cause the error to propagate through the function stack, requiring further suppressions to be added, making the issue worse.

Plus, due to the way how suppressions are handled in psalm before IssueBuffer::maybeAdd was added, suppressions can have the side-effect of altering further analysis which is way worse than the downsides of @var when used responsibly.

So long as many reasons for using @var are issues with psalm itself, I think it would be quite a usability downgrade to remove this useful helper.

[danog]

annoyingly in some cases a reduced test case is fine, but the same code wrapped in spaghetti causes psalm to infer wrongly

Hehe, this was my exact problem when I first started introducing psalm to our 830k codebase, to the point where I actually started writing a psalm plugin that could automatically generate reduced testcases by iteratively removing elements, didn't quite finish it but I still have it on a branch of our psalm fork :)

The @var annotation is better than a suppression because suppressions stay forever and can hide future issues

Suppressions can hide future issues of the same type, yes, which is why I also added a configuration key to disallow usage of @psalm-suppress all to at least somewhat reduce the amount of wrongly suppressed issues.
On the other hand, @var also stays forever, so even if an upstream library actually adds psalm typing and the correct typing is different from the one overridden using @var, Psalm won't emit a UnusedSuppress warning, unlike with suppresses.

Now that I think of it, maybe something similar to an UnusedVar could be emitted if the type is not contained anymore in the inferred type (basically the 3 suggestions @orklah made), though that's for another PR :)

I see that quite a few people are against making this a default in v5, that's fine, this PR will just add a configuration key that can be enabled if people need it.

[weirdan]

I'd be more than glad to hear more arguments for possible usecases and solutions

One important use case is overriding inferred generic type: https://psalm.dev/r/86fce654ba

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/86fce654ba

<?php

/** @var Collection<int> */
$c = new Collection([]);

/** @psalm-trace $c */;

$c->add(123); // will complain unless collection type is specified via @var

/** @template T */
class Collection {
    /** @param array<array-key, T> $items */
    public function __construct(private array $items) {}
    
    /** @param T $item */
    public function add($item): void { $this->items[] = $item; }
}

Psalm output (using commit 9168cef):

INFO: Trace - 6:23 - $c: Collection<int>

[danog]

One important use case is overriding inferred generic type

This is the reason why I added template support to @psalm-this-out: https://psalm.dev/r/468f504fd9

Although I admit as per your #7351, it still requires a use of @var (but only internally, not @ use!): I probably should change the type of $this before entering the method, not afterwards.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/468f504fd9

<?php

/** @template T */
final class Collection {
    /** @param array<array-key, T> $items */
    public function __construct(private array $items) {}
    
    /**
     * @template NewT
     * @param NewT $item 
     * @psalm-this-out self<NewT|T>
     */
    public function add($item): void { $this->items[] = $item; }
}

$c = new Collection([]);

/** @psalm-trace $c */;

$c->add(123); // won't complain

/** @psalm-trace $c */;

Psalm output (using commit 997592d):

INFO: Trace - 18:23 - $c: Collection<never>

INFO: Trace - 22:23 - $c: Collection<123>

ERROR: InvalidPropertyAssignmentValue - 13:40 - $this->items with declared type 'array<array-key, T:Collection as mixed>' cannot be assigned type 'non-empty-array<array-key, (NewT:fn-collection::add as mixed)|(T:Collection as mixed)>'

[weirdan]

@danog, that's two different use cases. One may want a collection that accepts anything (your example) and changes its type based on that, while others may need a collection that only accepts a particular type.

[danog]

that's two different use cases. One may want a collection that accepts anything (your example) and changes its type based on that, while others may need a collection that only accepts a particular type.

Oh I get it now 😅.
Yeah that's a valid usecase, though in our codebase we'd just use the first approach and rely on the final inferred type to do typechecks when the collection values are actually used.

[orklah]

@danog I'll merge that, can you rebase please?

[danog]

Done!

[orklah]

Thanks!