Allow type inference on dynamic ClassConstFetch

[orklah]

fix #5096 (almost)

I had to make the class in the example final to keep consistency with the code above. The whole block of code is copy/pasted from above with a few changes that makes it hard to refactor, even more given the code must be placed after analyzing the expression.

[orklah]

Note: I tried to refactor. I was pretty happy with the result but there was still a ComplexMethod issue... I don't get what it checks.
The CI was failing anyway, the refactor was broken.

[weirdan]

there was still a ComplexMethod issue... I don't get what it checks.

Intuitively, it's a number of paths through the method, given that extracting a method with a couple of conditionals from the top of the original method makes the most impact on the metrics it reports.

[weirdan]

(almost)

Is there's something missing?

[orklah]

Yeah, I explained below that the given example was still giving a mixed. This is due to the class in the example not being final.

However, I just checked again and the final thing was only applied for static:: so I don't think we should apply it when we have the type. I got rid of it so the original example should be completely solved

[weirdan]

Hm, I don't think we should allow constant type inference on non-final classes when accessing them dynamically. Consider this code:

<?php

class A
{
    public const C = 1;
}

class B extends A
{
    public const C = "one";
}

function f(A $a): int
{
    return $a::C;
}

f(new B);

It passes with flying colors on your branch, but results in runtime errors: https://3v4l.org/F6nbs

Master produces more sensible mixed result: https://psalm.dev/r/97a2a87c7c

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/97a2a87c7c

<?php

class A
{
    public const C = 1;
}

class B extends A
{
    public const C = "one";
}

function f(A $a): int
{
    return $a::C;
}

f(new B);

Psalm output (using commit 9e1f7ad):

INFO: MixedReturnStatement - 15:12 - Could not infer a return type

INFO: MixedInferredReturnType - 13:19 - Could not verify return type 'int' for f

[weirdan]

Unless constant is inherited from an interface, as those cannot be overridden.

[orklah]

I have to think more about that. The thing that bothers me is that in the two tests I added, the type of the class is known with certainty, it can't be a child of the object so I feel like Psalm should know that and not require the class to be final. I'm really not sure Psalm keeps tabs on that sort of thing though and I wonder how hard it could be to add that.

[orklah]

I tried introducing a new flag to make Psalm aware of case where the type of the class is known not to be a child. It seems to be working well. I didn't try to cover every case where the type is known, I think it's something we can refine on the go.

I added the flag to TNamedObject and TLiteralClassString but I think it may be useful in TClassString too (in some edge case where we do ::class on something we don't know the type of). For now I declared two separate properties, but it may have been better with a Trait (I don't really like traits...)

[weirdan]

I think I have one last nitpick - the default value for $definite_class. There's quite a few of direct instantiations of both types, and a couple of calls to getLiteralClassString() that omit the argument. Historically we mostly treated TNamedObject as something that allows descendants - so shouldn't we default that flag to false everywhere, for both types, and only enable it where we know we want it?

[orklah]

Oh, my bad, the wording flipped the whole meaning and I didn't update values. Before, it was true when it could be a child, now it should be true when it can't

[weirdan]

This is safe (interface constants cannot be overridden) but is currently not inferred:

interface A
{
    public const C = 1;
}

function f(A $a): int
{
    return $a::C;
}

It can be fixed separately though.

[orklah]

Yeah, I saw the issue you made. I think this PR is big enough, let's merge that and we'll check that later