Skip to content
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

Merged
merged 22 commits into from Feb 28, 2023

Conversation

martin-strecker-sonarsource
Copy link
Contributor

Fixes #6766

The old SE engine throws NotSupportedException for a lot of pattern kinds:

  • ParenthesizedPattern
  • BinaryPattern
  • UnaryPattern
  • VarPattern
  • RelationalPattern
  • ListPattern
  • SlicePattern

This PR fixes this by bypassing the patterns.

@martin-strecker-sonarsource
Copy link
Contributor Author

@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:

public void Cfg_VarPattern_InSwitchExpression_IsNotSupported()
{
var exception = Assert.ThrowsException<NotSupportedException>(() => Build(@"string a = taintedString switch {var x => null};"));
exception.Message.Should().Be("VarPattern");
}
[TestMethod]
public void Cfg_VarPattern_InIf_IsNotSupported()
{
var exception = Assert.ThrowsException<NotSupportedException>(() => Build(@"if (tainted is var x) { }"));
exception.Message.Should().Be("VarPattern");
}
[TestMethod]
public void Cfg_NotPattern_InIf_IsNotSupported()
{
var exception = Assert.ThrowsException<NotSupportedException>(() => Build(@"if (tainted is not null) { }"));
exception.Message.Should().Be("NotPattern");
}
[TestMethod]
public void Cfg_AndPattern_InIf_IsNotSupported()
{
var exception = Assert.ThrowsException<NotSupportedException>(() => Build(@"if (tainted is int and > 0) { }"));
exception.Message.Should().Be("AndPattern");
}
[TestMethod]
public void Cfg_OrPattern_InIf_IsNotSupported()
{
var exception = Assert.ThrowsException<NotSupportedException>(() => Build(@"if (tainted is string or int) { }"));
exception.Message.Should().Be("OrPattern");
}

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?

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a 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.

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a 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 :/

@martin-strecker-sonarsource
Copy link
Contributor Author

@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.

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a 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

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a 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

@sonarcloud
Copy link

sonarcloud bot commented Feb 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Feb 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@martin-strecker-sonarsource
Copy link
Contributor Author

I double-checked and the latest changes are still fixing the AD0001 for Parenthesized and List patterns (all other patterns were supported already).

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a 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 🎉

@pavel-mikula-sonarsource pavel-mikula-sonarsource changed the title SE: Handle unsupported pattern types gracefully Old SE: Handle unsupported syntax gracefully Feb 28, 2023
@pavel-mikula-sonarsource pavel-mikula-sonarsource merged commit 5c8e7e3 into master Feb 28, 2023
@pavel-mikula-sonarsource pavel-mikula-sonarsource deleted the Martin/SE_Bug_6766 branch February 28, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Old SE: Handle unsupported syntax gracefully
2 participants