Skip to content

Commit

Permalink
Initial implementation of a com.sun.source.util.Plugin
Browse files Browse the repository at this point in the history
This allows Error Prone to be used as a javac plugin (instead of a
wrapper) via the plugin API that was added in 8:
https://docs.oracle.com/javase/8/docs/jdk/api/javac/tree/com/sun/source/util/Plugin.html

Demo:

```
javac -J-Xbootclasspath/p:javac-9-dev-r3297-3.jar \
  -processorpath error_prone_ant-2.0.18-SNAPSHOT.jar \
  -Xplugin:ErrorProne Test.java
...
ShortSet.java:8: error: [CollectionIncompatibleType] Argument 'i - 1' should not be passed to this method; its type int is not compatible with its collection's type argument Short
      s.remove(i - 1);
              ^
    (see http://errorprone.info/bugpattern/CollectionIncompatibleType)
```

See #535

MOE_MIGRATED_REVID=148835033
  • Loading branch information
cushon committed Mar 1, 2017
1 parent 6de880c commit feb4b34
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.ResourceBundle;
import java.util.Set;
import javax.annotation.Nullable;
import javax.annotation.processing.Processor;
Expand Down Expand Up @@ -293,10 +294,9 @@ private Result wrapPotentialRefactoringCall(
}
}

/**
* Registers our message bundle.
*/
/** Registers our message bundle. */
public static void setupMessageBundle(Context context) {
JavacMessages.instance(context).add("com.google.errorprone.errors");
ResourceBundle bundle = ResourceBundle.getBundle("com.google.errorprone.errors");
JavacMessages.instance(context).add(l -> bundle);
}
}
7 changes: 7 additions & 0 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,13 @@
<version>r6</version>
<scope>test</scope>
</dependency>
<dependency>
<!-- Apache 2.0 -->
<groupId>com.google.auto.service</groupId>
<artifactId>auto-service</artifactId>
<version>1.0-rc2</version>
<scope>provided</scope>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright 2017 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone;

import com.google.auto.service.AutoService;
import com.google.errorprone.scanner.BuiltInCheckerSuppliers;
import com.sun.source.util.JavacTask;
import com.sun.source.util.Plugin;
import com.sun.tools.javac.api.BasicJavacTask;
import com.sun.tools.javac.util.Context;

/** A javac {@link Plugin} that runs Error Prone. */
@AutoService(Plugin.class)
public class ErrorProneJavacPlugin implements Plugin {
@Override
public String getName() {
return "ErrorProne";
}

@Override
public void init(JavacTask javacTask, String... args) {
Context context = ((BasicJavacTask) javacTask).getContext();
BaseErrorProneCompiler.setupMessageBundle(context);
javacTask.addTaskListener(
ErrorProneAnalyzer.createByScanningForPlugins(
BuiltInCheckerSuppliers.defaultChecks(), ErrorProneOptions.processArgs(args), context));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/*
* Copyright 2017 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone;

import static com.google.common.collect.MoreCollectors.onlyElement;
import static com.google.common.truth.Truth.assertThat;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Locale.ENGLISH;

import com.google.common.collect.ImmutableList;
import com.google.common.jimfs.Configuration;
import com.google.common.jimfs.Jimfs;
import com.sun.source.util.JavacTask;
import com.sun.tools.javac.api.JavacTool;
import com.sun.tools.javac.file.JavacFileManager;
import com.sun.tools.javac.util.Context;
import java.io.IOException;
import java.nio.file.FileSystem;
import java.nio.file.Files;
import java.nio.file.Path;
import javax.tools.Diagnostic;
import javax.tools.DiagnosticCollector;
import javax.tools.JavaFileObject;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** {@link ErrorProneJavacPlugin}Test */
@RunWith(JUnit4.class)
public class ErrorProneJavacPluginTest {
@Test
public void hello() throws IOException {
FileSystem fileSystem = Jimfs.newFileSystem(Configuration.unix());
Path source = fileSystem.getPath("Test.java");
Files.write(
source,
ImmutableList.of(
"import java.util.HashSet;",
"import java.util.Set;",
"class Test {",
" public static void main(String[] args) {",
" Set<Short> s = new HashSet<>();",
" for (short i = 0; i < 100; i++) {",
" s.add(i);",
" s.remove(i - 1);",
" }",
" System.out.println(s.size());",
" }",
"}"),
UTF_8);
JavacFileManager fileManager = new JavacFileManager(new Context(), false, UTF_8);
DiagnosticCollector<JavaFileObject> diagnosticCollector = new DiagnosticCollector<>();
JavacTask task =
JavacTool.create()
.getTask(
null,
fileManager,
diagnosticCollector,
ImmutableList.of("-Xplugin:ErrorProne"),
ImmutableList.of(),
fileManager.getJavaFileObjects(source));
assertThat(task.call()).isFalse();
Diagnostic<? extends JavaFileObject> diagnostic =
diagnosticCollector
.getDiagnostics()
.stream()
.filter(d -> d.getKind() == Diagnostic.Kind.ERROR)
.collect(onlyElement());
assertThat(diagnostic.getMessage(ENGLISH)).contains("[CollectionIncompatibleType]");
}
}

12 comments on commit feb4b34

@jbduncan
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh, nice! Is this part of an effort to eventually detach error-prone from the Java compiler @cushon ? :)

@cushon
Copy link
Collaborator Author

@cushon cushon commented on feb4b34 Mar 1, 2017

Choose a reason for hiding this comment

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

Error Prone still depends on the specific javac we repackage, and their are no immediate plan to change that. But if there are better-supported alternatives to using internal APIs we should migrate, and maybe some day that will make it possible to detach.

@kageiit
Copy link

@kageiit kageiit commented on feb4b34 Apr 3, 2017

Choose a reason for hiding this comment

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

@cushon This seems to work with some caveats. Plain old javac does not like any of errorprone's -Xep: options. These are processed by the compiler before creating the JavacTask implementation being passed over to plugins. Any suggestions on how to deal with this situation?

@cushon
Copy link
Collaborator Author

@cushon cushon commented on feb4b34 Apr 3, 2017

Choose a reason for hiding this comment

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

The syntax for passing arguments to Xplugins is e.g.:

javac -Xplugin:ErrorProne "-XepAllErrorsAsWarnings -Xep:CollectionIncompatibleType:ERROR"

(see https://docs.oracle.com/javase/9/tools/javac.htm#JSWOR627)

@kageiit
Copy link

@kageiit kageiit commented on feb4b34 Apr 3, 2017

Choose a reason for hiding this comment

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

Using quotes, I am now seeing

invalid flag: "-Xep:NullablePrimitive:OFF -Xep:DoubleCheckedLocking:OFF...

Am i missing something? The link you sent does not seem to have anything related to plugin args specifically. It just points at the document root and does not point to any section

@kageiit
Copy link

@kageiit kageiit commented on feb4b34 Apr 3, 2017

Choose a reason for hiding this comment

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

@cushon ^ :)

@cushon
Copy link
Collaborator Author

@cushon cushon commented on feb4b34 Apr 3, 2017

Choose a reason for hiding this comment

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

Sorry, I messed up the quoting. The linked documentation has a brief section on -Xplugin that acknowledges it's possible to pass args through to the plugin. This example works for me with JDK 8u152:

javac -J-Xbootclasspath/p:error_prone_ant-2.0.19.jar \
-processorpath error_prone_ant-2.0.19.jar \
'-Xplugin:ErrorProne -XepAllErrorsAsWarnings -Xep:CollectionIncompatibleType:ERROR' \
Test.java

@kageiit
Copy link

@kageiit kageiit commented on feb4b34 Apr 3, 2017

Choose a reason for hiding this comment

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

Ok that seemed to work. But it seems to have some trouble loading custom plugins. Has a stacktrace like

java.util.ServiceConfigurationError: com.google.errorprone.bugpatterns.BugChecker: Provider com.example.CustomChecker not a subtype
	at java.util.ServiceLoader.fail(ServiceLoader.java:239)
	at java.util.ServiceLoader.access$300(ServiceLoader.java:185)
	at java.util.ServiceLoader$LazyIterator.nextService(ServiceLoader.java:376)
	at java.util.ServiceLoader$LazyIterator.access$700(ServiceLoader.java:323)
	at java.util.ServiceLoader$LazyIterator$2.run(ServiceLoader.java:407)
	at java.security.AccessController.doPrivileged(Native Method)

Is this expected?

@cushon
Copy link
Collaborator Author

@cushon cushon commented on feb4b34 Apr 3, 2017

Choose a reason for hiding this comment

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

Nope, can you file a bug with a repro?

@kageiit
Copy link

@kageiit kageiit commented on feb4b34 Apr 3, 2017

Choose a reason for hiding this comment

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

sure will do

@kageiit
Copy link

@kageiit kageiit commented on feb4b34 Apr 3, 2017

Choose a reason for hiding this comment

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

@cushon It seems like error prone javac does not like -J-Xbootclasspath/p. So I did not set it. It could be related. It fails with invalid flag: -J-Xbootclasspath/p: when I try to set it with javac-9-dev-r3297-4.jar

@cushon
Copy link
Collaborator Author

@cushon cushon commented on feb4b34 Apr 5, 2017

Choose a reason for hiding this comment

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

@kageiit if you file a bug with a repro I'll investigate.

-J flags only work with the standard javac launcher. Depending on how you're invoking it you can pass -Xbootclasspath/p directly to the jvm, e.g.:

java -Xbootclasspath/p:error_prone_ant-2.0.19.jar \
com.sun.tools.javac.Main \
-processorpath error_prone_ant-2.0.19.jar \
'-Xplugin:ErrorProne -XepAllErrorsAsWarnings -Xep:CollectionIncompatibleType:ERROR' \
Test.java

Please sign in to comment.