Recent change now incorrectly requires @psalm-pure on all abstract methods and interfaces

[kkmuffme]

https://psalm.dev/r/ba3f7c9dd5

The recent changes now incorrectly require a @psalm-pure annotation on all abstract methods and interface methods, which is obviously wrong, as it forces all implementations to be pure too @danog

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/ba3f7c9dd5

<?php

abstract class A {
    abstract public function run( array|false $arg, string $b ): array|false;   
}

interface B {
    public function run( array|false $arg, string $b ): array|false;   
}

Psalm output (using commit 491dd74):

ERROR: MissingAbstractPureAnnotation - 8:21 - run must be marked with one of @psalm-pure, @psalm-mutation-free, @psalm-external-mutation-free, @psalm-impure to aid security analysis

ERROR: MissingAbstractPureAnnotation - 4:30 - run must be marked with one of @psalm-pure, @psalm-mutation-free, @psalm-external-mutation-free, @psalm-impure to aid security analysis

[kkmuffme]

e.g. fixing it for the abstract, it will result in an ImmutableDependency error then in the implementer https://psalm.dev/r/9e9d2adb02

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/9e9d2adb02

<?php

abstract class A {
    /**
     * @psalm-pure
     * @param array{hello: int|non-empty-string, world: array}|false $arg
     * @param string $b
     * @return array{hello: int|non-empty-string, world: array}|false
     */
    abstract public function run( array|false $arg, string $b ): array|false;   
}

class B extends A {
    /**
     * @param array{hello: int|non-empty-string, world: array}|false $arg
     * @param string $b
     * @return array{hello: int|non-empty-string, world: array}|false
     */
    public function run( array|false $arg, string $b ): array|false {
     	if ( rand( 0, 1 ) ) {
            return false;
        }
        
        return $arg;
    }
}

Psalm output (using commit 491dd74):

INFO: ImmutableDependency - 19:5 - A::run is marked at least @psalm-external-mutation-free, but B::run is not marked @psalm-external-mutation-free

[kkmuffme]

I mean, I get the point, since MissingAbstractPureAnnotation was added for it, however practically, this only adds noise:

• you have to add @psalm-pure on all abstract classes and interfaces now, if they're actually pure - not going to happen in most code bases since that will be a major breaking change and usually there are very few classes that have only pure methods to begin with
  OR
• add @psalm-pure to all abstract methods you want pure and @psalm-suppress for all other abstract methods - lots of work and lots of potentially breaking changes for many people/implementers
  OR
• disable MissingAbstractPureAnnotation for the whole code base. From that issues description however, it's unclear whether this has any negative impact on security analysis?
  The description makes it sound that it would, but practically I wonder how? Since the abstract/interface methods purity is irrelevant for taint analysis anyway, since what matters is the purity of the implementing method (and for that - if a method is pure - psalm has been reporting any auto-fixable error since ages already anyway)
  Could you explain that here (or in that issue description)?

Also I saw that @psalm-impure is added in the description there too and is a newly added tag.
Given that for psalm the default has always been "impure" (unlike for phpstan, which has always treated everything as pure, unless annotated as @phpstan-impure), this seems redundant - why require an @psalm-impure when that's already the default anyway?

Additionally, this will lead to stuff like this: https://psalm.dev/r/b36ecd7eb3 where the class is marked pure, but then individual methods are marked impure.
That's not possible for other things either, neither in PHP itself nor in psalm (can't have a "final" class and then mark some methods "unfinal", or "readonly" class with "actually this property writable") for good reason.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/b36ecd7eb3

<?php

/**
 * @psalm-pure
 */
abstract class A {
    /**
     * @psalm-impure
     */
    abstract public function run( array|false $arg, string $b ): array|false;   
}

Psalm output (using commit 491dd74):

No issues!

[danog]

The naming of the new issue is incorrect, will likely rename it since it's not about adding @psalm-pure to all abstract methods, it's about explicitly specifying the purity level of all abstract methods, which can be psalm-pure, psalm-mutation-free, psalm-external-mutation-free or even psalm-impure.

In turn, this fixes the problem where an implementing method cannot be auto-marked as pure/mutation-free/etc because the abstract method it implements is not marked as pure, because there is no way to infer the purity of an abstract method especially if it has no implementors (if it has implementors it can be done, and I'm currently working on graph-based purity inference logic to allow for this).

About https://psalm.dev/r/b36ecd7eb3, this is actually a bug, but either way, @psalm-impure was added specifically for abstract methods whose implementors are allowed to be impure (which is useful for example in cases where you pass around an object typed as the interface, and you need to know the guaranteed purity level of implementors).

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/b36ecd7eb3

<?php

/**
 * @psalm-pure
 */
abstract class A {
    /**
     * @psalm-impure
     */
    abstract public function run( array|false $arg, string $b ): array|false;   
}

Psalm output (using commit 491dd74):

No issues!

[danog]

Thinking about it, that issue could probably be removed altogether when graph-based purity inference is added