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

chore: Update to sonar-java 6.9.0 #156

Merged
22 changes: 11 additions & 11 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -49,22 +49,29 @@
<dependency>
<groupId>fr.inria.gforge.spoon</groupId>
<artifactId>spoon-core</artifactId>
<version>8.2.0-beta-12</version>
<version>8.3.0</version>
<exclusions>
<exclusion>
<!-- must be excluded as it is signed, which causes problems with unsigned version from sonar -->
<groupId>org.eclipse.jdt</groupId>
<artifactId>org.eclipse.jdt.core</artifactId>
</exclusion>
</exclusions>
Comment on lines +53 to +59
Copy link
Member

@algomaster99 algomaster99 Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slarse Do you remember what problems were arising in the absence of its exclusion? I am using the local build of spoon with this change in the comment, but sorald throws an error that it cannot find the .get method for HashtableOfPackage. I am assuming it is because of this line.


I tried removing the exclusion and now it throws OutOfMemory: Java heap space. Weird.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I recall, Sonar-java 6.9 pulled in (or bundled, maybe) an unsigned jdt package, while the one pulled in from Spoon is signed. Signed as in there's a GPG signature on it.

That caused errors on building it.

As both Spoon and Sonar-Java use jdt, you need to make sure that the version they depend on is the same. Otherwise you'll get strange errors.

</dependency>
<dependency>
<groupId>org.sonarsource.java</groupId>
<artifactId>java-checks-testkit</artifactId>
<version>5.14.0-SNAPSHOT</version>
<version>6.9.0.23563</version>
</dependency>
<dependency>
<groupId>org.sonarsource.java</groupId>
<artifactId>java-frontend</artifactId>
<version>5.14.0-SNAPSHOT</version>
<version>6.9.0.23563</version>
</dependency>
<dependency>
<groupId>org.sonarsource.java</groupId>
<artifactId>java-checks</artifactId>
<version>5.14.0-SNAPSHOT</version>
<version>6.9.0.23563</version>
</dependency>
<dependency>
<groupId>com.martiansoftware</groupId>
Expand Down Expand Up @@ -92,13 +99,6 @@
<maven.compiler.source>1.8</maven.compiler.source>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
</properties>
<repositories>
<repository>
<id>fermadeiral-maven-repository-snapshots</id>
<name>fermadeiral's Maven Repository</name>
<url>https://fermadeiral.github.io/maven-repository/snapshots/</url>
</repository>
</repositories>
<build>
<plugins>
<plugin>
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/sorald/Constants.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
import java.io.File;
import java.util.Collections;
import java.util.List;
import org.sonar.java.checks.*;
import org.sonar.java.se.checks.*;
import org.sonar.plugins.java.api.JavaFileScanner;
import sorald.sonar.Checks;

Expand Down
7 changes: 4 additions & 3 deletions src/main/java/sorald/DefaultRepair.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,9 @@ public void repair() {
}

Launcher launcher = createLauncher(inputDirPath, outputDirPath);
File inputBaseDir = FileUtils.getClosestDirectory(new File(inputDirPath));

Processor processor = createProcessor(ruleKey, inputDirPath);
Processor processor = createProcessor(ruleKey, inputDirPath, inputBaseDir);
this.addedProcessors.add((SoraldAbstractProcessor) processor);
Factory factory = launcher.getFactory();
ProcessingManager processingManager = new QueueProcessingManager(factory);
Expand Down Expand Up @@ -108,11 +109,11 @@ private Launcher createLauncher(String inputDirPath, String outputDirPath) {
return initLauncher(launcher, outputDirPath);
}

private Processor createProcessor(Integer ruleKey, String inputDirPath) {
private Processor createProcessor(Integer ruleKey, String inputDirPath, File inputBaseDir) {
SoraldAbstractProcessor processor = createBaseProcessor(ruleKey);
if (processor != null) {
return processor
.initResource(inputDirPath)
.initResource(inputDirPath, inputBaseDir)
.setMaxFixes(this.config.getMaxFixesPerRule());
}
return null;
Expand Down
39 changes: 39 additions & 0 deletions src/main/java/sorald/FileUtils.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package sorald;

import java.io.File;
import java.nio.file.Path;
import java.nio.file.Paths;

public class FileUtils {

/**
* Compare the two given paths as absolute, normalized paths.
*
* @param lhs A path.
* @param rhs A path.
* @return Whether or not the paths are equal as absolute, normalized paths.
*/
public static boolean pathAbsNormEqual(String lhs, String rhs) {
return pathAbsNormEqual(Paths.get(lhs), Paths.get(rhs));
}

/**
* Compare the two given paths as absolute, normalized paths.
*
* @param lhs A path.
* @param rhs A path.
* @return Whether or not the paths are equal as absolute, normalized paths.
*/
public static boolean pathAbsNormEqual(Path lhs, Path rhs) {
return lhs.toAbsolutePath().normalize().equals(rhs.toAbsolutePath().normalize());
}

/**
* @param file A file.
* @return The given file if it is a directory, or its parent directory if it is not a
* directory.
*/
public static File getClosestDirectory(File file) {
return file.isDirectory() ? file : file.getParentFile();
}
}
9 changes: 6 additions & 3 deletions src/main/java/sorald/SegmentRepair.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ public void repair() {
try {
Launcher launcher = createLauncher(segment, outputDirPath);

SoraldAbstractProcessor processor = createProcessor(ruleKey, segment, nbFixes);
File inputBaseDir =
FileUtils.getClosestDirectory(new File(config.getOriginalFilesPath()));
SoraldAbstractProcessor processor =
createProcessor(ruleKey, segment, nbFixes, inputBaseDir);
if (!this.processorNbsRepaired.containsKey(processor.getClass().getSimpleName())) {
this.processorNbsRepaired.put(processor.getClass().getSimpleName(), nbFixes);
}
Expand Down Expand Up @@ -89,11 +92,11 @@ private Launcher createLauncher(List<Node> segment, String outputDirPath) {
}

private SoraldAbstractProcessor createProcessor(
Integer ruleKey, List<Node> segment, int cachedNbFixes) throws Exception {
Integer ruleKey, List<Node> segment, int cachedNbFixes, File baseDir) {
SoraldAbstractProcessor processor = createBaseProcessor(ruleKey);
if (processor != null) {
return processor
.initResource(segment)
.initResource(segment, baseDir)
.setMaxFixes(this.config.getMaxFixesPerRule())
.setNbFixes(cachedNbFixes);
}
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/sorald/SoraldConfig.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package sorald;

import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
Expand Down Expand Up @@ -58,7 +59,8 @@ public RepairStrategy getRepairStrategy() {
}

public void setOriginalFilesPath(String originalFilesPath) {
this.originalFilesPath = originalFilesPath;
this.originalFilesPath =
Paths.get(originalFilesPath).normalize().toAbsolutePath().toString();
}

public String getOriginalFilesPath() {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/sorald/miner/MineSonarWarnings.java
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ private static Map<String, Integer> extractWarnings(String projectPath) {
}
for (JavaFileScanner javaFileScanner : SONAR_CHECK_INSTANCES) {
Set<RuleViolation> ruleViolations =
RuleVerifier.analyze(filesToScan, javaFileScanner);
RuleVerifier.analyze(filesToScan, file, javaFileScanner);
warnings.putIfAbsent(
javaFileScanner.getClass().getSimpleName(), ruleViolations.size());
}
Expand Down
12 changes: 7 additions & 5 deletions src/main/java/sorald/processor/SoraldAbstractProcessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import java.util.stream.Stream;
import org.sonar.plugins.java.api.JavaFileScanner;
import sorald.Constants;
import sorald.FileUtils;
import sorald.UniqueTypesCollector;
import sorald.segment.Node;
import sorald.sonar.RuleVerifier;
Expand All @@ -29,7 +30,7 @@ public SoraldAbstractProcessor() {}

public abstract JavaFileScanner getSonarCheck();

public SoraldAbstractProcessor initResource(String originalFilesPath) {
public SoraldAbstractProcessor initResource(String originalFilesPath, File baseDir) {
JavaFileScanner sonarCheck = getSonarCheck();
try {
List<String> filesToScan = new ArrayList<>();
Expand All @@ -46,14 +47,14 @@ public SoraldAbstractProcessor initResource(String originalFilesPath) {
e.printStackTrace();
}
}
ruleViolations = RuleVerifier.analyze(filesToScan, sonarCheck);
ruleViolations = RuleVerifier.analyze(filesToScan, baseDir, sonarCheck);
} catch (Exception e) {
e.printStackTrace();
}
return this;
}

public SoraldAbstractProcessor initResource(List<Node> segment) throws Exception {
public SoraldAbstractProcessor initResource(List<Node> segment, File baseDir) {
JavaFileScanner sonarCheck = getSonarCheck();
List<String> filesToScan = new ArrayList<>();
for (Node node : segment) {
Expand All @@ -71,7 +72,7 @@ public SoraldAbstractProcessor initResource(List<Node> segment) throws Exception
}
}

ruleViolations = RuleVerifier.analyze(filesToScan, sonarCheck);
ruleViolations = RuleVerifier.analyze(filesToScan, baseDir, sonarCheck);
return this;
}

Expand Down Expand Up @@ -111,7 +112,8 @@ public boolean isToBeProcessedAccordingToSonar(CtElement element) {
}

for (RuleViolation ruleViolation : ruleViolations) {
if (ruleViolation.getLineNumber() == line && ruleViolation.getFileName().equals(file)) {
if (ruleViolation.getLineNumber() == line
&& FileUtils.pathAbsNormEqual(ruleViolation.getFileName(), file)) {
return true;
}
}
Expand Down
1 change: 1 addition & 0 deletions src/main/java/sorald/sonar/Checks.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.sonar.java.checks.naming.MethodNamedHashcodeOrEqualCheck;
import org.sonar.java.checks.serialization.CustomSerializationMethodCheck;
import org.sonar.java.checks.serialization.ExternalizableClassConstructorCheck;
import org.sonar.java.checks.serialization.NonSerializableWriteCheck;
import org.sonar.java.checks.serialization.SerializableFieldInSerializableClassCheck;
import org.sonar.java.checks.serialization.SerializableObjectInSessionCheck;
import org.sonar.java.checks.serialization.SerializableSuperConstructorCheck;
Expand Down
115 changes: 108 additions & 7 deletions src/main/java/sorald/sonar/RuleVerifier.java
Original file line number Diff line number Diff line change
@@ -1,10 +1,26 @@
package sorald.sonar;

import java.util.Arrays;
import static java.nio.charset.StandardCharsets.UTF_8;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import org.sonar.java.checks.verifier.MultipleFilesJavaCheckVerifier;
import org.sonar.api.batch.fs.InputFile;
import org.sonar.api.batch.fs.internal.DefaultFileSystem;
import org.sonar.api.batch.fs.internal.TestInputFileBuilder;
import org.sonar.api.batch.sensor.internal.SensorContextTester;
import org.sonar.java.AnalyzerMessage;
import org.sonar.java.SonarComponents;
import org.sonar.java.ast.JavaAstScanner;
import org.sonar.java.checks.verifier.JavaCheckVerifier;
import org.sonar.java.model.VisitorsBridge;
import org.sonar.java.se.SymbolicExecutionMode;
import org.sonar.plugins.java.api.JavaFileScanner;

/** Adapter class for interfacing with sonar-java's verification and analysis facilities. */
Expand All @@ -18,7 +34,7 @@ public class RuleVerifier {
*/
@SuppressWarnings("UnstableApiUsage")
public static void verifyHasIssue(String filename, JavaFileScanner check) {
verifyHasIssue(Arrays.asList(filename), check);
JavaCheckVerifier.newVerifier().onFile(filename).withCheck(check).verifyIssues();
}

/**
Expand All @@ -29,19 +45,32 @@ public static void verifyHasIssue(String filename, JavaFileScanner check) {
*/
@SuppressWarnings("UnstableApiUsage")
public static void verifyHasIssue(List<String> filesToScan, JavaFileScanner check) {
MultipleFilesJavaCheckVerifier.verify(filesToScan, check, true);
filesToScan.forEach(filename -> verifyHasIssue(filename, check));
}

/**
* Analyze the files with respect to check.
*
* @param filesToScan A list of paths to files.
* @param baseDir The base directory of the current project.
* @param check A Sonar check.
* @return All messages produced by the analyzer, for all files.
*/
@SuppressWarnings("UnstableApiUsage")
public static Set<RuleViolation> analyze(List<String> filesToScan, JavaFileScanner check) {
return MultipleFilesJavaCheckVerifier.verify(filesToScan, check, false).stream()
public static Set<RuleViolation> analyze(
List<String> filesToScan, File baseDir, JavaFileScanner check) {
List<InputFile> inputFiles =
filesToScan.stream()
.map(filename -> toInputFile(baseDir, filename))
.collect(Collectors.toList());

SoraldSonarComponents sonarComponents = createSonarComponents(baseDir);
JavaAstScanner scanner =
createAstScanner(sonarComponents, Collections.singletonList(check));

scanner.scan(inputFiles);

return sonarComponents.getMessages().stream()
.map(RuleViolation::new)
.collect(Collectors.toSet());
}
Expand All @@ -54,6 +83,78 @@ public static Set<RuleViolation> analyze(List<String> filesToScan, JavaFileScann
*/
@SuppressWarnings("UnstableApiUsage")
public static void verifyNoIssue(String filename, JavaFileScanner check) {
MultipleFilesJavaCheckVerifier.verifyNoIssue(Arrays.asList(filename), check);
JavaCheckVerifier.newVerifier().onFile(filename).withCheck(check).verifyNoIssues();
}

private static InputFile toInputFile(File baseDir, String filename) {
// must append a separator to the basedir string as Sonar appends the filenames directly to
// it
final String baseDirStr = baseDir.toString() + File.separator;
try {
return new TestInputFileBuilder(baseDirStr, filename)
.setContents(new String(Files.readAllBytes(Paths.get(filename)), UTF_8))
.setCharset(UTF_8)
.setLanguage("java")
.build();
} catch (IOException e) {
throw new RuntimeException("failed to read file " + filename);
}
}

@SuppressWarnings("UnstableApiUsage")
private static JavaAstScanner createAstScanner(
SonarComponents sonarComponents, List<JavaFileScanner> checks) {
JavaAstScanner scanner = new JavaAstScanner(sonarComponents);
VisitorsBridge visitorsBridge =
new VisitorsBridge(
checks,
// TODO set the classpath to something reasonable
Collections.emptyList(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This argument sets the classpath for the VisitorsBridge. It's later supplied to the parser, so I'm assuming that it's used to resolve types etc (but I haven't checked inside the parser). We will probably want to set it to something reasonable in the context. For example, when analyzing a project that uses maven, the full classpath can be generated with mvn dependency:build-classpath.

sonarComponents,
SymbolicExecutionMode.getMode(checks.toArray(new JavaFileScanner[0])));
// TODO we may want to set the version of the visitors bridge, as without setting it the
// implementation defaults to the highest version supported by the parser (currently
// Java 14)
// visitorsBridge.setJavaVersion(new JavaVersionImpl(8));
scanner.setVisitorBridge(visitorsBridge);
return scanner;
}

private static SoraldSonarComponents createSonarComponents(File baseDir) {
// FIXME The SensorContextTester is an internal and unstable component in sonar,
// we should implement our own SensorContext
SensorContextTester context = SensorContextTester.create(baseDir);
Comment on lines +124 to +126
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the comment in the code says, we shouldn't be using the internal SensorContextTester, but it will serve for the time being.

SoraldSonarComponents sonarComponents = new SoraldSonarComponents(context.fileSystem());
sonarComponents.setSensorContext(context);
return sonarComponents;
}

/**
* A simple subclass of SonarComponents that stores all analyzer messages. These are by default
* stored in a storage container, but it seems easier for our use case to just intercept them.
*
* <p>This IS a bit of a hack, so it wouldn't be unreasonable to try to do this the "proper
* way".
*/
private static class SoraldSonarComponents extends SonarComponents {
private final List<AnalyzerMessage> messages;

public SoraldSonarComponents(DefaultFileSystem fs) {
// FIXME I'm very unsure about supplying null for all of these constructor values.
// They should not be null, but at this time I don't know that to put there, and
// with our current usage this hack appears to work.
super(null, fs, null, null, null, null);
Comment on lines +143 to +146
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the comment says, this is a pretty nasty hack. It appears to work, but it's definitely not pretty nor safe as it initializes fields that should not be null to null.

messages = new ArrayList<>();
}

@Override
public void reportIssue(AnalyzerMessage analyzerMessage) {
super.reportIssue(analyzerMessage);
messages.add(analyzerMessage);
}

public List<AnalyzerMessage> getMessages() {
return Collections.unmodifiableList(messages);
}
}
}
1 change: 1 addition & 0 deletions src/test/java/sorald/processor/ProcessorTestHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ static Stream<ProcessorTestCase<?>> getTestCaseStream() {
.map(File::listFiles)
.flatMap(Arrays::stream)
.filter(file -> file.getName().endsWith(".java"))
.filter(file -> !file.getName().startsWith("IGNORE"))
.map(ProcessorTestHelper::toProcessorTestCase);
}

Expand Down
Loading