Handle return statements with no expression in CSharpAsAndNullCheckDi… #25258
Conversation
| if (node == null) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
consider adding the null check above the switch-statement. then we can remove all the null checks inside the switch cases.
| @@ -413,6 +413,11 @@ private static SyntaxNode GetLeftmostCondition(SyntaxNode node) | |||
| { | |||
| while (true) | |||
| { | |||
| if (node == null) | |||
| { | |||
| return null; | |||
There was a problem hiding this comment.
why not while (node != null)?
| } | ||
|
|
||
| node = value; | ||
| node = declarators.Count == 1 ? declarators[0].Initializer?.Value : null; | ||
| continue; |
There was a problem hiding this comment.
what about having break; in all cases and a return from the default case? might be easier to understand in my opinion than having continue from inside a switch statement
There was a problem hiding this comment.
I would prefer to keep an escrow regression fix minimal, without style changes. Personally, I also prefer code as per your suggestion, but the current style is also readable as we loop with explicit continue statements and exit when we reach null node or node of unhandled kind.
|
+1 on making teh while loop simpler. However, i would still keep the 'continue' form as that make a lot of sense to me. Each block decomposes the node to get the 'left' side, then 'continues' the act of decomposition. 'break' makes me think that something will come 'next', when all that you really want to say is "keep going". This is effectively a recursive algorithm that is non recursive (because of concerns about large expressions iirc). So keeping the concepts similar between the two would be my preference. -- That said, i think large expressions here are pretty darn unlikely, so (after ask mode) i think it would be fine to be made recursive. |
…agnosticAnalyzer.GetLeftmostCondition Fixes dotnet#25237
|
@jasonmalinowski @dotnet/roslyn-infrastructure - it seems ubuntu_16_mono_debug_prtest is failing for an unrelated reason, and it is failing in quite a few branches as well. |
|
@mavasani That's fine. This is a branch we just created for these bugfixes, and it doesn't have the fix to fix that. Go ahead and ignore it. |
|
QB approval pending |
|
Approved to merge via link |
…agnosticAnalyzer.GetLeftmostCondition
Fixes #25237
Ask Mode template
Customer scenario
Open a C# source file which has a void method with a
return;statement. The built-in IDE analyzerCSharpAsAndNullCheckDiagnosticAnalyzerthrows a NullReferenceException producing an AD0001 diagnostic in the error list. Multiple instances of this diagnostic can easily show up in a normal C# solution.Bugs this fixes
#25237
Workarounds, if any
Disable IDE0019 using a ruleset entry for every project that has return statements with no expression.
Risk
Low risk. Fix involves just adding a null check.
Performance impact
None
Is this a regression from a previous update?
Yes, recent regression introduced in 10726d1#diff-64e1a867f5d1363da7ca518cdb658623.
Root cause analysis
Missing unit tests. A regression test has been added with the PR. Verified the NRE on the added unit test before the fix.
How was the bug found?
Dogfooding.
Test documentation updated?
N/A