Track nullable state in try statements #31082
Conversation
gafter
added
Bug
Area-Compilers
Feature - Nullable Reference Types
|
@gafter looks like there are some merge conflicts to resolve. #Resolved |
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Outdated
Show resolved
Hide resolved
…nto dev16.0-preview2-30561
|
Done review pass (commit 1) #Resolved |
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Show resolved
Hide resolved
| } | ||
| catch (System.Exception) | ||
| { | ||
| _ = s.Length; // warning 1 |
There was a problem hiding this comment.
_ = s.Length; // warning 1 [](start = 12, length = 26)
test idea: if we did s = string.Empty; here, I assume that warning 2 disappears. #Closed
There was a problem hiding this comment.
No. It is still possible for "something" to be thrown that isn't caught, even though catching it is not expressible. The language cannot express catching "everything".
In reply to: 232855231 [](ancestors = 232855231)
There was a problem hiding this comment.
Even catch { } will not catch everything?
In reply to: 232871216 [](ancestors = 232871216,232855231)
There was a problem hiding this comment.
Yes, catch {} will catch "everything" thrown. However, it would be incorrect to treat that as a special case that suppresses the warning in the finally, as it is still possible for s to be null in the finally (because an exception could occur on entry to the catch clause).
In reply to: 233190864 [](ancestors = 233190864,232871216,232855231)
| MayThrow(); | ||
| s = string.Empty; | ||
| } | ||
| catch (System.Exception) |
There was a problem hiding this comment.
catch (System.Exception) [](start = 8, length = 24)
test ideas:
Consider testing with catch that catches some other exception type? I think this will not affect the analysis, but when I saw the tests, I assumed that System.Exception was handled specially (as a catch all).
Consider some tests with multiple catch blocks.
Also, maybe some nested try/catch scenarios? #Resolved
There was a problem hiding this comment.
System.Exception is not handled specially.
Adding a test for multiple catch blocks.
Not adding nested scenarios because it is all compositional. There is no special handling of nesting for try blocks. Trying to test nesting of constructs would cause an explosion of the tests.
In reply to: 232864656 [](ancestors = 232864656)
There was a problem hiding this comment.
Actually, I will add some nested scenarios. There is nesting-specific state handling code.
In reply to: 233151144 [](ancestors = 233151144,232864656)
| Optional<TLocalState> oldTryState = _tryState; | ||
| _tryState = AllBitsSet(); | ||
| VisitTryBlock(tryBlock, node, ref tryState); | ||
| var tts = _tryState.Value; |
There was a problem hiding this comment.
tts [](start = 20, length = 3)
nit: even though this is moving existing code, consider giving tts and ots proper names. #Resolved
| } | ||
|
|
||
| [Fact, WorkItem(30561, "https://github.com/dotnet/roslyn/issues/30561")] | ||
| public void SetNullableStateInNestedTry_01() |
There was a problem hiding this comment.
SetNullableStateInNestedTry_01 [](start = 20, length = 30)
@jcouv: this tests nested try statements as suggested. #Resolved
There was a problem hiding this comment.
The precise flow-analysis rules for nullable analysis are not specified, so there isn't anything to update. |
|
@jaredpar For approval, please |
|
approved |
Fixes #30561