-
Notifications
You must be signed in to change notification settings - Fork 226
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
Old SE: Handle unsupported syntax gracefully #6768
Conversation
@costin-zaharia-sonarsource or @pavel-mikula-sonarsource We got a report about an AD0001 in the old SE engine (ParenthesizedPattern not supported exception). This PR tries to fix this, by suppressing the exception: Ignore the patterns by returning the unmodified SE state). Now there are some tests failing that explicit test that the exception is thrown: sonar-dotnet/analyzers/tests/SonarAnalyzer.UnitTest/CFG/Sonar/SonarControlFlowGraphTest.cs Lines 4105 to 4142 in 6bc8c18
My suspicion is that it is possible to throw such exceptions in either CSharpControlFlowGraphBuilder or SonarExplodedGraph but I need some advice on how to do the suppression properly. Can you have a look, please? |
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 UTs that are failing becuase they expect an exception can be deleted now.
analyzers/src/SonarAnalyzer.CFG/Sonar/CSharpControlFlowGraphBuilder.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Sonar/SonarExplodedGraphTest.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Sonar/SonarExplodedGraph.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/CFG/Sonar/SonarControlFlowGraphTest.cs
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Sonar/SonarExplodedGraphTest.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.
I took a deeper look and we're catching the wrong ghost.
We need a reproducer to start with. And fix that instead :/
analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Sonar/SonarExplodedGraphTest.cs
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Sonar/SonarExplodedGraphTest.cs
Outdated
Show resolved
Hide resolved
@pavel-mikula-sonarsource I added a reproducer now (see #6768 (comment)) (Spoiler: You were right about fishing in the wrong waters. The AD0001 was likely caused by a pattern in a switch statement). I did not remove the other tests yet as I wanted to get your feedback about the reproducer first. |
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 repro and actual fix looks good. Requesting changes to move the card for final cleanup
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.
Let's never throw
analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Sonar/SonarExplodedGraphTest.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Sonar/SonarExplodedGraph.cs
Outdated
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
I double-checked and the latest changes are still fixing the AD0001 for Parenthesized and List patterns (all other patterns were supported already). |
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, no more value-less AD0001s 🎉
Fixes #6766
The old SE engine throws
NotSupportedException
for a lot of pattern kinds:This PR fixes this by bypassing the patterns.