-
Notifications
You must be signed in to change notification settings - Fork 665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Array-like-object comparison message improvement #7445
base: master
Are you sure you want to change the base?
Conversation
I like the idea! I'm thinking maybe we should declare your new method as I think you'll need to add a check that the |
That makes sense, I can carry out both of those changes |
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. |
It's definitely a good idea. It definitely came up a few time in issues, for example among the last responses to #3379 |
$extension = ''; | ||
|
||
// Not sure if there's a better or more robust way to do this | ||
$param_types = $param_type->getAtomicTypes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah sorry, I read your response after my review^^
// There's many ways to illustrate this but this is the simplest and provides info | ||
// without being too opinionated | ||
$extension .= '. The differences are in the following keys: '; | ||
$param_keys = array_keys($first_param_type->properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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:
***@***.***>
|
I've just pushed to implement:
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. |
@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? |
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. |
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. |
@M1ke You can add a test somewhere (I'm not seeing an obviously applicable place, so maybe a new file) and debug it like 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):
to |
@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 |
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. |
@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" |
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: 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: |
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) {}
|
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.
In this case the eagle-eyed may spot that the second argument has misspelled
bread
asbreead
.The PR as currently implemented would augment this report as so:
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