-
Notifications
You must be signed in to change notification settings - Fork 659
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
Rewrite TryAnalyzer #7688
base: master
Are you sure you want to change the base?
Rewrite TryAnalyzer #7688
Conversation
Shepherd failure is waiting for #7684. |
Could someone baseline the PHPUnit cast to int errors? I almost didn't notice the extra errors that showed up there from this PR. I'll look tomorrow to see if they're anything to worry about. |
deb491c
to
21de80a
Compare
This is currently failing with |
I found these snippets: https://psalm.dev/r/491bbefc48<?php
try {
} catch (Exception $_e) {
$e = $_e;
}
if (!isset($e)) {
}
try {
} catch (Throwable $_e) {
$e = $e ?? $_e;
}
|
6e33152
to
c32ca03
Compare
I found these snippets: https://psalm.dev/r/ea3ae076d5<?php
class A {}
class B {}
$is_a = false;
$foo = new B();
if (random_int(0, 1)) {
$is_a = true;
$foo = new A();
}
if ($is_a) {
takesA($foo);
}
function takesA(A $_): void {}
|
c32ca03
to
f22d248
Compare
f22d248
to
b1ba365
Compare
|
||
use Psalm\CodeLocation; | ||
|
||
final class RedundantCatch extends CodeIssue |
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.
It probably makes sense to extend it from ClassIssue
so that users could suppress it for individual exception classes.
$exact = false; | ||
|
||
$var = $expr; |
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.
Is there any reason to alias this? $expr
doesn't seem to be used in the loop body.
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.
Nope, that must be a leftover change from when I was figuring out how to get it working.
One possible pattern that employs both |
I found these snippets: https://psalm.dev/r/8e926f8c41<?php
class DB {
private int $transactionLevel = 0;
/**
* @template T
* @param callable():T $f
* @return T
*/
public function transactional(callable $f) {
try {
if (0 === $this->transactionLevel++) {
$this->begin();
}
$ret = $f();
if (1 === $this->transactionLevel) {
$this->commit();
}
} catch (Throwable $e) {
$this->rollBack();
throw $e;
} finally {
$this->transactionLevel--;
}
return $ret;
}
private function begin(): void {}
private function commit(): void {}
private function rollBack(): void {}
}
|
'code' => '<?php | ||
$foo = 1; | ||
try { | ||
$foo = 2; | ||
} catch (Exception $_) { | ||
unset($foo); | ||
} | ||
', | ||
'assertions' => [ | ||
'$foo?===' => '1|2', |
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.
Shouldn't it be '$foo?===' => '2'
? Or do we assume exception could have been thrown after the assignment?
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 it's assumed to consider that an exception could happen at any moment. Maybe not on a simple case like this, but if you replace $foo = 2;
by $foo[] = 2;
you get an error that could be caught.
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 mean, assignment either happened and foo
is 2
or didn't (due to exception) and foo
is unset, so to me it looks like it should result in a possibly unset 2 => '$foo?===' => '2'
.
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're right, I'll have to take a look at that. If an uncaught exception is thrown execution won't continue, if a caught exception is thrown $foo
is unset, and if no exception is thrown $foo
is 2
. I think the issue is that right now it only considers a variable overridden by the catch
if an assignment happens, it doesn't realize that the unset
also lets us take only the last value from the try
.
} | ||
', | ||
'assertions' => [ | ||
'$foo===' => '1|2', |
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 don't see how this could ever result in 1
.
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.
Similar to above, returning catch
es should allow us to assume the try
completed successfully. I'm not quite sure why this one isn't working though, I thought I'd accounted for that.
'$foo' => 'int|mixed', | ||
'$bar' => 'mixed|string', |
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.
it's probably just mixed
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.
We could probably add a TypeCombiner::combine to collapse those
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.
🤷 that's what extract
gives. I've never liked that Psalm allows combining mixed
in Union
s without collapsing. I think the idea was to allow better completion for the language server, but I'd rather have the language server address that separately and have a sane type system. This is something I'm changing in my type comparison rewrite.
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 idea was to allow better completion
I think it's just a bug. Given the sheer amount of Unions created, it would probably be a horrible place to put an automatic combining there for performances, but IMHO, we should never end up with this kind of types, it's just the symptom of a wrong handling somewhere
That one should be fine, it only becomes an issue if the |
I found these snippets: https://psalm.dev/r/8e926f8c41<?php
class DB {
private int $transactionLevel = 0;
/**
* @template T
* @param callable():T $f
* @return T
*/
public function transactional(callable $f) {
try {
if (0 === $this->transactionLevel++) {
$this->begin();
}
$ret = $f();
if (1 === $this->transactionLevel) {
$this->commit();
}
} catch (Throwable $e) {
$this->rollBack();
throw $e;
} finally {
$this->transactionLevel--;
}
return $ret;
}
private function begin(): void {}
private function commit(): void {}
private function rollBack(): void {}
}
|
Is it still planned to finish this? It would be nice for all the related issues to be fixed (I just ran into #5700). |
I should have some time again next month, I'll try to rebase it and see what issues pop up, I think there was a thorny one with the |
As usual I bit off way more than I intended. It turns out try/catch/finally is actually really complicated...
Before reviewing I strongly recommend looking through the
unionAssignmentsWithMultipleNestedTry
test line by line and fully understanding it.Fixes #5123, fixes #5700, fixes #5967, fixes #5973, fixes #6286, fixes #6342, fixes #6659, fixes #7249.
The one thing that really annoys me about this change is that the
finally
analysis assumes that there will never be an uncaught exception thrown in acatch
:If anyone strongly objects to this I'll change it back, I just hate to throw away all that work :/.
I'm personally fairly ok with this since I don't think throwing in a
catch
with afinally
after it is too common of a pattern anyway, and even when it does happen I'd be surprised if these false negatives/positives caused a problem. It also helps prevent other false positives as well: