Psalm: No way to prevent psalter adding backwards-incompatible changes for packages that extend your classes

[iluuu1994]

This is probably more of a question than an error report.

We have our own legacy framework that we use as a composer package in many projects. I'd like to improve type hints using psalter but the problem is that psalter adds return types even when that is not safe.

For example, psalter will add native void return types. Unfortunately, some of those methods will be overridden in the project and return something.

Basically, my question is: Can the code base be marked as a library? In that case psalter should not add native type hints to code that can be overridden. I'd still like to have native type hints for final classes or private methods.

I realize this is probably not a common use case, feel free to close this if you think it's not worth it.

[muglug]

Are there Psalter bugs?

[muglug]

Which is to say, Psalter should not be doing anything that’s unsafe if you provide the right arguments to it.

[iluuu1994]

Sorry if I wasn't clear enough. I'd like psalter not to add native return types unless it's safe to do so (class is final or method is final or private).

Legacy library

class TestCommand extends Command
{
    protected function execute(InputInterface $input, OutputInterface $output)
    {
        // ...
    }
}

After psalter --issues=InvalidReturnType

class TestCommand extends Command
{
    protected function execute(InputInterface $input, OutputInterface $output): void
    {
        // ...
    }
}

If the projects overrides the TestCommand it will need to add the : void type hint. This is a breaking change which I'd like to avoid. Otherwise PHP will error. What flags am I missing?

[muglug]

This is a bug - Psalter should never add non-docblock types when they'd conflict with an inherited type.

[muglug]

Can you provide an example of the bug (in the form of a failing test inside tests/FileManipulation/ReturnTypeManipulationTest.php)? I'm not able to reproduce

[iluuu1994]

Added a test here: #1629

[muglug]

Thanks - I've amended that slightly to

            'fixInvalidIntReturnTypeJustInPhpDoc' => [
                '<?php
                    class A {
                        /**
                         * @return int
                         * @psalm-suppress InvalidReturnType
                         */
                        protected function foo() {}
                    }

                    class B extends A {
                        /**
                         * @return int
                         */
                        protected function foo() {}
                    }',
                '<?php
                    class A {
                        /**
                         * @return int
                         * @psalm-suppress InvalidReturnType
                         */
                        protected function foo() {}
                    }

                    class B extends A {
                        /**
                         * @return void
                         */
                        protected function foo() {}
                    }',
                '7.3',
                ['InvalidReturnType'],
                false,
            ],

[iluuu1994]

Cool, thanks!

[iluuu1994]

This one is still wrong:

'muglugIsBetterAtNamingThings' => [
    '<?php
        class A {
            /**
             * @return int
             */
            protected function foo() {}
        }',
    '<?php
        class A {
            /**
             * @return void
             */
            protected function foo() {}
        }',
    '7.3',
    ['InvalidReturnType'],
    false,
],

[muglug]

That should be a perfectly safe replacement, no?

[iluuu1994]

Some class could extend A and override the foo function. If A::foo() suddenly has the : void return type all subclasses must also add : void to their function declaration. This would be a breaking change in our library.

[iluuu1994]

If it's not completely clear, the example test above still fails. : void is still added.

[muglug]

Some class could extend A and override the foo function.

Sure, but that would have to happen in a separate project (as Psalm detects usages of A in the current project).

To avoid BC changes you should pass --php-version=5.6 (so no new PHP return types will be added).

[iluuu1994]

but that would have to happen in a separate project

Exactly. That's why I wrote in the original description of this issue:

Can the code base be marked as a library?

To avoid BC changes you should pass --php-version=5.6 (so no new PHP return types will be added).

Isn't that kind of a workaround? Other changes might not be applied that could be. You could also drop 5.6 support in the future where this would become impossible to do.

[muglug]

It's definitely a workaround, but there are no plans to drop support for 5.6-compatible replacements.

To your broader point, sorry for not grasping what you were asking for earlier.

This is not a priority to fix, but you're very welcome to open a PR.

It might look like --allow-backwards-incompatible-changes=false, with commensurate checks in ReturnTypeAnalyzer for final/private methods

[iluuu1994]

No worries mate. I'll create a PR soon 👍