rectorphp / type-perfect

Next level type declaration check PHPStan rules
https://getrector.com/blog/introducing-type-perfect-for-extra-safety
MIT License
73 stars 3 forks source link

Provide more specific return type "ConcreteImpl" over abstract one #2

Closed staabm closed 3 months ago

staabm commented 3 months ago

I wonder why the package suggests to use a more specific return type, if a method is typed using a interface type.

<?php

namespace IfaceOverConcrete;

class Foo {
    public function doFoo(): MyInterface { // Provide more specific return type "IfaceOverConcrete\Impl1" over abstract one
        return new Impl1();
    }
}

interface MyInterface
{

}

class Impl1 implements MyInterface
{

}

having a method typed with an interface type instead of a concrete subtype is the better way regarding OOP. I think the suggestion in this case is wrong and misleading.

typing a concrete impl will leak implementation details over concepts (the interface type or abstract class type) and hurt swappability


what is the idea of this rule? I can't think of a example when using a concrete subtype would improve something.

other linting tools I used in the past even propose the other direction. swapping concrete-impl types with interface-types in case non impl specific methods are used anywhere

TomasVotruba commented 3 months ago

In your provided example, there is no added value in using more generic return type. Its like having scalar return type, but returning a string :)

The goal is to report exact type possible, unless required by some parent interface. In some cases, interface can require methods that are no longer used.

If exact type is provided, this can be checked.

TomasVotruba commented 3 months ago

Also, classes can have extra public methods over interface. I recall that's how this rule started. There was false positive on extra public method.

staabm commented 3 months ago

So it sounds like this rule is reporting errors in the common case which only work for edge-cases? Should it be opt-in?

TomasVotruba commented 3 months ago

Purpose of this rule is to eliminate these types of hard-to-find errors and avoid leaky design. As this whole package :) Generic interface should be used only if replaceability is needed. If it doesn't fit your preference, feel free to ignore it.