Support type annotations for class consts (fixes #942). #7123

AndrolGenhald · 2021-12-10T21:12:50Z

No description provided.

AndrolGenhald · 2021-12-10T21:13:54Z

src/Psalm/Issue/UnresolvableConstant.php

+
+class UnresolvableConstant extends CodeIssue
+{
+    public const ERROR_LEVEL = -1;


Should this be set to something else?

I believe -1 means that the error will be emitted no matter what level the user is on. It's usually reserved to obvious errors or issue behind a specific flag.

I'm not sure I see yet in what scenario this issue could be emitted, but you have to decide a level based on severity

-1 denotes insuppressible issue, -2 - issues controlled by a flag (like --find-dead-code).

So -1 should be reserved for certain fatal errors.

It should be emitted when key-of or value-of is used on a constant that can't be resolved, which I think probably should always be emitted, as it's indicative of a typo or other issue with the type annotation, and it's an annotation that's specifically for static analysis. I never use any of the less strict error levels though, so I'm not confident I should be the one deciding.

src/Psalm/Storage/ClassConstantStorage.php

AndrolGenhald · 2021-12-10T21:16:27Z

tests/ConstantTest.php

+                ',
+                'error_message' => 'UnresolvableConstant',
+            ],
+            'SKIPPED-keyofSelfConstDoesntImplyKeyofStaticConst' => [


Would be nice to do, but psalm isn't catching this either: https://psalm.dev/r/4efbbbda75
I thought there was an option to be strict about array keys somewhere, but I'm not seeing it at the moment, and I don't know how I would enable that for the test.

I believe you might need those two:

<xs:attribute name="ensureArrayIntOffsetsExist" type="xs:boolean" default="false" /> <xs:attribute name="ensureArrayStringOffsetsExist" type="xs:boolean" default="false" />

To enable it you would have to extract the case into a full-fledged test method, like this:

psalm/tests/ArrayAccessTest.php

Lines 17 to 36 in 3cce691

public function testEnsureArrayOffsetsExist(): void

{

$this->expectException(CodeException::class);

$this->expectExceptionMessage('PossiblyUndefinedStringArrayOffset');

Config::getInstance()->ensure_array_string_offsets_exist = true;

$this->addFile(

'somefile.php',

'<?php

function takesString(string $s): void {}

/** @param array<string, string> $arr */

function takesArrayIteratorOfString(array $arr): void {

echo $arr["hello"];

}'

);

$this->analyzeFile('somefile.php', new Context());

}

AndrolGenhald · 2021-12-10T21:26:39Z

I just tested #5743 and it seems to work, but if you don't add an argument it reports "Too few arguments for - expecting 1 but saw 0". Seems like the in_array isn't getting into the message somewhere.

Edit: Actually I guess that's sort of expected, since the type should be callable(mixed): bool, but the message should probably be fixed still.

weirdan · 2021-12-10T21:39:27Z

tests/ConstantTest.php

+                    class Foo
+                    {
+                        public const BAR = ["bar"];
+
+                        /**
+                         * @return value-of<static::BAR>
+                         */
+                        public function bar(): string
+                        {
+                            return static::BAR[0];
+                        }
+                    }
+                ',
+                'error_message' => 'UnresolvableConstant',


Should this really be UnresolvableConstant? I think I'd find it confusing.

Should this really be UnresolvableConstant? I think I'd find it confusing.

Yeah, perhaps a bit confusing, but I made a note of it in the UnresolvableConstant documentation. I didn't really want to have another issue for IllegalConstant or something like that. I'm happy to change the name if you can come up with something that intuitively encompasses both usages, or would you like to have a separate issue?

Maybe UnresolvableLiteralConstant?

weirdan · 2021-12-10T21:46:07Z

tests/ConstantTest.php

+                ',
+                'error_message' => 'UnresolvableConstant',
+            ],
+            'SKIPPED-keyofSelfConstDoesntImplyKeyofStaticConst' => [


I believe you might need those two:

<xs:attribute name="ensureArrayIntOffsetsExist" type="xs:boolean" default="false" /> <xs:attribute name="ensureArrayStringOffsetsExist" type="xs:boolean" default="false" />

To enable it you would have to extract the case into a full-fledged test method, like this:

psalm/tests/ArrayAccessTest.php

Lines 17 to 36 in 3cce691

public function testEnsureArrayOffsetsExist(): void

{

$this->expectException(CodeException::class);

$this->expectExceptionMessage('PossiblyUndefinedStringArrayOffset');

Config::getInstance()->ensure_array_string_offsets_exist = true;

$this->addFile(

'somefile.php',

'<?php

function takesString(string $s): void {}

/** @param array<string, string> $arr */

function takesArrayIteratorOfString(array $arr): void {

echo $arr["hello"];

}'

);

$this->analyzeFile('somefile.php', new Context());

}

AndrolGenhald · 2021-12-10T22:04:24Z

So here's something I'm not sure of: should we default to the annotated type or the inferred type?
I mainly see two different uses for type annotation:

Annotating the type that a constant should always have. The constant may be changed in the future, but it will always have this type.
Annotating the type that child classes must be covariant with.

Unfortunately, I could see places where one might want the behavior of 2 without the behavior of 1. Should this be configurable? Should we just go with 1 and say if you annotate it, that's what psalm is going to use?

Contrived illustration of where 2 without 1 may be useful: https://psalm.dev/r/8a9abce968

Edit: I suppose one possible solution is to say if you use @var it's going to use the inferred type, and if you use @const it'll use the docblock type, but that seems a bit obscure. Is it worth creating another annotation for?

psalm-github-bot · 2021-12-10T22:04:27Z

I found these snippets:

https://psalm.dev/r/8a9abce968

<?php

class Foo
{
    /** @var string */
    public const BAR = "bar";
    
    /**
     * @return "bar"
     */
    public function bar(): string
    {
        return self::BAR;
    }
    
    public function childBar(): string
    {
        return static::BAR;
    }
}

Psalm output (using commit 76bb8bc):

INFO: MixedReturnStatement - 18:16 - Could not infer a return type

INFO: MixedInferredReturnType - 16:33 - Could not verify return type 'string' for Foo::childBar

weirdan · 2021-12-10T22:31:16Z

Is there actually a difference between 1 and 2 though?

AndrolGenhald · 2021-12-10T22:33:37Z

Is there actually a difference between 1 and 2 though?

In the example I gave it should work fine with 2, but with 1 it will have an issue for return self::BAR; because self::BAR will be string rather than "bar".

AndrolGenhald · 2021-12-10T23:06:55Z

#5743 is broken again after I fixed a mistake. I meant to use the inferred type except when using late static binding, rather than use the annotated type except for with self::. I think that should be reopened and fixed separately, depending on how we decide to handle inferred vs annotated type.

My preference is to use the inferred type wherever possible and explicitly opt out of that, because it allows a lot more static analysis, but right now there's no way to opt out and say you want the annotated type to be used for a particular constant.

I'm tempted to say that the fix for #5743 should actually be to allow interpreting "in_array" as a callable, but I think that was decided against previously, probably for a good reason. ~~Perhaps we should just leave it closed and say that first class callables in PHP 8.1 are the correct solution?~~ Scratch that, you can't use those in a constant expression.

AndrolGenhald · 2021-12-10T23:08:24Z

src/Psalm/Internal/Analyzer/ClassLikeAnalyzer.php

+     * @param array|scalar|null $value
+     *
+     */
+    public static function getLiteralTypeFromValue($value): Type\Union


Not sure if this function already exists somewhere and I missed it. Should this go somewhere else? Maybe as Type::getLiteral?

weirdan · 2021-12-10T23:28:34Z

I'd say we should follow the principle of least surprise and just use the declared type, and also flag cases where the constant value contradicts the declared type. As far as inheritance is concerned, constant types should be covariant.

Do you see any problems with this approach?

AndrolGenhald · 2021-12-10T23:45:20Z

I'd say we should follow the principle of least surprise and just use the declared type

Well, key-of and value-of currently still use the inferred type, and I think it'd be more surprising if they didn't, so at the very least that's going to feel a bit inconsistent.

It also causes annoyances like this: https://psalm.dev/r/cde953e232

and also flag cases where the constant value contradicts the declared type

I meant to do that and completely forgot about it.

psalm-github-bot · 2021-12-10T23:45:23Z

I found these snippets:

https://psalm.dev/r/cde953e232

<?php

class Foo {
    /**
     * I don't care about child classes, this annotation just exists
     * to make sure all elements match the type.
     *
     * @var array<string, array{foo: string, bar: int}>
     */
    public const CONFIG = [
        "a" => ["foo" => "foo", "bar" => 1],
    ];
    
    /**
     * @param key-of<self::CONFIG> $key
     *
     * @return array{foo: string, bar: int}
     */
    public function getConfig(string $key): array
    {
        // Should cause PossiblyUndefinedStringArrayOffset because the
        // annotated type is used, even though I don't want it to be
        return self::CONFIG[$key];
    }
}

Psalm output (using commit 76bb8bc):

No issues!

tests/ConstantTest.php

src/Psalm/Storage/ClassConstantStorage.php

orklah · 2022-01-20T20:08:13Z

@AndrolGenhald what is the status of this? Are you waiting for another PR, for a review or do you still need to work on it?

AndrolGenhald · 2022-01-20T22:56:10Z

@orklah Ready for review, I just rebased on master and got everything up-to-date.

#7396 might be able to make key/value-of usable for static const arrays.

src/Psalm/Storage/ClassConstantStorage.php

AndrolGenhald · 2022-01-20T23:11:03Z

I think the only thing here that might be controversial is what I mentioned here, I have it defaulting to using the inferred type whenever possible, so the @var annotation is really just used to make sure the literal type matches (useful for complicated nested arrays), and once #7154 is merged it's used to make sure children have covariant constant types.

I can see where some people might want to override the constant type, but that would prevent analysis like this where you want to constrain the types a child can override the constant with, but you still want to use the inferred type for other analysis (returning a literal in that example).

I think using the inferred type wherever possible is more useful, and it more closely matches the way psalm handles constants now when it ignores the @var annotation. If preventing usage of the inferred type is desired I think we should open a separate issue for that and maybe add some sort of annotation for it.

psalm-github-bot · 2022-01-20T23:11:05Z

I found these snippets:

https://psalm.dev/r/8a9abce968

<?php

class Foo
{
    /** @var string */
    public const BAR = "bar";
    
    /**
     * @return "bar"
     */
    public function bar(): string
    {
        return self::BAR;
    }
    
    public function childBar(): string
    {
        return static::BAR;
    }
}

Psalm output (using commit 0a81f8c):

INFO: MixedReturnStatement - 18:16 - Could not infer a return type

INFO: MixedInferredReturnType - 16:33 - Could not verify return type 'string' for Foo::childBar

orklah · 2022-01-22T20:32:03Z

I have it defaulting to using the inferred type whenever possible

So, just to be sure, I see 3 possible types for a class const.

the type of the parent const
the type of the content of the const
the type of the @var

For me, const should be invariant, like properties, so the type of the parent const must be the same as the child.
However, this can be hard because const content will be literals so unless you inherit with the same exact literal (which makes no sense), Psalm should scream.

I think this is where @var will be useful to define a standard type among all childs/parent (for example, if you have a parent that defined "hello" and a child "hello2", both should be @var'ed to 'string'

So Psalm just need to make sure that the type from the @var is consistent with the content of the const. And then apply invariant checks.

Do I make sense?

AndrolGenhald · 2022-01-22T20:37:05Z

@orklah We discussed this before at #5589, do you want to move the discussion over there? I think it makes more sense for const types to be covariant rather than invariant, see my last comment there with the examples.

orklah · 2022-01-22T20:50:05Z

Ok, now that we settled we're indeed covariant, it makes things easier I think.

As long as the phpdoc type is consistent with the content of the constant, the phpdoc is allowed. Then the phpdoc is always the default because it can be more precise

orklah · 2022-01-22T21:14:09Z

Ok, fine by me then :)

Thanks for taking the time to explain :p

orklah · 2022-01-22T21:17:57Z

src/Psalm/Internal/Analyzer/ClassLikeAnalyzer.php

+     * @throws InvalidArgumentException
+     *
+     */
+    public static function getLiteralTypeFromValue($value, bool $sealed_array = true): Type\Union


Maybe getLiteralTypeFromScalarValue could be reused or refactored or something?

I overlooked that one, I'll take a look at it in a bit. One thing I'm not too happy about is that allowing array here could allow a non-scalar to be passed, but I don't think we have a way to type "possibly deeply nested array with only scalars allowed".

Done. I'm half tempted to try adding something like TNestedArray where nested-array<scalar> would be like array<scalar|array<scalar|array...>>, but I'm not sure how useful it would be in practice.

It would be a hell to add I think. It means you'll have to implement the logic to compare any array to the nested array.
That means:

can any array be equal to the nested one

can any array be contained in the nested and the other way around

nested assertions probably

etc...

AndrolGenhald · 2022-01-22T21:24:08Z

Thanks for taking the time to explain :p

No problem, convincing someone is a really good sanity check.

…t issues.

orklah · 2022-01-25T19:54:46Z

Thanks!

orklah · 2022-01-25T20:03:07Z

That's weird, PHPUnit test and Psalm itself started to output the same kind of error after I merged. They weren't on CI.

Could you take a look at it to see if they can be fixed?

AndrolGenhald · 2022-01-25T20:10:12Z

@orklah I assume you're referring to src/Psalm/DocComment.php:21:18: InvalidConstantAssignmentValue: Psalm\DocComment::PSALM_ANNOTATIONS with declared type non-empty-array<int<1, max>, string> cannot be assigned type array{'return', 'param',...? My bet is something changed between the last CI run and now that didn't cause a conflict but messed up the type comparison. There's no reason I can see that it should think the type is non-empty-array<int<1, max>, string>.

orklah · 2022-01-25T20:14:28Z

Mmh, it may actually be one of my PR when I removed positive-int. I'll try to take a look at it

AndrolGenhald · 2022-01-25T20:25:39Z

I think I'm closing in on it, I'm trying to figure out why it's getting the wrong type set in ClassLikeNodeScanner.
Edit: Looks like SimpleTypeInferrer must be inferring it wrong.
Edit 2: Yeah, it must be IntRange stuff, at SimpleTypeInferer.php:479 $array_creation_info->item_key_atomic_types is a list of literal ints that includes 0. TypeCombiner returns int<1, max>.

AndrolGenhald · 2022-01-25T20:36:05Z

@orklah TypeCombiner.php:1195:

value_types['int'] = $all_nonnegative
    ? TIntRange(1, null)
    : TNonspecificLiteralInt();`

Should be 0 there, 0 is non-negative. It's not immediately clear to me how it worked before, the literal 0 must have gotten added back somewhere, but that's not happening now.

orklah · 2022-01-25T20:38:12Z

Yeah, I just saw that too. It was new TPositiveInt before my change, it just made it appear. I'll fix that

AndrolGenhald · 2022-01-25T20:41:03Z

Ah, I see, there's a $had_zero there to add the literal 0 back into it to give 0|positive-int, but that got removed as well, so instead of 0|int<1,max> it just resulted in int<1,max>.

orklah · 2022-01-25T21:08:07Z

Thanks btw! You're pretty efficient at finding bugs in Psalm!

AndrolGenhald · 2022-01-25T21:10:31Z

No problem, I enjoy debugging 🙂

AndrolGenhald · 2022-01-25T22:41:28Z

@orklah is this a bug on psalm.dev? When I run it in a test locally the exception is correctly caught and an issue is raised, rather than causing psalm to crash.
Edit: nevermind, I have work to do. I never noticed it because the tests always worked because they stop at the first issue raised.

psalm-github-bot · 2022-01-25T22:41:30Z

I found these snippets:

https://psalm.dev/r/3531bf3469

<?php

class Foo
{
    /** @var non-empty-list<int> */
    public const BAZ = [1];

    /** @return value-of<static::BAZ> */
    public static function foobar(int $int): int
    {
        return static::BAZ[0];
    }
}

Psalm encountered an internal error:

/vendor/vimeo/psalm/src/Psalm/Internal/Type/TypeExpander.php:

AndrolGenhald commented Dec 10, 2021

View reviewed changes

src/Psalm/Storage/ClassConstantStorage.php Show resolved Hide resolved

AndrolGenhald commented Dec 10, 2021

View reviewed changes

weirdan reviewed Dec 10, 2021

View reviewed changes

AndrolGenhald commented Dec 10, 2021

View reviewed changes

AndrolGenhald commented Dec 11, 2021

View reviewed changes

tests/ConstantTest.php Outdated Show resolved Hide resolved

AndrolGenhald commented Dec 11, 2021

View reviewed changes

src/Psalm/Storage/ClassConstantStorage.php Show resolved Hide resolved

AndrolGenhald force-pushed the feature/942-type-annotate-class-constants branch 2 times, most recently from e2e88ec to 0f899ac Compare December 13, 2021 19:42

AndrolGenhald mentioned this pull request Dec 13, 2021

Improve class constant static analysis #7154

Merged

AndrolGenhald force-pushed the feature/942-type-annotate-class-constants branch from 1d3dfe9 to 21edfcc Compare January 20, 2022 22:55

AndrolGenhald commented Jan 20, 2022

View reviewed changes

src/Psalm/Storage/ClassConstantStorage.php Show resolved Hide resolved

orklah added the PR: Need review label Jan 20, 2022

orklah added the release:feature label Jan 22, 2022

orklah reviewed Jan 22, 2022

View reviewed changes

orklah added release:feature and removed release:feature labels Jan 22, 2022

AndrolGenhald added 10 commits January 22, 2022 17:06

Support type annotations for class consts (fixes vimeo#942).

c18a155

Fix trailing comma.

df31465

Use inferred type instead of annotated type where possible.

c877ce0

Add type checking for class const assignments, fix several other cons…

558208e

…t issues.

Infer type from known ternary condition.

0fb0632

Suppress PossiblyUnusedProperty, will be used in the future.

c61bde8

Default to using inferred type unless in late static binding context.

1686f28

Fix CS issue due to rebase.

1f1f1c5

AndrolGenhald force-pushed the feature/942-type-annotate-class-constants branch from 21edfcc to 1f1f1c5 Compare January 22, 2022 23:12

orklah merged commit 7c8441b into vimeo:master Jan 25, 2022

AndrolGenhald mentioned this pull request Jan 25, 2022

Fix crash due to unresolvable constant. #7490

Merged

	public function testEnsureArrayOffsetsExist(): void
	{
	$this->expectException(CodeException::class);
	$this->expectExceptionMessage('PossiblyUndefinedStringArrayOffset');

	Config::getInstance()->ensure_array_string_offsets_exist = true;

	$this->addFile(
	'somefile.php',
	'<?php
	function takesString(string $s): void {}

	/** @param array<string, string> $arr */
	function takesArrayIteratorOfString(array $arr): void {
	echo $arr["hello"];
	}'
	);

	$this->analyzeFile('somefile.php', new Context());
	}

Support type annotations for class consts (fixes #942). #7123

Support type annotations for class consts (fixes #942). #7123

Conversation

AndrolGenhald commented Dec 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrolGenhald commented Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrolGenhald commented Dec 10, 2021 • edited Loading

psalm-github-bot bot commented Dec 10, 2021

weirdan commented Dec 10, 2021

AndrolGenhald commented Dec 10, 2021

AndrolGenhald commented Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

weirdan commented Dec 10, 2021

AndrolGenhald commented Dec 10, 2021 • edited Loading

psalm-github-bot bot commented Dec 10, 2021 • edited Loading

orklah commented Jan 20, 2022

AndrolGenhald commented Jan 20, 2022

AndrolGenhald commented Jan 20, 2022

psalm-github-bot bot commented Jan 20, 2022

orklah commented Jan 22, 2022

AndrolGenhald commented Jan 22, 2022

orklah commented Jan 22, 2022

orklah commented Jan 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrolGenhald commented Jan 22, 2022

orklah commented Jan 25, 2022

orklah commented Jan 25, 2022

AndrolGenhald commented Jan 25, 2022

orklah commented Jan 25, 2022

AndrolGenhald commented Jan 25, 2022 • edited Loading

AndrolGenhald commented Jan 25, 2022 • edited Loading

orklah commented Jan 25, 2022

AndrolGenhald commented Jan 25, 2022

orklah commented Jan 25, 2022

AndrolGenhald commented Jan 25, 2022

AndrolGenhald commented Jan 25, 2022 • edited Loading

psalm-github-bot bot commented Jan 25, 2022

AndrolGenhald commented Dec 10, 2021 •

edited

Loading

AndrolGenhald commented Dec 10, 2021 •

edited

Loading

AndrolGenhald commented Dec 10, 2021 •

edited

Loading

AndrolGenhald commented Dec 10, 2021 •

edited

Loading

psalm-github-bot bot commented Dec 10, 2021 •

edited

Loading

AndrolGenhald commented Jan 25, 2022 •

edited

Loading

AndrolGenhald commented Jan 25, 2022 •

edited

Loading

AndrolGenhald commented Jan 25, 2022 •

edited

Loading