add tracking of NotNullInfo for Match, Case, Try trees (fix #21380) #21389
Conversation
|
This would benefit from additional tests. @noti0na1 would you be able to add some? |
|
I fixed the NotNullInfos for Try and added some more tests. We want the retracted NotNullInfo from the body of |
| ) | ||
| ) | ||
| val nni = pat1.notNullInfo | ||
| .seq(guard1.notNullInfoIf(false).alt(guard1.notNullInfoIf(true))) |
There was a problem hiding this comment.
If the guard is false, the body does not execute. I don't think it's sound to .seq the body after an .alt that includes the case of the guard being false.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // Hence, we create a copy of cases with empty body and type check that first, then type check | ||
| // the rest of the tree in order. | ||
| val casesEmptyBody1 = tree.cases.mapconserve(cpy.CaseDef(_)(body = EmptyTree)) | ||
| val casesEmptyBody2 = typedCases(casesEmptyBody1, EmptyTree, defn.ThrowableType, WildcardType) |
There was a problem hiding this comment.
Does this work if the pattern uses a variable whose nullability is changed by the try block (expr)? Won't we be typing such a variable in the pattern with the wrong type here?
There was a problem hiding this comment.
The addCanThrowCapabilities only uses the types of exception patterns and whether the guards are empty, so I think it is ok here.
In general, since we don't know at which point the try body will throw an exception, the catch cases and statements after try should only rely on the retracted info from the body.
| ).seq(finalizer1.notNullInfo) | ||
|
|
||
| var nni = expr2.notNullInfo.retractedInfo | ||
| if cases2.nonEmpty then nni = nni.seq(cases2.map(_.notNullInfo).reduce(_.alt(_))) |
There was a problem hiding this comment.
It's possible that none of the cases match (i.e. that no exception is thrown). So we either need an .alt with an empty NotNullInfo or we need to take the .retractedInfo of the cases (those two alternatives should be equivalent).
There was a problem hiding this comment.
Yes, we could have no catch case in try.
|
I may not have time to work on this PR until next week. Feel free to modify/remove my commit. |
|
Emm, one test fails with
|
It is tricky to fix the error: in the example, when the pat of the case is typed, the type argument is bind with a fresh name, so if we type check the cases twice, the Normally, this is not an issue, because this kind of cc @jchyb |
|
@noti0na1 Feel free to add |
No description provided.