Psalter with MissingPureAnnotation makes unnecessary docblock changes

[still-dreaming-1]

Some psalter options make a lot of unnecessary changes to the docblocks by reformatting them to have a "blank" docblock line between each annotation even if it didn't make any other changes. For example, this will do that:

psalm --no-cache --alter --issues=MissingPureAnnotation

One reproduce is when it finds something is already correctly annotated as @psalm-pure and has a @psalm-assert annotation below that. It doesn't seem to have this issue with some other annotations such as if @psalm-return were below @psalm-pure instead of @psalm-assert. Here is an example:

https://psalm.dev/r/ae01ce15ca

For isTrue() it will add an extra docblock line between the annotations. A more destructive example is that with isFalse(), it will delete the code comment describing what the method does and turn it into a "blank" docblock line.

I wanted to see what other people think about this. Do you consider it standard to have an extra line between each annotation? Are there standard PHP tools that will automatically adjust all the docblocks at once to be formatted this way? Do you think it is appropriate for Psalm to make formatting changes in this way?

Personally I think it goes a long way toward making a very powerful tool like psalter a lot less practically usable because it takes a long time to go through each change and decide if you want to keep it or not. Ideally you could configure it such that you can run it frequently and expect it to produce no changes if you already have a clean code base. When it does produce changes you should typically like the ones it makes or be able to configure it to stop making those particular changes.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/ae01ce15ca

<?php

declare(strict_types=1);

final class Asserter
{
    /**
     * @psalm-pure
     * @psalm-assert true $expectedToBeTrue
     */
    public static function isTrue(mixed $expectedToBeTrue): void
    {
        if ($expectedToBeTrue !== true) {
            throw new \Exception('not aw');
        }
    }
    
    /**
     * @psalm-pure
     * Asserts that something is false
     * @psalm-assert true $expectedToBeTrue
     */
    public static function isFalse(mixed $expectedToBeFalse): void
    {
        if ($expectedToBeFalse !== false) {
            throw new \Exception('not aw');
        }
    }
}

Psalm output (using commit ef02ded):

No issues!

[orklah]

Note that there is a --add-newline-between-docblock-annotations=false option if you ask Psalter to output its options. It's not perfectly implemented everywhere, but it will help reduce noise a lot.

However, Psalm/Psalter will never match your code style perfectly and it's not really its role. We should probably says somewhere in the docs that we encourage using it with a CS tool behind in order to fix every rule that is not correct according to your standard

[still-dreaming-1]

I will try it. To me it seems like a bug for it to make unnecessary changes that has nothing to do with what you asked it to do. In the example I posted, it didn't find anything to change, yet it still made stylistic changes anyways. It also deleted some of the code comments in that example.

[orklah]

Yep, those are bugs. We have a detection of change docblocks but it's not perfect yet

[still-dreaming-1]

I tried using that option like this: psalm --no-cache --alter --issues=MissingPureAnnotation --add-newline-between-docblock-annotations=false. It is still adding the docblock line between annotations.

[orklah]

Well, it still needs some improvements then...

The variable that tracks if a docblock must be re-rendered is here:

psalm/src/Psalm/Internal/FileManipulation/FunctionDocblockManipulator.php

Line 340 in 8d36bdc

$modified_docblock = false;

And here's the place where we render the docblock when modified and where we use the config I mentionned:

psalm/src/Psalm/Internal/Scanner/ParsedDocblock.php

Line 62 in b924032

&& static::shouldAddNewLineBetweenAnnotations()

If you want to take a look...