-
Notifications
You must be signed in to change notification settings - Fork 672
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
Remove special handling for elseifs that breaks for else if #8273
Remove special handling for elseifs that breaks for else if #8273
Conversation
I found these snippets: https://psalm.dev/r/a1f9a6439e<?php
class A {}
class B extends A {}
function foo(?A $a, ?A $b): A {
if ($a) {
$a = new B;
} elseif ($b) {
// do nothing
} else {
return new A;
}
if (!$a) return $b;
return $a;
}
|
Feels weird to make a change that only worsen the analysis, even for the good reasons. I guess fixing it for all the cases would be hard, right? Maybe we should leave a small comment to describe that a feature is missing there? |
I added a comment! The original test was added over 5 years ago, which likely means it was found in Vimeo code at some point. It's possible that Psalm's analysis will generate false-positives on other codebases due to this change, but I think that's pretty unlikely. And I think slightly-simplifying if/else analysis is worth that disruption. |
This matches a general limitation of Psalm when dealing with nested versions of things: Consider https://psalm.dev/r/c8a6cfe83c vs https://psalm.dev/r/9a69e87e69 Both equivalent, but Psalm's special-sauce generation of logical clauses from these sorts of if statements only works at the level they're expressed on. |
I found these snippets: https://psalm.dev/r/c8a6cfe83c<?php
class A {}
function foo(?A $one, ?A $two) : A {
if (!$one) {
if (!$two) {
throw new \Exception();
}
}
if (!$one) {
return $two;
}
return $one;
}
https://psalm.dev/r/9a69e87e69<?php
class A {}
function foo(?A $one, ?A $two) : A {
if (!$one && !$two) {
throw new \Exception();
}
if (!$one) {
return $two;
}
return $one;
}
|
Let's roll with it then! We'll see if we get feedback! |
Thanks! |
This is a weird PR, because it breaks a niche existing analysis, but I think for the right reasons.
The test
twoVarLogicNotNestedWithElseifCorrectlyReinforcedInIf
breaks when theelseif
is changed toelse if
, and the "elseif" version doesn't work online either: https://psalm.dev/r/a1f9a6439e for unknown reasons.Generally I think it's bad when
elseif
andelse if
are treated differently (maybe I erred in having separate handling forelseif
). Either way, I think this is a minimally-bad change, and it limits the use of theUnion::substitute
method, which I don't think should be used in if/else handling — if/else handling should just use regular type reconciliation everywhere.I saw this mismatch when looking at Hack (which has no
elseif
)