Array-like-object comparison message improvement

[M1ke]

I am opening this as a draft because I am unsure how we'd desire the implementation to work, but I feel it's helpful and it turned out not too hard to implement a basic case.

When dealing with a large object-like array it can be difficult to work out which keys are missing, e.g.

Argument 1 of myFunction() expects array{chicken: bool, fish: bool, pasta: bool, bread: bool, bacon: bool, tea: bool, coffee: bool, beer: bool, onions: bool}, array{chicken: bool, fish: bool, pasta: bool, breead: bool, bacon: bool, tea: bool, coffee: bool, beer: bool, onions: bool} provided (see https://psalm.dev/193)

In this case the eagle-eyed may spot that the second argument has misspelled bread as breead.

The PR as currently implemented would augment this report as so:

Argument 1 of myFunction() expects array{chicken: bool, fish: bool, pasta: bool, bread: bool, bacon: bool, tea: bool, coffee: bool, beer: bool, onions: bool}, array{chicken: bool, fish: bool, pasta: bool, breead: bool, bacon: bool, tea: bool, coffee: bool, beer: bool, onions: bool} provided. The differences are in the following keys: bread, breead (see https://psalm.dev/193)

This allows a simple check on the result that is much easier to track.

Currently it is implemented for ArgumentAnalyzer with PossiblyInvalidArgument and InvalidArgument. It will also likely benefit being added to ArgumentTypeCoercion and possibly others - we can discuss that if the basic premise seems worthwhile. I thought I'd open now to allow discussion and to see if this affects any parts of the CI build.

Will fix #7050

[AndrolGenhald]

I like the idea! I'm thinking maybe we should declare your new method as Union::getExtendedComparisonDescription(Union $other_type): string or something of that sort? That way it would be usable everywhere and it could be used for other things in the future (TObjectWithProperties certainly, and perhaps others).

I think you'll need to add a check that the Union is a single Atomic, otherwise you might be giving a bad description for passing array{fooo: int}|int to a function wanting array{foo: int}. The case for comparing array{fooo: int}|int to array{foo: int}|int is a bit more complicated, so that can probably be left unfinished for now.

[M1ke]

That makes sense, I can carry out both of those changes

[M1ke]

Worth noting, I've been using my change during the last hour of my work and it's already made a few Psalm issues for array-like objects easier to debug. Definitely going to be good for workflows that involve decorating array-like objects in legacy projects.

[orklah]

It's definitely a good idea. It definitely came up a few time in issues, for example among the last responses to #3379

[orklah]

yeah, something like that:

if ($param_type->isSingle()) {
    $first_param_type = $param_type->getSingleAtomic();
}

Then you don't need the isset on ['array'] and you can go on with the instanceof.

You may not want to try handling the Union when there is anything else in it (except maybe null), because you risk having weird results like

... expects array{...very long list of types...}|string , possibly different type array{...exact same list...}|int provided. The differences are in the following keys: (nothing)

[AndrolGenhald]

You may not want to try handling the Union when there is anything else in it

@orklah That's what I said 😛

Actually, I think if the containing Union has other stuff it's probably fine, but this message should probably only be generated if the compared union is a single atomic.

[orklah]

yeah sorry, I read your response after my review^^

[orklah]

Sometimes, the difference comes with the fact that some keys are always defined but some are possibly undefined.

This is represented with a ? after the key, it could be interesting to allow this feature to display that. It adds complexity, but it may be worth it.

For example:
array{a: string, b?: int} vs array{a: string, b: int} would display those keys as different b, b?

[AndrolGenhald]

Oh, that's a good point as well!
I think the message needs to be careful to avoid implying that the keys mentioned are the only ones that are potentially wrong. Without testing, I think this will generate a confusing message for array{a: string} contained by array{a: int, b: string}. I would say maybe the message should be more specific and say "these keys are missing", and just ignore sealed arrays for now (ie extra keys on the child type are fine).

ie, for array{a: string, d: int} contained by array{a: int, b: string, c?: float} it should say "The following keys are missing: b", but for array{a?: string} contained by array{a: string} it needs to have a separate message (or no message and leave that for a future improvement).

[M1ke]

Thanks both, I've finished work for the day but I should be able to sort these changes tomorrow and likely test it a bit more with my actual development work. Will message when ready for another look.

…

On Thu, 20 Jan 2022, 6:58 pm AndrolGenhald, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php <#7445 (comment)>: > @@ -1628,4 +1635,34 @@ private static function processTaintedness( return $input_type; } + + private static function extendKeyedArrayComparison(Union $param_type, Union $input_type): string + { + // Default value is an empty string + $extension = ''; + + // Not sure if there's a better or more robust way to do this + $param_types = $param_type->getAtomicTypes(); You may not want to try handling the Union when there is anything else in it @orklah <https://github.com/orklah> That's what I said 😛 Actually, I think if the containing Union has other stuff it's probably fine, but this message should probably only be generated if the compared union is a single atomic. — Reply to this email directly, view it on GitHub <#7445 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJLLCYKEAOD54M2Q5NPXPDUXBLNNANCNFSM5MNH45IA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[M1ke]

I've just pushed to implement:

• Moved the method as a public on Union
• Add the same logic when discovering ArgumentTypeCoercion issues
• Handle undefined keys
• Improve logic/fix Psalm errors with regards to pulling the array key vs using isSingle()

I think the message logic still needs some work and I'd like to go further to discover mismatches in sub-arrays (probably just one level for now) but that second part may be best in an expansion PR as there's likely a lot more possible conditions as it fans out.

I see the new push has a few failed items so I'll watch and try and resolve those once it all completes.

[M1ke]

@orklah one challenge I've had here is testing with a real codebase. What I am doing is copying my changes onto the Psalm version in the vendor directory of a real project, then finding the right error types to run in that codebase. It's handy to see it for real but makes simulating different conditions awkward - and I can't step debug it.

Is there a mechanism in the Psalm development repo to quickly run a detection of one of these error types?

[M1ke]

Am adding the same for more/less specific return type though I've not had an obvious test case so can't be sure I've got the comparison the right way round.

[M1ke]

Noticed that the system doesn't catch if the types don't match, just if the keys don't. Will rework to ensure it catches that.

[AndrolGenhald]

@M1ke You can add a test somewhere (I'm not seeing an obviously applicable place, so maybe a new file) and debug it like php -d xdebug.mode=debug -dxdebug.start_with_request=yes vendor/bin/phpunit tests/ArgTest.php --filter '@argumentUnpackingLiteral'

For this particular feature it should probably be matching the error message rather than the issue type, for instance, add the following test (might need tweaked):

<?php
/** @psalm-param array{a: string, b: int, c?: float} $_arr */
function foo(array $_arr): void {}
foo(["a" => 1, "d" => 1]);

to providerInvalidCodeParse with 'error_message' => "The following keys are missing: b."
This will ensure it's complaining about "b" being missing, but ignores "c" and "d".

[orklah]

@M1ke to test on real project, you'll see that you have a CI build named 'phar-build'. You can go on it, look for an 'Artifact' link and download a phar you can use to run Psalm with

[M1ke]

Just to add a note, I am still going to put in more work on this, just need to find some time. It's definitely been helpful just the small change I've done so far, so need to expand to more use cases.

[torian257x]

@M1ke my homie lets gooo. I need this... doesnt need to be perfect imho. Something is still better than what we get currently aka "lmao figure out yourself what is wrong"

[andyg0808]

Cross-post from #3379 (comment):

I built a little tool to parse and display Psalm array error messages in what's hopefully a helpful way:
https://ifixit.github.io/psalm-error-parser/

It's hosted on GitHub pages and written entirely in JS.

Here's an example: copy-pasting the first error from https://psalm.dev/r/876f473d2c results in this view:

instead of this:

INFO: LessSpecificReturnStatement - 7:12 - The type 'array{config: array{bread: 42, chalk: 'some'}, greeting: 'hello'}' is more general than the declared return type 'array{config: array{bread: int, cheese: int}, greeting: string}' for takesAnInt

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/876f473d2c

<?php

/**
 * @return array{config: array{bread: int, cheese: int}, greeting: string}
 */
function takesAnInt(string $i) {
    return ["config" => ["bread" => 42, "chalk" => "some"], "greeting" => "hello"];
}

$data = ["some text", 5];
takesAnInt($data[0]);

$condition = rand(0, 5);
if ($condition) {
} elseif ($condition) {}

Psalm output (using commit a75d26a):

INFO: LessSpecificReturnStatement - 7:12 - The type 'array{config: array{bread: 42, chalk: 'some'}, greeting: 'hello'}' is more general than the declared return type 'array{config: array{bread: int, cheese: int}, greeting: string}' for takesAnInt

INFO: UnusedParam - 6:28 - Param i is never referenced in this method

INFO: MoreSpecificReturnType - 4:12 - The declared return type 'array{config: array{bread: int, cheese: int}, greeting: string}' for takesAnInt is more specific than the inferred return type 'array{config: array{bread: 42, chalk: 'some'}, greeting: 'hello'}'

ERROR: TypeDoesNotContainType - 15:11 - Operand of type 0 is always falsy

ERROR: TypeDoesNotContainType - 15:11 - Type 0 for $condition is always !falsy