-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
refactor: Fix PSalm issues. #194
Conversation
first_checkIntElement(Collection::fromIterable([1, 2, 3])->first()->current()); | ||
/** @psalm-suppress PossiblyNullArgument @phpstan-ignore-next-line */ | ||
/** @psalm-suppress NullArgument @phpstan-ignore-next-line */ |
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.
this might be problematic I think if Psalm always thinks that these are null
. I tried debugging a bit:
INFO: Trace - tests/static-analysis/last.php:59:1 - $var: loophp\collection\Contract\Collection<0|1|2, 1|2|3>&loophp\collection\Contract\Collection (see https://psalm.dev/224)
/** @psalm-trace $var */
$var = Collection::fromIterable([1, 2, 3])->first();
Psalm now sees first()
as creating an intersection type for some reason and I think that's why when applying current()
after it resolves the type to null
.
I tried to replicate somewhat in the playground but couldn't :( https://psalm.dev/r/609938fcc6
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.
I think there is an issue and we should let the PSalm team know about it.
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.
I will open an issue and see where we get, if it's a legitimate thing or not
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.
@drupol opened an issue here, let's see what they think: vimeo/psalm#6685
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.
The code changes themselves are fine for now so I think there's no harm in getting this merged to fix the code analysis. We might need to figure out the root of the cause for this later, though it might just be a Psalm bug.
This PR:
I'm definitely unsure about this thing, @AlexandruGG WDYT?