Vscode-intelephense: return type "self" should be interpreted covariant'ly (PHP 7.4+)

[procodix]

First things first:

• thanks for a great VSCode extension!
• this is actually a really stupid PHP bug. If the language evolution would be steered by some educated OOP guy, this wouldn't have happend again.
• maybe this is not a bug, but the extension could handle it correctly, because currently it is handled as wrong as the php documentation is.

Now what's the bug? Since PHP 7.4 we have covariance return types. Class A as a return type can also be satisfied be returning a derived class B object. That works. But we have the abstract type placeholder self, which resolves to the declaring type, when it should be the calling type (In fact it behaves just as stupid, as it did when late static binding was missing).

I'll file a bug report, that we need a static return type. As type checking takes place during runtime we can check it at runtime against the calling class vs the declaring class. Maybe intellephense can leap forward and correctly interpret return type self as static, so that the follwing code does not indicate a "unknown method" problem.

class A {
    function Test(): self {return $this;}
}
class B extends A {
    function More() {}
}

(new B)->Test()->More();
// return type of Test() is assumed to be an "A" which doesn't have method More()

Thanks for reading & considering this.

[jfcherng]

class A
{
    /**
     * @return static
     */
    public function Test(): self
    {
        return $this;
    }
}

[procodix]

Yes, but this opens a DRY problem. Now that we have the syntactic type hints a big doccomment block is redundant.

But yes again, intellephense should handle a :self like a @return static

[KapitanOczywisty]

self and static are two different things in php, one is declaring class and second is currently called. Test code https://3v4l.org/WjC22
If we don't check what is in return itself (what extension often can't do), @procodix example is not entirely correct https://psalm.dev/r/b7f130b4d1 and @return static is clear way to fix that as provided by @jfcherng https://psalm.dev/r/deffdf3ec7
Also DRY is mostly about reusing code itself not documenting it. Redundand phpdoc is common even with typehinted parameters/return, there are some things which cannot be typehinted like type unions (at least in current php state).
Imho PHP should have static return type, but it's not best in oop terms.

[procodix]

DocComments may be useful for different things, but just to denote a simple return type is not anymore the use case, because that can be done by syntax. (Maybe some day we get ArrayOfClass return types like [string]).

Benefit: You have no duplication and you don't have to choose where to declare a return type everytime you write a method.

Allowing :self to mean :self and :static wouldn't hurt anyone not using it, but helps anyone wanting to use it. So, no disadvantage.

As to whether the example is correct, it is running successfully in PHP 7.4. Psalm's error is a false positive, as it gets confused by the return type. Fix it, by copying the Test Method to class B. Nobody would do that, because that is, what OOP inheritance does.

See here: https://psalm.dev/r/26e5f6f006

[jfcherng]

self and static are two different things in php, one is declaring class and second is currently called. Test code https://3v4l.org/WjC22

I think @KapitanOczywisty's example has shown that why :self cannot be assumed static unless intelephense knows what exactly the method returns at runtime.

Your example only shows that self === static works in your example but doesn't hold generally.

[KapitanOczywisty]

Psalm is way, way more advanced tool than intelephense and will just read return $true and ignore self (which is compatible, but not identical).

Mentioned Covariant Returns changed in php are only applicable when you're trying to override parent signature. This means that this is now possible: https://3v4l.org/flvW8

What you want is breaking Design by Contact, A::Test guarantees that it will return something compatible with self / A, but that doesn't mean it will return child, with self you can assume that anything from this class will work, but you can't be sure for anything from child classes.

Extension can as psalm treat return $true as static internaly, but probably irl code would be a bit more complex and self means nothing more compatible with me

[procodix]

self and static are two different things

True. However self caused many problems by not supporting late static binding. And it still does. Besides it is identical to __CLASS__ (!). Later PHP got the static keyword introduced to solve these problems. Today there is almost not point in using self, because static does the same plus it allows inheritance to work correctly. So self is different but has little use left.

:self cannot be assumed static

Why is that? static is always guaranteed to be either self or a descendant. Thus it is always call compatible with self - exactly what covariance is. The same way, that declaring to return A allows to return a derived B as well.

[KapitanOczywisty]

Today there is almost not point in using self

True or not, this is for bugs.php.net. For now self is what it is.

[jfcherng]

Why is that? static is always guaranteed to be either self or a descendant. Thus it is always call compatible with self - exactly what covariance is. The same way, that declaring to return A allows to return a derived B as well.

As long as (new B)->TestSelf()->ShowType(); from https://3v4l.org/WjC22 returns A, there is nothing needs to be discussed further. The truth is there. :self !== static.

[procodix]

What you want is breaking Design by Contact, A::Test guarantees that it will return something compatible with self / A, but that doesn't mean it will return child, with self you can assume that anything from this class will work, but you can't be sure for anything from child classes.

When declaring A:Test():self to return a self, it is guaranteed to return either A or B or anything else derived from A. So it is always some kind of A which is compatible to A.

Returning A or a child class means, it is guaranteed to be always compatible with A. And that is also working in PHP 7.4. (it could include void with :?self, btw). Whatever I return I can call anything from A on it.

PHP allows to call methods from B on it, if it is a B, even though I declared it to be an A. But intellephense doesn't allow it. That is a false problem report, because the code works.

[procodix]

@jfcherng

As long as (new B)->TestSelf()->ShowType(); from https://3v4l.org/WjC22 returns A, there is nothing needs to be discussed further. The truth is there. :self !== static.

If you check again, you'll see, that a B is returned, not an A. And according to the PHP compiler :self allows to mean A or anything extended from A like B. That is whay the code works with (new B)->...

So PHP's :self works with both A and B. Intellephense does not. Again this is a false error reported.

[procodix]

Put in one line:

PHP: self means A but allows A and B and C - behaves like static
Intellephense: self means only A

[jfcherng]

So PHP's :self works with both A and B. Intellephense does not. Again this is a false error reported.

PHP knows what is returned exactly because it actually runs the code while static analyzers don't. And yes, it's a false alarm. But it will get alerted by most of static analyzers I guess...

[procodix]

And yes, it's a false alarm.

Then I'm glad, that I opened a bug report and not a feature request :-)

And Intellephense has the capability to silence this false alarm by simply assuming self to mean static. That is what PHP does. The code is there, because it works correctly when I use @DocComment.

Why not handle it this way:
If return type of a method is self -> resolve the class. Next, assume it would be declared as @return static and resolve it again. If both return types equal, we can proceed with :self. If they are different, the return type is a extended class, we should assume :static to provide code completion for the right class.

I'd hate to have to write DocComments just to silent false alarms. That's bad practice.

[jfcherng]

If return type of a method is self -> resolve the class. Next, assume it would be declared as @return static and resolve it again. If both return types equal, we can proceed with :self.

This cannot be done perfectly without actually running codes since static is a runtime thing. If this can be done, it has been done (at least in other famous static analyzers).

I'd hate to have to write DocComments just to silent false alarms. That's bad practice.

It's a unfortunate compromise.

[procodix]

cannot be done perfectly without actually running codes

Why then does it work with @return static DocComment?

It's a unfortunate compromise.

That would be true, if it the false alarm will be fixed. Otherwise it is a deterioration of code. Either way "unfortunate" is the right word.

[jfcherng]

Why then does it work with @return static DocComment?

You are the designer. You know how your codes are being used. In your example, adding @return static, you directly claim (new B)->Test() returns B. Without this, static analyzers don't know it returns A or B before the codes get actually run.

[KapitanOczywisty]

When declaring A:Test():self to return a self, it is guaranteed to return either A or B or anything else derived from A. So it is always some kind of A which is compatible to A.

Returning A or a child class means, it _is guaranteed_ to be always compatible with A. And that is also working in PHP 7.4. (it could include void with :?self, btw). Whatever I return I can call anything from A on it.

You write this, and then something opposite

PHP allows to call methods from B on it, if it is a B, even though I declared it to be an A. But intellephense doesn't allow it. That is a false problem report, because the code works.

There is some weird cause and effect logic. I bought diesel so every car will work on diesel.

---

And Intellephense has the capability to silence this false alarm by simply assuming self to mean static. That is what PHP does.

PHP is running code, intelephense not. This is a big difference. PHP doesn't assume anything, just when given $this will compare compatibility with self.

Why not handle it this way:
If return type of a method is self -> resolve the class. Next, assume it would be declared as @return static and resolve it again

Why you don't just write static when you want to return static? You was complaining about lack of "educated OOP guy" and now you want to implement bad OOP. In other languages this would result in error ( https://rextester.com/LRD6710 ) in php this is allowed because OOP is not followed.

I'd hate to have to write DocComments just to silent false alarms. That's bad practice.

You're not silencing warnings, but telling IDE what you want to return.

[bmewburn]

A similar example without the confusion of self.

class A { }
class B extends A { }
class C extends A { }
function aFactory($name): A { 
    if($name === 'B') return new B;
}
$b = aFactory('B'); // What A? No it's B.

self resolves to the class it's used in. The _declaration_ below guarantees that calling Test will return an A. Because return types are covariant the _implementation_ can return whatever subtype of A it likes. What if the original example is made more complex by introducing a variable.

class A {
    function Test($x): self {return $x ? new static() : new self();}
}

What should intelephense do here? It has to go with what is declared and guaranteed not with what could be. I wouldn't even describe this as a false alarm, it's just that php doesn't have the syntax to declare such things and it needs supplementary info in phpdoc.

[procodix]

Misunderstanding. I meant to ask: Why does Intellephense work with the DocComment? This doesn't give a false alarm:

class A {
    /** @return static */
    function Test(): self {return $this;}
}
class B extends A {
    function More() {}
}

(new B)->Test()->More();

[bmewburn]

phpdoc is treated as supplementary to actual code and it is assumed that if the user has gone to the trouble of writing it then it is something important and the user probably knows better than the extension.

[procodix]

@bmewburn

I wouldn't even describe this as a false alarm

The extension doesn't report a warning but an error saying the method doesnt exist, while it does. That sounds a lot like a false alaram to me. And it shouldnt happen.

if the user has gone to the trouble of writing it then it is something important

So there is some logic that does the same job as the zend engine to figure out the right type for static. Maybe the same logic that determines the correct class called when I use static::DoSomething().

[KapitanOczywisty]

Zend engine don't have to figure out anything, at point when static:: is called it has class_entry for current object and called object. But key is that IDE is not running code, so cannot have these informations.

[procodix]

@bmewburn

class A {
    // made it static to call it directly.
    function static Test($x): self {return $x ? new static() : new self();}
}
class B extends A {} // and we want a derived class.

Interesting example. When calling A::Test(true | false) both calls will obviously return an A.

When Calling B::Test() what does it return? Either A or B. Truth is, I don't care, because that is as sensless as writing bogus code:

    function static Test($x):self {return $x ? new A() : new NotAnA();}

Here some other / similar edge case:

    function static Test($x):?self {return $x ? new static() : null;}

This code happens. And Intellephense handles it by showing return type of void|A. What about code completion? I didnt test, but I guess you decided to offer methods for A even though they might not exist on void. Could be handled similarly on self|static

[procodix]

@bmewburn

The issue isnt about returning one class or the other. My example from top always returns one deterministic type. PHP can determine it during execution and Intellephense seems to determine it during inspection by finding @return static and analyzing the invocations.

[procodix]

Zend engine don't have to figure out anything, at point when static:: is called it has class_entry for current object and called object. But key is that IDE is not running code, so cannot have these informations.

I beg to differ, Intellephense correctly find the class meant by static:

[jfcherng]

I beg to differ, Intellephense correctly find the class meant by static:

https://github.com/bmewburn/vscode-intelephense/issues/911#issuecomment-567637679

[procodix]

And for sake of completion the case without DocComment:

[procodix]

@jfcherng

adding @return static, you directly claim (new B)->Test() returns B

And thats the key point. You are wrong, I didnt. B is never mentioned as return type. The return type is not even mentioned in the B class but in A. So somehow Intellephense seems to walk the path back from static to through the class hierarchy and finds, that means B at this point. Either way it takes some steps to figure that static means "B" at this line of code.

BUT:

• I didnt do it
• PHP does it at runtime
• Intellephense does it (somehow) at code-time -> see screenshots.

[bmewburn]

My example from top always returns one deterministic type

Your _implementation_ returns the calling class. Your _declaration_ states that it returns the base class. Static code analysis works off declarations, even when inferring the return type this process starts with declarations of other symbols. With the added phpdoc the return type becomes static|A .

If there is no return type declaration and no phpdoc then the extension has no choice but to load the file and try and infer the return type. This is much more expensive than looking up a return type from the symbol index. If there is phpdoc and/or declarations then these are always preferred.

$this and static are considered types in themselves and can be resolved to the correct type when chaining methods. Unfortunately they cant be used in type declarations. Only in phpdoc or when inferring type from implementation.

[jfcherng]

And thats the key point. You are wrong, I didnt. B is never mentioned as return type. The return type is not even mentioned in the B class but in A. So somehow Intellephense seems to walk the path back from static to through the class hierarchy and finds, that means B at this point. Either way it takes some steps to figure that static means "B" at this line of code.

I always treat /** @return static */ as a compromise because you cannot typehint a method to return a static to the PHP interpreter.

I consider the following two codes are "conceptually equivalent".

class A
{
    /** @return static */
    public function Test() {}
}

class A
{
    public function Test(): static {}
    //                      ^^^^^^ syntax error in real world
}

To be making sense, $X->Test() should returns the type of the caller (i.e., $X's type) just like what return new static(); does. If this statement holds, then intelephense knows (new B)->Test() returns a B without any assumption or further inspection.

[procodix]

@bmewburn

If there is no return type declaration and no phpdoc then the extension has no choice but to load the file and try and infer the return type. This is much more expensive [...]

We are talking about a JavaScript extension, loaded & interpreted in a JavaScript framework, loaded & interpreted in JavaScript engine executed within a browser, written for some totally different purpose.

I mean, really, how bad can the performance impact be to look that up two or three more times? If I had to save some nanoseconds I would look somewhere else in that stack: drawing the red wavy line under the "problem" with JavaScript, HTML & CSS is costs probably more performance :-)

I'd say: Give it a try.

[KapitanOczywisty]

@procodix Your request is based on missunderstending of oop and php, and even if this would took 1ms (which would be way more on real / bigger projects) this is just more complications / potential points of failure to make something questionable work.

Yes this works in php, but only because php in its nature don't do static analisys before run (C# for example will throw error at this https://rextester.com/LRD6710 ). A::Test():self returns A and expecting anything more from that is just wrong. self is not static and if you want return static make it clear in @return.

[procodix]

@KapitanOczywisty I really want to refrain from discussing mastership of oop or php with you.

You're deadlocked in overteaching the differences of Intellephens and PHP - while nobody is comparing them. In reality Intellephense just supplements PHP as an IDE. Thus it should reflect the behaviour and assumptions of the language in order to give sensible hints to the developer.

Now, when any IDE throws an error because it doesnt grasp a return type the same way as the underlying language, this is considered a bug and should be changed because it throws off the developer. It was already described as a false positive. No more explanation needed here.

What I've been trying to promote is workaround, that
1) doesnt force me to produce comments, that - despite their name - are type hints (which might have debatle parsing performance themselves) but encourages to use a real type hint
2) doesnt block easy refactoring when PHP finally comes to support ::static return type. In the vast majority there is no harm done by replacing ::self with ::static - now and later.
3) doesnt require substantial changes in Intellephense. We already established, that Intellephense currently is very well able to figure out the subtle difference between "self" and "static".

[jfcherng]

Now, when any IDE throws an error because it doesnt grasp a return type the same way as the underlying language, this is considered a bug and should be changed because it throws off the developer. It was already described as a false positive. No more explanation needed here.

There are some "bugs" (whatever you like to call it) that never can be "fixed". Such as asking a static analyzer to fully understand what will happen at runtime. Intelephense is not the only static analyzer that throws this error and MOST of famous analyzers throw it as well.

Even if intelephense decides to resolve this in its own way without requiring users to do anything, it doesn't help me as long as I use other formal static analyzer. But I do think there should be a way to ignore this kind of error like other analyzers.

---

As for is it "easy" to implement your workaround and maintain it, it's intelephense author's job to make decisions and judgments. So I have no comment on how easy it is. Your comment makes me feel like you are able to see intelephense's unobscured source codes.

[KapitanOczywisty]

Okej, as compromise maybe extension could silently change self to static when return is only $this (or new static). This wouldn't negatively affect anyone. @procodix would this be enough?

[procodix]

@KapitanOczywisty: sounds good.

One more point. May be a less debatable "bug" this time. The @return static DocBlock fails if it is in an intermediate class, so @jfcherng using DocBlock is not really a good compromise, because it fails in a common scenario:

<?php

class A {
    /*
        @return static
    */
        public static function Instance() {return (new static)->Method();}
    public function Method() {return $this;}
    public function SayA() {echo 'A';}
}
class B extends A {
    /*
        @return static   <- this is ignored
    */
    public static function Instance() {return parent::Instance()->Method();}
    public function SayB() {echo 'B';}
}
class C extends B {
    public function SayC() {echo 'C';}
}
class D extends A {
    public function SayD() {echo 'D';}
}
class E extends D {
    public function SayE() {echo 'E';}
}

A::Instance()->SayA(); // ok
B::Instance()->SayB(); // undefined method
C::Instance()->SayC(); // undefined method
D::Instance()->SayD(); // ok, because there is no factory method redeclared inbetween
E::Instance()->SayE(); // ok, because there is no factory method redeclared inbetween

[jfcherng]

@procodix I cannot reproduce it. Please use a correct format of phpdoc.

/** @return static */

or

/**
 * @return static
 */

---

<?php

class A {
    /** @return static */
    public static function Instance() {return (new static)->Method();}
    public function Method() {return $this;}
    public function SayA() {echo 'A';}
}
class B extends A {
    /** @return static */
    public static function Instance() {return parent::Instance()->Method();}
    public function SayB() {echo 'B';}
}
class C extends B {
    public function SayC() {echo 'C';}
}
class D extends A {
    public function SayD() {echo 'D';}
}
class E extends D {
    public function SayE() {echo 'E';}
}

A::Instance()->SayA(); // ok
B::Instance()->SayB(); // ok
C::Instance()->SayC(); // ok
D::Instance()->SayD(); // ok
E::Instance()->SayE(); // ok

[procodix]

@jfcherng you're right. that was my mistake, sorry.

[bmewburn]

Closing this as designed. To summarise why:

1. self resolves to the containing class and is distinct from static.
2. It is akin to how statically typed language compilers behave. If we consider php as gradually typed then it is reasonable to identify a problem when a declared return type does not declare/define a called method.
3. There are workarounds with phpdoc annotations and phpstorm metadata to overcome limitations in both the php type system and intelephense.
4. Undefined methods can be disabled and there is further work planned to disable these checks on a statement level.

[bmewburn]

https://wiki.php.net/rfc/static_return_type

[procodix]

be progressive:

https://stitcher.io/blog/new-in-php-8 #3

[procodix]

https://github.com/bmewburn/vscode-intelephense/issues/1141#issue-618888977