Skip to content

Commit

Permalink
Add path/flow support for Clang Static Analyzer *.plist reports
Browse files Browse the repository at this point in the history 
Closes SonarOpenCommunity#1707.
  • Loading branch information
Adam Romanek committed Apr 23, 2019
1 parent d822d94 commit b7e16aa
Show file tree
Hide file tree
Showing 6 changed files with 237 additions and 3 deletions.
70 changes: 70 additions & 0 deletions cxx-sensors/src/main/java/org/sonar/cxx/sensors/clangsa/CxxClangSASensor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ protected void processReport(final SensorContext context, File report)
String filePath = ((NSString) sourceFiles[fileIndex]).getContent();

CxxReportIssue issue = new CxxReportIssue(checkerName, filePath, Integer.toString(line), description);

addFlowToIssue(diag, sourceFiles, issue);

saveUniqueViolation(context, issue);
}
} catch (Exception e) {
Expand All @@ -119,4 +122,71 @@ protected CxxMetricsFactory.Key getMetricKey() {
return CxxMetricsFactory.Key.CLANG_SA_SENSOR_ISSUES_KEY;
}

private enum PathElementKind {
CONTROL, EVENT, UNKNOWN
}

private class PathElement {
private final NSDictionary pathDict;

public PathElement(NSObject pathObject) {
pathDict = (NSDictionary) pathObject;
}

public PathElementKind getKind() {
String kind = ((NSString) require(pathDict.get("kind"), "Missing mandatory entry 'kind'")).getContent();
if ("event".equals(kind)) {
return PathElementKind.EVENT;
} else if ("control".equals(kind)) {
return PathElementKind.CONTROL;
} else {
return PathElementKind.UNKNOWN;
}
}
}

private class PathEvent {
private final NSDictionary eventDict;
private final NSObject[] sourceFiles;

public PathEvent(final NSObject eventDict, final NSObject[] sourceFiles) {
this.eventDict = (NSDictionary) eventDict;
this.sourceFiles = sourceFiles;
}

public String getExtendedMessage() {
return ((NSString) require(eventDict.get("extended_message"), "Missing mandatory entry 'extended_message'"))
.getContent();
}

public String getLineNumber() {
int lineNumber = ((NSNumber) require(getLocation().get("line"), "Missing mandatory entry 'line'")).intValue();
return Integer.toString(lineNumber);
}

public String getFilePath() {
int fileIndex = ((NSNumber) require(getLocation().get("file"), "Missing mandatory entry 'file'")).intValue();
if (fileIndex < 0 || fileIndex >= sourceFiles.length) {
throw new IllegalArgumentException("Invalid file index");
}
return ((NSString) sourceFiles[fileIndex]).getContent();
}

private NSDictionary getLocation() {
return (NSDictionary) require(eventDict.get("location"), "Missing mandatory entry 'location'");
}
}

private void addFlowToIssue(final NSDictionary diagnostic, final NSObject[] sourceFiles, final CxxReportIssue issue) {
NSObject[] path = ((NSArray) require(diagnostic.objectForKey("path"), "Missing mandatory entry 'path'")).getArray();
for (NSObject pathObject : path) {
PathElement pathElement = new PathElement(pathObject);
if (pathElement.getKind() != PathElementKind.EVENT) {
continue;
}

PathEvent event = new PathEvent(pathObject, sourceFiles);
issue.addFlowElement(event.getFilePath(), event.getLineNumber(), event.getExtendedMessage());
}
}
}
17 changes: 17 additions & 0 deletions cxx-sensors/src/main/java/org/sonar/cxx/sensors/utils/CxxIssuesReportSensor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ private void saveViolation(SensorContext sensorContext, CxxReportIssue issue) {

Set<InputFile> affectedFiles = new HashSet<>();
List<NewIssueLocation> newIssueLocations = new ArrayList<>();
List<NewIssueLocation> newIssueFlow = new ArrayList<>();

for (CxxReportLocation location : issue.getLocations()) {
if (location.getFile() != null && !location.getFile().isEmpty()) {
Expand All @@ -192,12 +193,28 @@ private void saveViolation(SensorContext sensorContext, CxxReportIssue issue) {
}
}

for (CxxReportLocation location : issue.getFlow()) {
NewIssueLocation newIssueLocation = createNewIssueLocationFile(sensorContext, newIssue, location, affectedFiles);
if (newIssueLocation != null) {
newIssueFlow.add(newIssueLocation);
} else {
LOG.error("Failed to create new issue location from flow location {}", location);
newIssueFlow.clear();
break;
}
}

if (!newIssueLocations.isEmpty()) {
try {
newIssue.at(newIssueLocations.get(0));
for (int i = 1; i < newIssueLocations.size(); i++) {
newIssue.addLocation(newIssueLocations.get(i));
}
// paths with just one element are not reported as flows to avoid
// presenting 1-element flows in SonarQube UI
if (newIssueFlow.size() > 1) {
newIssue.addFlow(newIssueFlow);
}
newIssue.save();

for (InputFile affectedFile : affectedFiles) {
Expand Down
78 changes: 78 additions & 0 deletions cxx-sensors/src/test/java/org/sonar/cxx/sensors/clangsa/CxxClangSASensorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,12 @@
*/
package org.sonar.cxx.sensors.clangsa;

import java.util.Collections;
import java.util.Optional;

import static org.assertj.core.api.Assertions.assertThat;

import org.apache.commons.lang.RandomStringUtils;
import org.assertj.core.api.SoftAssertions;
import org.junit.Before;
import org.junit.Test;
Expand All @@ -30,12 +34,17 @@
import org.sonar.api.batch.fs.internal.TestInputFileBuilder;
import org.sonar.api.batch.sensor.internal.DefaultSensorDescriptor;
import org.sonar.api.batch.sensor.internal.SensorContextTester;
import org.sonar.api.batch.sensor.issue.Issue;
import org.sonar.api.batch.sensor.issue.Issue.Flow;
import org.sonar.api.batch.sensor.issue.IssueLocation;
import org.sonar.api.batch.sensor.measure.Measure;
import org.sonar.api.config.internal.MapSettings;
import org.sonar.cxx.CxxLanguage;
import org.sonar.cxx.CxxMetricsFactory;
import org.sonar.cxx.sensors.utils.TestUtils;

import com.google.common.collect.Iterables;

public class CxxClangSASensorTest {

private DefaultFileSystem fs;
Expand Down Expand Up @@ -130,6 +139,75 @@ public void shouldReportCorrectMetrics() {
softly.assertAll();
}

private String generateTestFileContents(int linesNum, int lineLen) {
String line = RandomStringUtils.randomAscii(lineLen);
return String.join("\n", Collections.nCopies(linesNum, line));
}

@Test
public void shouldReportCorrectFlows() {
SensorContextTester context = SensorContextTester.create(fs.baseDir());

settings.setProperty(language.getPluginProperty(CxxClangSASensor.REPORT_PATH_KEY),
"clangsa-reports/clangsa-report.plist");
context.setSettings(settings);

/*
* 2 issues
*/
DefaultInputFile testFile0 = TestInputFileBuilder.create("ProjectKey", "src/lib/component0.cc").setLanguage("cpp")
.setContents(generateTestFileContents(100, 80)).build();
/*
* 1 issue
*/
DefaultInputFile testFile1 = TestInputFileBuilder.create("ProjectKey", "src/lib/component1.cc").setLanguage("cpp")
.setContents(generateTestFileContents(100, 80)).build();

context.fileSystem().add(testFile0);
context.fileSystem().add(testFile1);

CxxClangSASensor sensor = new CxxClangSASensor(language);
sensor.execute(context);

assertThat(context.allIssues()).hasSize(3);

{
Issue issue = Iterables.get(context.allIssues(), 0);
assertThat(issue.flows()).hasSize(1);
Flow flow = issue.flows().get(0);
assertThat(flow.locations()).hasSize(2);

// flow locations are enumerated backwards - from the final to the root location
{
IssueLocation issueLocation = flow.locations().get(1);
assertThat(issueLocation.inputComponent()).isEqualTo(testFile0);
assertThat(issueLocation.message()).isEqualTo("'a' declared without an initial value");
assertThat(issueLocation.textRange().start().line()).isEqualTo(31);
assertThat(issueLocation.textRange().end().line()).isEqualTo(31);
}

{
IssueLocation issueLocation = flow.locations().get(0);
assertThat(issueLocation.inputComponent()).isEqualTo(testFile0);
assertThat(issueLocation.message()).isEqualTo("Branch condition evaluates to a garbage value");
assertThat(issueLocation.textRange().start().line()).isEqualTo(32);
assertThat(issueLocation.textRange().end().line()).isEqualTo(32);
}
}

// paths with just one element are not reported as flows to avoid
// presenting 1-element flows in SonarQube UI
{
Issue issue = Iterables.get(context.allIssues(), 1);
assertThat(issue.flows()).hasSize(0);
}

{
Issue issue = Iterables.get(context.allIssues(), 2);
assertThat(issue.flows()).hasSize(0);
}
}

@Test
public void invalidReportReportsNoIssues() {
SensorContextTester context = SensorContextTester.create(fs.baseDir());
Expand Down
19 changes: 16 additions & 3 deletions cxx-squid/src/main/java/org/sonar/cxx/utils/CxxReportIssue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,25 @@ public class CxxReportIssue {

private final String ruleId;
private final List<CxxReportLocation> locations;
private final List<CxxReportLocation> flow;

public CxxReportIssue(String ruleId, @Nullable String file, @Nullable String line, String info) {
super();
this.ruleId = ruleId;
this.locations = new ArrayList<>();
this.flow = new ArrayList<>();
addLocation(file, line, info);
}

public final void addLocation(@Nullable String file, @Nullable String line, String info) {
locations.add(new CxxReportLocation(file, line, info));
}

public final void addFlowElement(@Nullable String file, @Nullable String line, String info) {
flow.add(0, new CxxReportLocation(file, line, info));
}


public String getRuleId() {
return ruleId;
}
Expand All @@ -53,15 +60,20 @@ public List<CxxReportLocation> getLocations() {
return Collections.unmodifiableList(locations);
}

public List<CxxReportLocation> getFlow() {
return Collections.unmodifiableList(flow);
}

@Override
public String toString() {
String locationsToString = locations.stream().map(Object::toString).collect(Collectors.joining(", "));
return "CxxReportIssue [ruleId=" + ruleId + ", locations=" + locationsToString + "]";
String flowToString = flow.stream().map(Object::toString).collect(Collectors.joining(", "));
return "CxxReportIssue [ruleId=" + ruleId + ", locations=" + locationsToString + ", flow=" + flowToString + "]";
}

@Override
public int hashCode() {
return Objects.hash(locations, ruleId);
return Objects.hash(locations, flow, ruleId);
}

@Override
Expand All @@ -76,7 +88,8 @@ public boolean equals(Object obj) {
return false;
}
CxxReportIssue other = (CxxReportIssue) obj;
return Objects.equals(locations, other.locations) && Objects.equals(ruleId, other.ruleId);
return Objects.equals(locations, other.locations) && Objects.equals(flow, other.flow)
&& Objects.equals(ruleId, other.ruleId);
}

}
46 changes: 46 additions & 0 deletions cxx-squid/src/test/java/org/sonar/cxx/utils/CxxReportIssueTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;

import java.util.List;

import org.junit.Test;

public class CxxReportIssueTest {
Expand Down Expand Up @@ -58,4 +61,47 @@ public void reportIssueEquality() {
assertNotEquals(issue0.hashCode(), issue2.hashCode());
}

@Test
public void reportIssueEqualityConsideringFlow() {
CxxReportIssue issue0 = new CxxReportIssue("exceptThrowInDestructor", "path2.cpp", "1", "Exception thrown in destructor.");
issue0.addFlowElement("path0.cpp", "1", "a");
issue0.addFlowElement("path1.cpp", "1", "b");
issue0.addFlowElement("path2.cpp", "1", "c");

CxxReportIssue issue1 = new CxxReportIssue("exceptThrowInDestructor", "path2.cpp", "1", "Exception thrown in destructor.");
issue1.addFlowElement("path0.cpp", "1", "a");
issue1.addFlowElement("path1.cpp", "1", "b");
issue1.addFlowElement("path2.cpp", "1", "c");

CxxReportIssue issue2 = new CxxReportIssue("exceptThrowInDestructor", "path2.cpp", "1", "Exception thrown in destructor.");
issue2.addFlowElement("path1.cpp", "1", "b");
issue2.addFlowElement("path2.cpp", "1", "c");

CxxReportIssue issue3 = new CxxReportIssue("exceptThrowInDestructor", "path2.cpp", "1", "Exception thrown in destructor.");

assertEquals(issue0, issue1);
assertEquals(issue0.hashCode(), issue1.hashCode());

assertNotEquals(issue0, issue2);
assertNotEquals(issue0.hashCode(), issue2.hashCode());

assertNotEquals(issue1, issue2);
assertNotEquals(issue1.hashCode(), issue2.hashCode());

assertNotEquals(issue0, issue3);
assertNotEquals(issue0.hashCode(), issue3.hashCode());
}

@Test
public void reportIssueFlowOrder() {
CxxReportIssue issue0 = new CxxReportIssue("exceptThrowInDestructor", "path2.cpp", "1", "Exception thrown in destructor.");
issue0.addFlowElement("path0.cpp", "1", "a");
issue0.addFlowElement("path1.cpp", "2", "b");
issue0.addFlowElement("path2.cpp", "3", "c");

List<CxxReportLocation> flow = issue0.getFlow();
assertEquals(new CxxReportLocation("path2.cpp", "3", "c"), flow.get(0));
assertEquals(new CxxReportLocation("path1.cpp", "2", "b"), flow.get(1));
assertEquals(new CxxReportLocation("path0.cpp", "1", "a"), flow.get(2));
}
}
10 changes: 10 additions & 0 deletions pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,16 @@
</developer>
</developers>

<contributors>
<contributor>
<name>Adam Romanek</name>
<email>[email protected]</email>
<organization>Liberty Global B.V.</organization>
<organizationUrl>https://www.libertyglobal.com</organizationUrl>
<timezone>Europe/Warsaw</timezone>
</contributor>
</contributors>

<modules>
<module>cxx-squid</module>
<module>cxx-checks</module>
Expand Down

0 comments on commit b7e16aa

Please sign in to comment.