Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsound behaviour allowed with return static #8809

Open
muglug opened this issue Mar 14, 2021 · 4 comments
Open

Unsound behaviour allowed with return static #8809

muglug opened this issue Mar 14, 2021 · 4 comments
Labels

Comments

@muglug
Copy link
Contributor

muglug commented Mar 14, 2021

<<__ConsistentConstruct>>
abstract class C1<T> {
    final public function __construct(private T $value) {}

    public static function from(mixed $value): this {
        return new static($value);
    }

    public function getValue() : T {
        return $this->value;
    }
}

final class C2 extends C1<string> {}

<<__EntryPoint>>
function main() : string {
    return C2::from(10)->getValue();
}

Expected behavior

Some sort of typechecker error maybe?

Actual behavior

No error (but running this produces a TypeError)

Environment

hhvm 4.100

Would be very interested to know how this might be handled. Related issue here: vimeo/psalm#5383

@muglug
Copy link
Contributor Author

muglug commented Mar 14, 2021

I think the only way to prevent this is to prohibit new static for all generic classes

@fredemmott
Copy link
Contributor

Thanks - FB T87049790

@jthemphill
Copy link
Contributor

On T87049790, @dlreeves wrote:

Hmm we have known about this unsoundness at least since when we introduced reified generics, which is why we banned using new static in this way if T is a reified generic. Haven’t tested this yet, but I feel this particular unsoundness would go away if we used a where constraint in some way. Something like where this as C<mixed>. Then at the call site we will initiate the this type to be C2 which would fail the constraint C2 <: C and so we wouldn’t allow the method to be invoked.

If that does work then the trick now is figuring out how to force this constraint when declaring the method. Don’t have a great idea for that currently.

@muglug
Copy link
Contributor Author

muglug commented Apr 10, 2021

FYI I solved this in Psalm by introducing a @psalm-consistent-templates annotation that works similarly to @psalm-consistent-constructor (which itself duplicates the behaviour of <<__ConsistentConstruct>>).

Using it forces all child classes to have the exact same template params with the same constraints and requires them to map directly to the parent params. new static is then prohibited in any function whose signature indicates it returns something involving static where that annotation is not also present on the class.

<<__ConsistentConstruct>>
<<__ConsistentGeneric>>
abstract class C1<T> {
    final public function __construct(private T $value) {}

    public static function from(mixed $value): this {
        return new static($value); // would be allowed
    }

    public function getValue() : T {
        return $this->value;
    }
}

final class C2 extends C1<string> {} // fail

final class C3<T3, TOther> extends C1<T3> {} // fail

final class C4<T4> extends C1<mixed> {} // fail

final class C5<T5 as object> extends C1<T5> {} // fail

final class C6<T6> extends C1<T6> {} // pass

@lexidor lexidor added the hack label Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants