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.

Sign up for GitHub

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

Jump to bottom

UnnecessaryJavacSuppressWarnings: reimplement in a simpler way, by relying on compiler diagnostics to detect necessary suppressions, rather than attempting to reimplement the compiler logic #2632

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

UnnecessaryJavacSuppressWarnings: reimplement in a simpler way, by relying on compiler diagnostics to detect necessary suppressions, rather than attempting to reimplement the compiler logic #2632

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
195 changes: 128 additions & 67 deletions check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@
import static com.sun.tools.javac.util.Position.NOPOS;
import static java.util.stream.Collectors.joining;

import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.base.Objects;
Expand All @@ -51,6 +52,7 @@
import com.google.errorprone.apply.DescriptionBasedDiff;
import com.google.errorprone.apply.ImportOrganizer;
import com.google.errorprone.apply.SourceFile;
import com.google.errorprone.fixes.SuggestedFixes.FixCompiler.Result;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.ErrorProneToken;
import com.google.errorprone.util.FindIdentifiers;
Expand Down Expand Up @@ -122,6 +124,7 @@
import java.util.TreeMap;
import java.util.TreeSet;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.stream.IntStream;
import java.util.stream.StreamSupport;
import javax.annotation.Nullable;
import javax.lang.model.element.Element;
Expand Down Expand Up @@ -1225,76 +1228,16 @@ private static boolean compilesWithFix(
return true;
}

JCCompilationUnit compilationUnit = (JCCompilationUnit) state.getPath().getCompilationUnit();
JavaFileObject modifiedFile = compilationUnit.getSourceFile();
BasicJavacTask javacTask = (BasicJavacTask) state.context.get(JavacTask.class);
if (javacTask == null) {
throw new IllegalArgumentException("No JavacTask in context.");
}
Arguments arguments = Arguments.instance(javacTask.getContext());
List<JavaFileObject> fileObjects = new ArrayList<>(arguments.getFileObjects());
URI modifiedFileUri = modifiedFile.toUri();
for (int i = 0; i < fileObjects.size(); i++) {
final JavaFileObject oldFile = fileObjects.get(i);
if (modifiedFileUri.equals(oldFile.toUri())) {
DescriptionBasedDiff diff =
DescriptionBasedDiff.create(compilationUnit, ImportOrganizer.STATIC_FIRST_ORGANIZER);
diff.handleFix(fix);
SourceFile fixSource;
try {
fixSource =
new SourceFile(
modifiedFile.getName(),
modifiedFile.getCharContent(false /*ignoreEncodingErrors*/));
} catch (IOException e) {
return false;
}
diff.applyDifferences(fixSource);
fileObjects.set(
i,
new SimpleJavaFileObject(sourceURI(modifiedFile.toUri()), Kind.SOURCE) {
@Override
public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOException {
return fixSource.getAsSequence();
}
});
break;
}
}
DiagnosticCollector<JavaFileObject> diagnosticListener = new DiagnosticCollector<>();
Context context = new Context();
Options options = Options.instance(context);
Options originalOptions = Options.instance(javacTask.getContext());
for (String key : originalOptions.keySet()) {
String value = originalOptions.get(key);
if (key.equals("-Xplugin:") && value.startsWith("ErrorProne")) {
// When using the -Xplugin Error Prone integration, disable Error Prone for speculative
// recompiles to avoid infinite recursion.
continue;
}
if (SOURCE_TARGET_OPTIONS.contains(key) && originalOptions.isSet("--release")) {
// javac does not allow -source and -target to be specified explicitly when --release is,
// but does add them in response to passing --release. Here we invert that operation.
continue;
}
options.put(key, value);
}
JavacTask newTask =
JavacTool.create()
.getTask(
CharStreams.nullWriter(),
state.context.get(JavaFileManager.class),
diagnosticListener,
extraOptions,
arguments.getClassNames(),
fileObjects,
context);
FixCompiler fixCompiler;
try {
newTask.analyze();
fixCompiler = FixCompiler.create(fix, state);
} catch (IOException e) {
throw new UncheckedIOException(e);
return false;
}

Result compilationResult = fixCompiler.compile(extraOptions);
URI modifiedFileUri = FixCompiler.getModifiedFileUri(state);

// If we reached the maximum number of diagnostics of a given kind without finding one in the
// modified compilation unit, we won't find any more diagnostics, but we can't be sure that
// there isn't an diagnostic, as the diagnostic may simply be the (max+1)-th diagnostic, and
Expand All @@ -1303,7 +1246,7 @@ public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOExcept
int countWarnings = 0;
boolean warningIsError = false;
boolean warningInSameCompilationUnit = false;
for (Diagnostic<? extends JavaFileObject> diagnostic : diagnosticListener.getDiagnostics()) {
for (Diagnostic<? extends JavaFileObject> diagnostic : compilationResult.diagnostics()) {
warningIsError |= diagnostic.getCode().equals("compiler.err.warnings.and.werror");
JavaFileObject diagnosticSource = diagnostic.getSource();
// If the source's origin is unknown, assume that new diagnostics are due to a modification.
Expand Down Expand Up @@ -1333,6 +1276,124 @@ public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOExcept
return true;
}

/**
* A class to hold the files from the compilation context, with a diff applied to the
* currently-processed one; the files can then be recompiled.
*/
public static final class FixCompiler {
private final List<JavaFileObject> fileObjects;
private final VisitorState state;
private final BasicJavacTask javacTask;

private FixCompiler(
List<JavaFileObject> fileObjects, VisitorState state, BasicJavacTask javacTask) {
this.fileObjects = fileObjects;
this.state = state;
this.javacTask = javacTask;
}

public Result compile(ImmutableList<String> extraOptions) {
DiagnosticCollector<JavaFileObject> diagnosticListener = new DiagnosticCollector<>();
Context context = createContext();
Arguments arguments = Arguments.instance(javacTask.getContext());
JavacTask newTask =
JavacTool.create()
.getTask(
CharStreams.nullWriter(),
state.context.get(JavaFileManager.class),
diagnosticListener,
extraOptions,
arguments.getClassNames(),
fileObjects,
context);
try {
newTask.analyze();
} catch (IOException e) {
throw new UncheckedIOException(e);
}
return Result.create(diagnosticListener.getDiagnostics());
}

private Context createContext() {
Context context = new Context();
Options options = Options.instance(context);
Options originalOptions = Options.instance(javacTask.getContext());
for (String key : originalOptions.keySet()) {
String value = originalOptions.get(key);
if (key.equals("-Xplugin:") && value.startsWith("ErrorProne")) {
// When using the -Xplugin Error Prone integration, disable Error Prone for speculative
// recompiles to avoid infinite recursion.
continue;
}
if (SOURCE_TARGET_OPTIONS.contains(key) && originalOptions.isSet("--release")) {
// javac does not allow -source and -target to be specified explicitly when --release is,
// but does add them in response to passing --release. Here we invert that operation.
continue;
}
options.put(key, value);
}
return context;
}

public static URI getModifiedFileUri(VisitorState state) {
JCCompilationUnit compilationUnit = (JCCompilationUnit) state.getPath().getCompilationUnit();
JavaFileObject modifiedFile = compilationUnit.getSourceFile();
return modifiedFile.toUri();
}

public static FixCompiler create(Fix fix, VisitorState state) throws IOException {
BasicJavacTask javacTask = (BasicJavacTask) state.context.get(JavacTask.class);
if (javacTask == null) {
throw new IllegalArgumentException("No JavacTask in context.");
}
Arguments arguments = Arguments.instance(javacTask.getContext());
ArrayList<JavaFileObject> fileObjects = new ArrayList<>(arguments.getFileObjects());
applyFix(fix, state, fileObjects);
return new FixCompiler(fileObjects, state, javacTask);
}

private static void applyFix(Fix fix, VisitorState state, ArrayList<JavaFileObject> fileObjects)
throws IOException {

JCCompilationUnit compilationUnit = (JCCompilationUnit) state.getPath().getCompilationUnit();
JavaFileObject modifiedFile = compilationUnit.getSourceFile();
CharSequence modifiedFileContent =
modifiedFile.getCharContent(/* ignoreEncodingErrors= */ false);

URI modifiedFileUri = getModifiedFileUri(state);
IntStream.range(0, fileObjects.size())
.filter(i -> fileObjects.get(i).toUri().equals(modifiedFileUri))
.findFirst()
.ifPresent(
i -> {
DescriptionBasedDiff diff =
DescriptionBasedDiff.create(
compilationUnit, ImportOrganizer.STATIC_FIRST_ORGANIZER);
diff.handleFix(fix);
SourceFile fixSource = new SourceFile(modifiedFile.getName(), modifiedFileContent);
diff.applyDifferences(fixSource);
fileObjects.set(
i,
new SimpleJavaFileObject(sourceURI(modifiedFile.toUri()), Kind.SOURCE) {
@Override
public CharSequence getCharContent(boolean ignoreEncodingErrors) {
return fixSource.getAsSequence();
}
});
});
}

/** The result of the compilation. */
@AutoValue
public abstract static class Result {
public abstract List<Diagnostic<? extends JavaFileObject>> diagnostics();

private static Result create(List<Diagnostic<? extends JavaFileObject>> diagnostics) {
return new AutoValue_SuggestedFixes_FixCompiler_Result(diagnostics);
}
}
}

private static final ImmutableSet<String> SOURCE_TARGET_OPTIONS =
ImmutableSet.of("-source", "--source", "-target", "--target");

Expand Down