Psalm 5 breaking changes

[muglug]

• Redundant casts should become their own issue (currently RedundantCondition)

[kkmuffme]

Perhaps extend this to redundant intval, floatval,... calls too?

[muglug]

Closing, going to make this a minor change instead

[weirdan]

• functions in Psalm namespace (https://github.com/vimeo/psalm/blob/3ad3375777b431a559b624d359c9af667de319f1/src/command_functions.php) are going to be removed, deprecated in Cli command classes #5858
• allowCoercionFromStringToClassConst config attribute is going to be removed, deprecated in Formally deprecate allowCoercionFromStringToClassConst #5897
• cli scripts in src folder are going to be removed, deprecated in Cli command classes #5858

[orklah]

• Legacy plugins hooks should be removed too (or is it too soon?)

[weirdan]

@orklah They need to be deprecated first, so I went ahead and marked them as such in #5898. We can always postpone the removal and do it in v6, but the earlier we announce deprecation the better.

[muglug]

= make array shapes explicit — array{foo: Bar} means an array with exactly one element. array{foo: Bar}&array<string, object> can be used to denote an array with optional extra elements

[weirdan]

make array shapes explicit

Ideally this would first come with a config switch and v5 would just flip the default, unless you think that's too much work.

[orklah]

I'm not sure introducing a config long term on something so fundamental would be beneficial for the project. It will make every future bug report hard to reason about depending on the value of the config.

[weirdan]

The following methods were previously deprecated, linked PR marks them for removal in Psalm 5

• DocComment::parse()
• TypeAnalyzer::isContainedBy(), TypeAnalyzer::isAtomicContainedBy()
• CodeIssue::getLocation(), CodeIssue::getFilename(), CodeIssue::getMessage()

[weirdan]

Things to be removed can be located by grepping the src folder for the Psalm 5 substring.

[VincentLanglet]

I'm not sure introducing a config long term on something so fundamental would be beneficial for the project. It will make every future bug report hard to reason about depending on the value of the config.

The format array{foo: string}&array is currently not supported by phpstan. So this Bc break would require to have both a phpstan-param and psalm-param annotation for all the array phpdoc.
Also, for my personnal case, most of the array shape I had to type was non strict, so this BC break would require to change all the existing phpdoc I have.

Having a config option would be nice for the migration (and also until phpstan support the same format).

[ondrejmirtes]

make array shapes explicit

I don't think this is a good idea. Same as you wouldn't want to make arrays behave differently in the PHP language, you don't want to change interpretation of such a basic building block when there's already a lot of code out there that uses it.

I think the new behaviour needs to be connected with a different type, like strict-array{foo: Bar} or something like that. It's not beautiful but that's the price of mistakes of the past :)

[ondrejmirtes]

Also, I don't love the array{foo: Bar}&array<string, object> syntax - without any special handling, this gets normalized to array{foo: Bar} because out of pair of two types, the subtype always wins the intersection.

Maybe something more similar to TypeScript's notation would be better, maybe: array{foo: Foo, [k: string]: Bar}. More about it here: https://www.typescriptlang.org/docs/handbook/2/objects.html#index-signatures

[VincentLanglet]

make array shapes explicit

I don't think this is a good idea. Same as you wouldn't want to make arrays behave differently in the PHP language, you don't want to change interpretation of such a basic building block when there's already a lot of code out there that uses it.

I think the new behaviour needs to be connected with a different type, like strict-array{foo: Bar} or something like that. It's not beautiful but that's the price of mistakes of the past :)

Also, in the same way that a method which accept a class Foo accept all the class extending Foo, I find more natural that array{foo: string} accepts array with extra values.

I would prefer strict-array

[ondrejmirtes]

I don't like hijacking other issues but I'm gonna continue here because I'm not sure that @muglug wants to change his mind (I hope so!).

Maybe a better syntax would be:

• array{foo: Bar} - same meaning as today, basically a shortcut for array{foo: Bar, [k: mixed]: mixed}
• array{foo: Bar, []} - no additional keys allowed, same as the proposed strict-array
• array{foo: Bar, [k: string]: object} - additional keys of specified key/value types allowed

Also:

1. I'm not sure if the k identifier has any meaning in TypeScript
2. I'm not sure if TypeScript allows [k: Foo, l: Bar]: Baz and what it would mean

[ondrejmirtes]

My fear about the proposed array shape meaning change is not only backward compatibility with all the existing analysed code out there (it would be pain for people to upgrade to the new meaning), but also compatibility and synchronicity with PHPStan. We'd have to release new versions simultaneously and that only solves part of the problem.

A configuration option to toggle between the meanings isn't great either - you need to be able interpret old code vs. new code at the same time (1st party code + vendor).

[orklah]

I must admit Ondrej makes a lot of good points. Especially on the syntax topic, the intersection doesn't appear fitted for the job. Once this is laid out, the BC issue and compatibility obstacle with PHPStan feels unnecessary

I was quite fond of array{} being strict by default (because I feel that's the most intuitive for clean code, someone who makes the effort of describing the structure seems likely to describe the whole array most of the times), so maybe lax-array instead of strict-array would be better.

The hybrid syntax suggested by Ondrej could be interesting too, (but do we really want to support possible integers keys in an object-like structure? I feel this would open up to yet more PHP weirdness with numeric array-keys that turns to integer keys)

[orklah]

So you think the lost of the (currently wrong) inference will cause more psalm-error rather than making the array strict ?
I don't.

I do. This inferrence is there because most of the times it's right and it's a really useful assumption to make. (because again, for me most case already describe strict arrays and Psalm acts accordingly)

Tomorrow, with an array that is lax by default, this inferrence will never be made again. This will introduce new errors in builds that were prevented by this inference.

I agree the BC break is annoying but I think it'd be better for the long term

[VincentLanglet]

(because again, for me most case already describe strict arrays and Psalm acts accordingly)

This is where we disagree. I believe the opposite. Most case correctly describe lax array to me.

Tomorrow, with an array that is lax by default, this inferrence will never be made again. This will introduce new errors in builds that were prevented by this inference.

Just update array to strict-array and it will make the inferrence again. I don't see the difference with the fact you're asking people to change array to lax-array.

I agree the BC break is annoying but I think it'd be better for the long term

I don't see any long term for BC-break. When the BC-break occur, dev will have to update all their code base and then it won't have any impact in the futur. Either people will have to change all the strict array to strict-array, either they will have to change all the lax array to lax-array.

The decision should be based on

• What would introduce the less BC-break (debatable)
• What is the more natural to developer (debatable)
• What is the easiest compatible with phpstan (strict-array)

Just asked to my developer team and they find also more natural that when we add a term, it should be stricter.
Like class-string is stricter than string or non-empty-array is stricter to array.
I find better for the long term to keep this consistency.

[M1ke]

I agree on these concepts about strictness. At the very least if Psalm made the BC break it should ship with a rule that replaces all instances of array{} across a codebase with the old inference. But I feel that at that point you're acknowledging that basically new syntax is required.

I also feel that array{something: Bar}&array<string, mixed> with the new strict concept is not the same. At present if I declare array{something: Bar} and then I try to use $arr['whatever'] Psalm will tell me the key is undefined, but with the &array<string, mixed> added on, it downgrades it to a warning.

See https://psalm.dev/r/b40bdfa369 vs https://psalm.dev/r/c8c8bc59ff

Psalm is pretty fine with that code but it's a gigantic bug

I'd suggest that the bug here is using count on something which is declared as object-like OR that count is naive to the fact that the current semantics for array{} do not require a fixed number. So in this case I'd suggest that the cleanest fix is for Psalm to decide that count(array{key: type]) is int rather than 1.

[klimick]

Any possibilities to see this in Psalm 5?
#4564

[orklah]

• Remove allowPhpStormGenerics

[bdsl]

On the strict-array stuff, maybe it would help if someone could show an example use case for requiring an array to not contain certain elements. To me it seems like array{foo: int, bar: string} is an LSP-compliant subtype of array{foo: int}.

[orklah]

@bdsl well for example, the inference is not optimal on $_b here: https://psalm.dev/r/7c42d25a9f
Psalm can only infer int<0, max> because the array is not sealed
However, on $_d, Psalm can know it's int<1, 2> because it knows no other element can be in there.
I could give you a similar exemple of a loop over the array where keys would be known with certainty to be 'a'|'b' but we can't do that with an unsealed array

It can be useful to be able to infer this for some applications and there's no dedicated syntax for it.

I kinda changed my mind since the discussion above and I no longer think using intersection between two arrays makes sense (even more so now that we have intersection in the language). I still believe that array shapes described right now should be the strict form, but I won't change it unilaterally.

As a user and occasional contributor, I had the luxury of having strong opinions that I can't hold on my own now that Matt is gone :D

I'll let PHPStan take the lead on whatever syntax is the best going forward and we'll follow

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/7c42d25a9f

<?php

/** @param array{a?: string, b?: string} $a */
function a(array $a): void{
    $_b = count($a);
    /** @psalm-trace $_b */;
    
    $_c = ['a' => 'a'];
    if(rand(0,1)){
        $_c['b'] = 'b';
    }
    $_d = count($_c);
    /** @psalm-trace $_d */;
    
}

Psalm output (using commit e334923):

INFO: Trace - 6:28 - $_b: int<0, max>

INFO: Trace - 13:28 - $_d: int<1, 2>

[bdsl]

Right, I see what you mean - if you consider 'has a count of 1' to be part of the behaviour specification of array{foo: int} then of course the larger array is not a behavioural subtype, I see how similar applies to the loop behaviour. They still feel like edge cases to me that I wouldn't normally want to be part of the specification.

[orklah]

@bdsl that's why we need to introduce a new syntax. It may be strict-array{} or lax-array{} or something else it doesn't matter. The fact is, when you describe an array shape in phpdoc currently, it's not the same thing internally than an array shape that you could have created with code with knowns literals

[VincentLanglet]

The best move I see is

• Keeping array untouched to avoid the BC-break
• Introducing something like strict-array in both Psalm and PHPStan
• Waiting for PHPStan to make a move about intersection and following the syntax.

[weirdan]

<exitFunctions> config element is going to be removed: #6794

[weirdan]

<allowPhpStormGenerics> config element is going to be removed: #6694

[weirdan]

ForbiddenEcho issue type and forbidEcho config attribute are going to be removed: #6892

[zmitic]

About the strict-array; wouldn't the word shape fit better like how list is used? I.e. it is not indexed-array but a completely new word.

Reason:

PHP itself might support shapes but with dash in the name, it will not not happen. But I think something like this is possible:

function test(): shape{a: string, b?: ?int} 

It would also be close to identical to shapes in Hacklang, look for HH_FIXME[4057] Typechecker error: we have an extra field

[AndrolGenhald]

About the strict-array; wouldn't the word shape fit better like how list is used? I.e. it is not indexed-array but a completely new word.

The downside to that is that it's not a reserved word for PHP, and it's a common enough word that there are probably projects that have a Shape class.

I'm still planning to move forward with array{a: string, b?: ?int, [array-key]: never} like I suggested here, whenever I can actually find the time to implement it.

[zmitic]

common enough word that there are probably projects that have a Shape class

Agreed but given that Shape is a class, capital letter and most likely namespaced; would that be a big BC?

Whatever the decision is, I would be happy anyway.

[AndrolGenhald]

Agreed but given that Shape is a class, capital letter and most likely namespaced; would that be a big BC?

1. You can import it
2. PHP rarely concerns itself with proper casing, and even if we warn about proper casing I'd rather not break this

I suppose if we only treat it as a shape if it's followed by a { it might work, but then the error message for a parsing error due to a typo would probably be a bit less helpful (eg if someone did shape<a: int> accidentally). I think it's generally just a better idea to use either PHP reserved words or strings that cannot be identifiers (ie because they have a dash).

I'm not necessarily opposed to strict-array, it would probably be easier to add than index-signature-like arrays even though it would be less useful (in the end we might end up with both). I think the big thing right now is figuring out something that other tools would support as well.

[weirdan]

The release is imminent, so I'm closing this issue.