-
Notifications
You must be signed in to change notification settings - Fork 231
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
Fix S2583 FP: nested try catch blocks #8484
Conversation
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 test case for nested try-catch blocks is missing. Add it to ConditionEvaluatesToConstant.cs
// Reproducer for https://github.com/SonarSource/sonar-dotnet/issues/8484
public class Repro_8484
{
void TestMethod()
{
Exception failure = null;
try
{
try
{
var x = new object();
}
catch (InvalidOperationException)
{
}
}
catch (NotSupportedException exception)
{
failure = exception;
}
if (failure != null) // Compliant
{
Console.WriteLine("Found failures.");
}
}
}
...alyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Monitor.TryCatch.cs
Show resolved
Hide resolved
...narAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.TryCatchFinally.cs
Show resolved
Hide resolved
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.
Looks good. I like the refactoring with handlersAreExhaustive
variable; removing the yield break makes the code a bit more readable
@@ -52,7 +52,7 @@ public void Method4(bool condition) | |||
|
|||
public void Method5(string arg) | |||
{ | |||
Monitor.Enter(obj); // Compliant | |||
Monitor.Enter(obj); // Noncompliant |
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.
This was actually an FN. In case of a not-NullReference-Exception, Exit
would not be called.
...narAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.TryCatchFinally.cs
Show resolved
Hide resolved
...narAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.TryCatchFinally.cs
Show resolved
Hide resolved
...narAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.TryCatchFinally.cs
Show resolved
Hide resolved
...ts/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RoslynSymbolicExecution.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RoslynSymbolicExecution.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RoslynSymbolicExecution.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RoslynSymbolicExecution.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RoslynSymbolicExecution.cs
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RoslynSymbolicExecution.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RoslynSymbolicExecution.cs
Show resolved
Hide resolved
bcebbfb
to
3cb2551
Compare
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 "because" seems wrong. And it actually turns out it is really needed there :)
...narAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.TryCatchFinally.cs
Outdated
Show resolved
Hide resolved
...narAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.TryCatchFinally.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RoslynSymbolicExecution.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/RoslynSymbolicExecution.cs
Show resolved
Hide resolved
3cb2551
to
91f036c
Compare
@pavel-mikula-sonarsource I have rewritten the code to be more readable and have improved precision at the same time. I will be off on holiday. If more changes are necessary, please forward the PR to @cristian-ambrosini-sonarsource. |
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.
LGTM
|
|
Fixes #8140