From 16a064c618b970bf6111959c55f58f64edef0896 Mon Sep 17 00:00:00 2001 From: cushon Date: Tue, 22 Aug 2017 10:56:40 -0700 Subject: [PATCH 1/6] Fix a crash in IncrementInForLoopAndHeader Fixes #711 RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=166083196 --- README.md | 44 -------------------------------------------- 1 file changed, 44 deletions(-) delete mode 100644 README.md diff --git a/README.md b/README.md deleted file mode 100644 index b1948b07da5..00000000000 --- a/README.md +++ /dev/null @@ -1,44 +0,0 @@ -# Error Prone - -Error Prone is a static analysis tool for Java that catches common programming -mistakes at compile-time. - -```java -public class ShortSet { - public static void main (String[] args) { - Set s = new HashSet<>(); - for (short i = 0; i < 100; i++) { - s.add(i); - s.remove(i - 1); - } - System.out.println(s.size()); - } -} -``` - -``` -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) -1 error -``` - -## Getting Started - -Our documentation is at [errorprone.info](http://errorprone.info). - -Error Prone works with [Bazel](http://bazel.io), [Maven](http://maven.apache.org), [Ant](http://ant.apache.org), and [Gradle](http://gradle.org). See our [installation instructions](http://errorprone.info/docs/installation) for details. - -## Developing Error Prone - -Developing and building Error Prone is documented on the [wiki](https://github.com/google/error-prone/wiki/For-Developers). - -## Links - -- Mailing lists - - [General discussion](https://groups.google.com/forum/#!forum/error-prone-discuss) - - [Announcements](https://groups.google.com/forum/#!forum/error-prone-announce) -- [Javadoc](http://errorprone.info/api/latest/) -- Pre-release snapshots are available from [Sonatype's snapshot repository](https://oss.sonatype.org/content/repositories/snapshots/com/google/errorprone/). From bb76200082ed7db78029fc376b420fd135efbf6b Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 1 Sep 2017 08:05:54 -0700 Subject: [PATCH 2/6] Add license info to all pom.xml files This enables tools like the Gradle License Plugin (https://github.com/jaredsburrows/gradle-license-plugin) to work Fixes #721 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=167276031 --- annotation/pom.xml | 7 +++++++ annotations/pom.xml | 7 +++++++ ant/pom.xml | 8 ++++++++ check_api/pom.xml | 7 +++++++ core/pom.xml | 7 +++++++ docgen/pom.xml | 7 +++++++ docgen_processor/pom.xml | 7 +++++++ refaster/pom.xml | 8 ++++++++ test_helpers/pom.xml | 7 +++++++ 9 files changed, 65 insertions(+) diff --git a/annotation/pom.xml b/annotation/pom.xml index 824d5902598..7ace8ae16f1 100644 --- a/annotation/pom.xml +++ b/annotation/pom.xml @@ -27,6 +27,13 @@ @BugPattern annotation error_prone_annotation + + + Apache 2.0 + http://www.apache.org/licenses/LICENSE-2.0.txt + + + diff --git a/annotations/pom.xml b/annotations/pom.xml index 784434addb1..a5cc7136afc 100644 --- a/annotations/pom.xml +++ b/annotations/pom.xml @@ -36,6 +36,13 @@ + + + Apache 2.0 + http://www.apache.org/licenses/LICENSE-2.0.txt + + + diff --git a/ant/pom.xml b/ant/pom.xml index bd47327910b..88ad235afe8 100644 --- a/ant/pom.xml +++ b/ant/pom.xml @@ -25,6 +25,14 @@ Ant build support error_prone_ant + + + + Apache 2.0 + http://www.apache.org/licenses/LICENSE-2.0.txt + + + org.apache.ant diff --git a/check_api/pom.xml b/check_api/pom.xml index 4f8aa99df67..589c5acc411 100644 --- a/check_api/pom.xml +++ b/check_api/pom.xml @@ -37,6 +37,13 @@ error-prone check api error_prone_check_api + + + Apache 2.0 + http://www.apache.org/licenses/LICENSE-2.0.txt + + + diff --git a/core/pom.xml b/core/pom.xml index 4553a3ac80a..8cea25dce0c 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -37,6 +37,13 @@ error-prone library error_prone_core + + + Apache 2.0 + http://www.apache.org/licenses/LICENSE-2.0.txt + + + diff --git a/test_helpers/pom.xml b/test_helpers/pom.xml index 0351014069c..aaf684508f6 100644 --- a/test_helpers/pom.xml +++ b/test_helpers/pom.xml @@ -37,6 +37,13 @@ error-prone test helpers error_prone_test_helpers + + + Apache 2.0 + http://www.apache.org/licenses/LICENSE-2.0.txt + + + From 60a13712507b38740d231075935c0629e824e306 Mon Sep 17 00:00:00 2001 From: cushon Date: Fri, 1 Sep 2017 12:15:55 -0700 Subject: [PATCH 3/6] Add a check for /* name= */-style parameter comments This is a simplified subset of NamedParameterChecker. Only handling the recommended comment style means associating comments with arguments is more precise, simpler, and faster, so this could potentially be enabled as an error. RELNOTES: Add a check to ensure /* name= */-style parameter comments match the corresponding formal parameter name ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=167308354 --- .../errorprone/bugpatterns/ParameterName.java | 161 ++++++++++++++++++ .../NamedParameterComment.java | 4 +- .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/ParameterNameTest.java | 70 ++++++++ docs/bugpattern/ParameterName.md | 19 +++ 5 files changed, 254 insertions(+), 2 deletions(-) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/ParameterName.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/ParameterNameTest.java create mode 100644 docs/bugpattern/ParameterName.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ParameterName.java b/core/src/main/java/com/google/errorprone/bugpatterns/ParameterName.java new file mode 100644 index 00000000000..bd6125bef37 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ParameterName.java @@ -0,0 +1,161 @@ +/* + * 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.bugpatterns; + +import static com.google.common.collect.Iterables.getLast; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.sun.tools.javac.parser.Tokens.Comment.CommentStyle.BLOCK; + +import com.google.common.collect.Range; +import com.google.common.collect.Streams; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; +import com.google.errorprone.bugpatterns.argumentselectiondefects.NamedParameterComment; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.Comments; +import com.google.errorprone.util.ErrorProneToken; +import com.google.errorprone.util.ErrorProneTokens; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import com.sun.tools.javac.parser.Tokens.Comment; +import com.sun.tools.javac.tree.JCTree; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Deque; +import java.util.List; +import java.util.function.BiConsumer; +import java.util.function.BiFunction; +import java.util.regex.Matcher; +import java.util.stream.Stream; + +/** @author cushon@google.com (Liam Miller-Cushon) */ +@BugPattern( + name = "ParameterName", + summary = + "Detects `/* name= */`-style comments on actual parameters where the name doesn't match the" + + " formal parameter", + severity = WARNING +) +public class ParameterName extends BugChecker + implements MethodInvocationTreeMatcher, NewClassTreeMatcher { + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + checkArguments(tree, tree.getArguments(), state); + return NO_MATCH; + } + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + checkArguments(tree, tree.getArguments(), state); + return NO_MATCH; + } + + private void checkArguments( + Tree tree, List arguments, VisitorState state) { + if (arguments.isEmpty()) { + return; + } + MethodSymbol sym = (MethodSymbol) ASTHelpers.getSymbol(tree); + if (NamedParameterComment.containsSyntheticParameterName(sym)) { + return; + } + int start = ((JCTree) tree).getStartPosition(); + int end = state.getEndPosition(getLast(arguments)); + Deque tokens = + new ArrayDeque<>( + ErrorProneTokens.getTokens( + state.getSourceCode().subSequence(start, end).toString(), state.context)); + forEachPair( + sym.getParameters().stream(), + arguments.stream(), + (p, a) -> { + while (!tokens.isEmpty() + && ((start + tokens.peekFirst().pos()) < ((JCTree) a).getStartPosition())) { + tokens.removeFirst(); + } + if (tokens.isEmpty()) { + return; + } + Range argRange = + Range.closedOpen(((JCTree) a).getStartPosition(), state.getEndPosition(a)); + if (!argRange.contains(start + tokens.peekFirst().pos())) { + return; + } + checkArgument(p, a, start, tokens.removeFirst(), state); + }); + } + + private void checkArgument( + VarSymbol formal, + ExpressionTree actual, + int start, + ErrorProneToken token, + VisitorState state) { + List matches = new ArrayList<>(); + for (Comment comment : token.comments()) { + if (comment.getStyle() != BLOCK) { + continue; + } + Matcher m = + NamedParameterComment.PARAMETER_COMMENT_PATTERN.matcher( + Comments.getTextFromComment(comment)); + if (!m.matches()) { + continue; + } + String name = m.group(1); + if (formal.getSimpleName().contentEquals(name)) { + // If there are multiple comments, bail if any one of them is an exact match. + return; + } + matches.add(comment); + } + for (Comment match : matches) { + state.reportMatch( + buildDescription(actual) + .setMessage( + String.format( + "%s does not match parameter name '%s'", + match.getText(), formal.getSimpleName())) + .addFix( + SuggestedFix.replace( + start + match.getSourcePos(0), + start + match.getSourcePos(match.getText().length() - 1) + 1, + String.format("/* %s= */", formal.getSimpleName()))) + .build()); + } + } + + // TODO(cushon): use Streams.forEach when guava 22 is available + static void forEachPair(Stream xs, Stream bx, BiConsumer c) { + BiFunction f = + (a, b) -> { + c.accept(a, b); + return null; + }; + long unused = Streams.zip(xs, bx, f).count(); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterComment.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterComment.java index dab6d58acf9..eba16f682b6 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterComment.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NamedParameterComment.java @@ -38,9 +38,9 @@ *

We look for a NamedParameterComment: this is the last block comment before the argument * which ends with an equals sign. */ -final class NamedParameterComment { +public final class NamedParameterComment { - private static final Pattern PARAMETER_COMMENT_PATTERN = + public static final Pattern PARAMETER_COMMENT_PATTERN = Pattern.compile("\\s*([\\w\\d_]+)\\s*=\\s*"); private static final String PARAMETER_COMMENT_MARKER = "="; diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index fcb97e9f9a9..19f5263aa72 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -149,6 +149,7 @@ import com.google.errorprone.bugpatterns.PackageInfo; import com.google.errorprone.bugpatterns.PackageLocation; import com.google.errorprone.bugpatterns.ParameterComment; +import com.google.errorprone.bugpatterns.ParameterName; import com.google.errorprone.bugpatterns.PreconditionsCheckNotNull; import com.google.errorprone.bugpatterns.PreconditionsCheckNotNullPrimitive; import com.google.errorprone.bugpatterns.PreconditionsInvalidPlaceholder; @@ -465,6 +466,7 @@ public static ScannerSupplier errorChecks() { OverridesGuiceInjectableMethod.class, OverrideThrowableToString.class, OvershadowingSubclassFields.class, + ParameterName.class, PreconditionsInvalidPlaceholder.class, ProtoFieldPreconditionsCheckNotNull.class, ReferenceEquality.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ParameterNameTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ParameterNameTest.java new file mode 100644 index 00000000000..19942c7368f --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ParameterNameTest.java @@ -0,0 +1,70 @@ +/* + * 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.bugpatterns; + +import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import java.io.IOException; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** {@link ParameterName}Test */ +@RunWith(JUnit4.class) +public class ParameterNameTest { + + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(ParameterName.class, getClass()); + + @Test + public void positive() throws IOException { + BugCheckerRefactoringTestHelper.newInstance(new ParameterName(), getClass()) + .addInputLines( + "in/Test.java", + "class Test {", + " void f(int foo, int bar) {}", + " {", + " f(/* bar= */ 1, /* foo= */ 2);", + " }", + "}") + .addOutputLines( + "out/Test.java", + "class Test {", + " void f(int foo, int bar) {}", + " {", + " f(/* foo= */ 1, /* bar= */ 2);", + " }", + "}") + .doTest(TEXT_MATCH); + } + + @Test + public void negative() { + testHelper + .addSourceLines( + "Test.java", + "class Test {", + " void f(int foo, int bar) {}", + " {", + " f(/* foo= */ 1, 2);", + " }", + "}") + .doTest(); + } +} diff --git a/docs/bugpattern/ParameterName.md b/docs/bugpattern/ParameterName.md new file mode 100644 index 00000000000..4541afd5fc4 --- /dev/null +++ b/docs/bugpattern/ParameterName.md @@ -0,0 +1,19 @@ +In certain contexts literal arguments - such as `0`, `""`, `true` and `false`, +or `null` - can make it difficult for readers to know what a method will do. +Defining methods that take boolean parameters or otherwise expect users to pass +in ambiguous literals is generally discouraged. However, when you must call such +a method you're encouraged to use the parameter name as an inline comment at the +call site, so that readers don't need to look at the method declaration to +understand the parameter's purpose. + +Error Prone recognizes such comments that use the following formatting, and +emits an error if the comment doesn't match the name of the corresponding formal +parameter: + +```java +booleanMethod(/* enableFoo= */ true); +``` + +If the comment deliberately does not match the formal parameter name, using a +regular block comment without the `=` is recommended: `/* enableFoo */`. + From adbe798ee0de8a4d1612fbed9fbc64618f0f0a3f Mon Sep 17 00:00:00 2001 From: epmjohnston Date: Fri, 1 Sep 2017 16:37:22 -0700 Subject: [PATCH 4/6] Create ProvidesFix BugChecker to populate and enforce new ProvidesFix field of @BugPattern annotation. RELNOTES: Create ProvidesFixChecker ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=167340492 --- .../bugpatterns/ProvidesFixChecker.java | 133 ++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/ProvidesFixCheckerTest.java | 198 ++++++++++++++++++ 3 files changed, 333 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/ProvidesFixChecker.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/ProvidesFixCheckerTest.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ProvidesFixChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/ProvidesFixChecker.java new file mode 100644 index 00000000000..15f84f25fe4 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ProvidesFixChecker.java @@ -0,0 +1,133 @@ +/* + * 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.bugpatterns; + +import static com.google.common.base.MoreObjects.firstNonNull; +import static com.google.errorprone.BugPattern.ProvidesFix.NO_FIX; +import static com.google.errorprone.BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.util.ASTHelpers.getSymbol; + +import com.google.common.collect.Iterables; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.AnnotationMatcherUtils; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.source.util.TreeScanner; +import java.util.regex.Pattern; + +@BugPattern( + name = "ProvidesFix", + summary = "BugChecker has incorrect ProvidesFix tag, please update", + severity = WARNING, + providesFix = REQUIRES_HUMAN_ATTENTION +) +public class ProvidesFixChecker extends BugChecker implements ClassTreeMatcher { + + private static final Matcher IS_BUGCHECKER = + Matchers.isSubtypeOf("com.google.errorprone.bugpatterns.BugChecker"); + private static final Matcher DESCRIPTION_WITH_FIX = + anyOf( + MethodMatchers.instanceMethod() + .onDescendantOf("com.google.errorprone.matchers.Description.Builder") + .withNameMatching(Pattern.compile("addFix|addAllFixes")), + MethodMatchers.instanceMethod() + .onDescendantOf("com.google.errorprone.bugpatterns.BugChecker") + .named("describeMatch") + .withParameters("com.sun.source.tree.Tree", "com.google.errorprone.fixes.Fix")); + private static final Matcher DESCRIPTION_CONSTRUCTOR = + Matchers.constructor() + .forClass("com.google.errorprone.matchers.Description") + .withParameters( + "com.sun.source.tree.Tree", + "java.lang.String", + "com.google.errorprone.fixes.Fix", + "com.google.errorprone.BugPattern.SeverityLevel"); + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + if (!IS_BUGCHECKER.matches(tree, state)) { + return NO_MATCH; + } + + AnnotationTree bugPatternAnnotation = + ASTHelpers.getAnnotationWithSimpleName(tree.getModifiers().getAnnotations(), "BugPattern"); + if (bugPatternAnnotation == null) { + return NO_MATCH; + } + + if (!providesFix(tree, state)) { + // N.B. BugCheckers can provide a fix in ways we cannot detect. + // Ignore checkers that don't appear to have a fix, but are labeled as having one. + return NO_MATCH; + } + + SuggestedFix.Builder fixBuilder = + SuggestedFix.builder().addImport("com.google.errorprone.BugPattern.ProvidesFix"); + ExpressionTree providesFixArgument = + AnnotationMatcherUtils.getArgument(bugPatternAnnotation, "providesFix"); + if (providesFixArgument == null) { + // If providesFix argument not already present, add it to the end of @BugPattern args. + fixBuilder.postfixWith( + Iterables.getLast(bugPatternAnnotation.getArguments()), + ", providesFix = ProvidesFix." + REQUIRES_HUMAN_ATTENTION.name()); + } else { + if (!getSymbol(providesFixArgument).getSimpleName().contentEquals(NO_FIX.name())) { + // If providesFix arg already states there is a fix, we're good. + return NO_MATCH; + } + // If providesFix arg incorrectly states there is no fix, correct it. + fixBuilder.replace(providesFixArgument, "ProvidesFix." + REQUIRES_HUMAN_ATTENTION.name()); + } + return describeMatch(tree, fixBuilder.build()); + } + + private static boolean providesFix(Tree tree, VisitorState state) { + return tree.accept( + new TreeScanner() { + @Override + public Boolean visitMethodInvocation(MethodInvocationTree callTree, Void unused) { + return DESCRIPTION_WITH_FIX.matches(callTree, state); + } + + @Override + public Boolean visitNewClass(NewClassTree constructorTree, Void unused) { + return DESCRIPTION_CONSTRUCTOR.matches(constructorTree, state); + } + + @Override + public Boolean reduce(Boolean m1, Boolean m2) { + return firstNonNull(m1, false) || firstNonNull(m2, false); + } + }, + null); + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 19f5263aa72..7925bd7f4bc 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -160,6 +160,7 @@ import com.google.errorprone.bugpatterns.ProtoFieldPreconditionsCheckNotNull; import com.google.errorprone.bugpatterns.ProtoStringFieldReferenceEquality; import com.google.errorprone.bugpatterns.ProtocolBufferOrdinal; +import com.google.errorprone.bugpatterns.ProvidesFixChecker; import com.google.errorprone.bugpatterns.RandomModInteger; import com.google.errorprone.bugpatterns.RedundantThrows; import com.google.errorprone.bugpatterns.ReferenceEquality; @@ -535,6 +536,7 @@ public static ScannerSupplier errorChecks() { PrivateConstructorForUtilityClass.class, PrivateConstructorForNoninstantiableModule.class, ProtoStringFieldReferenceEquality.class, + ProvidesFixChecker.class, QualifierOrScopeOnInjectMethod.class, QualifierWithTypeUse.class, RedundantThrows.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ProvidesFixCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ProvidesFixCheckerTest.java new file mode 100644 index 00000000000..1d70e98271a --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ProvidesFixCheckerTest.java @@ -0,0 +1,198 @@ +/* + * 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.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class ProvidesFixCheckerTest { + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(new ProvidesFixChecker(), getClass()); + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(ProvidesFixChecker.class, getClass()); + + @Test + public void noFixes() throws Exception { + testHelper + .addSourceLines( + "ExampleChecker.java", + "import com.google.errorprone.BugPattern;", + "import com.google.errorprone.BugPattern.SeverityLevel;", + "import com.google.errorprone.VisitorState;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;", + "import com.google.errorprone.matchers.Description;", + "import com.sun.source.tree.ClassTree;", + "@BugPattern(name = \"Example\", summary = \"\", severity = SeverityLevel.ERROR)", + "public class ExampleChecker extends BugChecker implements ClassTreeMatcher {", + " @Override public Description matchClass(ClassTree t, VisitorState s) {", + " return Description.NO_MATCH;", + " }", + "}") + .doTest(); + } + + @Test + public void addsArgument() throws Exception { + refactoringTestHelper + .addInputLines( + "in/ExampleChecker.java", + "import com.google.errorprone.BugPattern;", + "import com.google.errorprone.BugPattern.ProvidesFix;", + "import com.google.errorprone.BugPattern.SeverityLevel;", + "import com.google.errorprone.VisitorState;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;", + "import com.google.errorprone.fixes.SuggestedFix;", + "import com.google.errorprone.matchers.Description;", + "import com.sun.source.tree.ClassTree;", + "@BugPattern(name = \"Example\", summary = \"\", severity = SeverityLevel.ERROR)", + "public class ExampleChecker extends BugChecker implements ClassTreeMatcher {", + " @Override public Description matchClass(ClassTree t, VisitorState s) {", + " return describeMatch(t, SuggestedFix.replace(t, \"goodbye\"));", + " }", + "}") + .addOutputLines( + "out/ExampleChecker.java", + "import com.google.errorprone.BugPattern;", + "import com.google.errorprone.BugPattern.ProvidesFix;", + "import com.google.errorprone.BugPattern.SeverityLevel;", + "import com.google.errorprone.VisitorState;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;", + "import com.google.errorprone.fixes.SuggestedFix;", + "import com.google.errorprone.matchers.Description;", + "import com.sun.source.tree.ClassTree;", + "@BugPattern(", + " name = \"Example\",", + " summary = \"\",", + " severity = SeverityLevel.ERROR,", + " providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION)", + "public class ExampleChecker extends BugChecker implements ClassTreeMatcher {", + " @Override public Description matchClass(ClassTree t, VisitorState s) {", + " return describeMatch(t, SuggestedFix.replace(t, \"goodbye\"));", + " }", + "}") + .doTest(); + } + + @Test + public void replacesExistingInPlace() throws Exception { + refactoringTestHelper + .addInputLines( + "in/ExampleChecker.java", + "import com.google.errorprone.BugPattern;", + "import com.google.errorprone.BugPattern.ProvidesFix;", + "import com.google.errorprone.BugPattern.SeverityLevel;", + "import com.google.errorprone.VisitorState;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;", + "import com.google.errorprone.fixes.SuggestedFix;", + "import com.google.errorprone.matchers.Description;", + "import com.sun.source.tree.ClassTree;", + "@BugPattern(", + " name = \"Example\",", + " summary = \"\",", + " providesFix = ProvidesFix.NO_FIX,", + " severity = SeverityLevel.ERROR)", + "public class ExampleChecker extends BugChecker implements ClassTreeMatcher {", + " @Override public Description matchClass(ClassTree t, VisitorState s) {", + " return describeMatch(t, SuggestedFix.replace(t, \"goodbye\"));", + " }", + "}") + .addOutputLines( + "out/ExampleChecker.java", + "import com.google.errorprone.BugPattern;", + "import com.google.errorprone.BugPattern.ProvidesFix;", + "import com.google.errorprone.BugPattern.SeverityLevel;", + "import com.google.errorprone.VisitorState;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;", + "import com.google.errorprone.fixes.SuggestedFix;", + "import com.google.errorprone.matchers.Description;", + "import com.sun.source.tree.ClassTree;", + "@BugPattern(", + " name = \"Example\",", + " summary = \"\",", + " providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION,", + " severity = SeverityLevel.ERROR)", + "public class ExampleChecker extends BugChecker implements ClassTreeMatcher {", + " @Override public Description matchClass(ClassTree t, VisitorState s) {", + " return describeMatch(t, SuggestedFix.replace(t, \"goodbye\"));", + " }", + "}") + .doTest(); + } + + @Test + public void alreadyHasFixTag() throws Exception { + testHelper + .addSourceLines( + "ExampleChecker.java", + "import com.google.errorprone.BugPattern;", + "import com.google.errorprone.BugPattern.ProvidesFix;", + "import com.google.errorprone.BugPattern.SeverityLevel;", + "import com.google.errorprone.VisitorState;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;", + "import com.google.errorprone.fixes.SuggestedFix;", + "import com.google.errorprone.matchers.Description;", + "import com.sun.source.tree.ClassTree;", + "@BugPattern(", + " name = \"Example\",", + " summary = \"\",", + " severity = SeverityLevel.ERROR,", + " providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION)", + "public class ExampleChecker extends BugChecker implements ClassTreeMatcher {", + " @Override public Description matchClass(ClassTree t, VisitorState s) {", + " return describeMatch(t, SuggestedFix.replace(t, \"goodbye\"));", + " }", + "}") + .doTest(); + } + + @Test + public void erroneousFixTag() throws Exception { + testHelper + .addSourceLines( + "ExampleChecker.java", + "import com.google.errorprone.BugPattern;", + "import com.google.errorprone.BugPattern.ProvidesFix;", + "import com.google.errorprone.BugPattern.SeverityLevel;", + "import com.google.errorprone.VisitorState;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;", + "import com.google.errorprone.fixes.SuggestedFix;", + "import com.google.errorprone.matchers.Description;", + "import com.sun.source.tree.ClassTree;", + "@BugPattern(", + " name = \"Example\",", + " summary = \"\",", + " severity = SeverityLevel.ERROR,", + " providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION)", + "public class ExampleChecker extends BugChecker implements ClassTreeMatcher {", + " @Override public Description matchClass(ClassTree t, VisitorState s) {", + " return Description.NO_MATCH;", + " }", + "}") + .doTest(); + } +} From 8d3584dc4ffb491e9e2bc2716865398a805ff475 Mon Sep 17 00:00:00 2001 From: johnerez Date: Sat, 2 Sep 2017 12:24:48 -0700 Subject: [PATCH 5/6] Add {.good} and {.bad} to error-prone documentation code blocks. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=167388444 --- docs/bugpattern/MutableMethodReturnType.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/bugpattern/MutableMethodReturnType.md b/docs/bugpattern/MutableMethodReturnType.md index 11363a45a47..41ce1a1f26b 100644 --- a/docs/bugpattern/MutableMethodReturnType.md +++ b/docs/bugpattern/MutableMethodReturnType.md @@ -11,7 +11,7 @@ interfaces in every important sense of the word. That is, prefer this: -```java +```java {.good} ImmutableList getCoutries() { return ImmutableList.of("Denmark", "Norway", "Sweden"); } @@ -19,7 +19,7 @@ ImmutableList getCoutries() { to this: -```java +```java {.bad} List getCoutries() { return ImmutableList.of("Denmark", "Norway", "Sweden"); } From 8d570cb2a440c2032029de007cf97c1ff054789e Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 6 Sep 2017 08:50:47 -0700 Subject: [PATCH 6/6] Allow long non-file arguments to be spilled Allow long non-file arguments (e.g. -classpath) to go to a file. This prevents E2BIG when the classpath argument is too long. This can't easily be fixed in upstream ant because: (a) it would require an API change, and (b) there is no way for ant to know which arguments are safe to spill Fixes #671 RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=167726515 --- .../errorprone/ErrorProneExternalCompilerAdapter.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/ant/src/main/java/com/google/errorprone/ErrorProneExternalCompilerAdapter.java b/ant/src/main/java/com/google/errorprone/ErrorProneExternalCompilerAdapter.java index 5ea21e5d6b5..deb6eb22ecc 100644 --- a/ant/src/main/java/com/google/errorprone/ErrorProneExternalCompilerAdapter.java +++ b/ant/src/main/java/com/google/errorprone/ErrorProneExternalCompilerAdapter.java @@ -98,10 +98,14 @@ public boolean execute() throws BuildException { } cmd.createArgument().setValue(ErrorProneCompiler.class.getName()); + // This is the first argument that may be spilled to a file. + // The ant API describes it as the first argument which is a + // file, but in fact only uses it for spilling. Putting the + // break here allows long classpath arguments to be handled. + int firstSpillableArgument = cmd.size(); setupModernJavacCommandlineSwitches(cmd); - int firstFile = cmd.size(); logAndAddFilesToCompile(cmd); - return executeExternalCompile(cmd.getCommandline(), firstFile, true) == 0; + return executeExternalCompile(cmd.getCommandline(), firstSpillableArgument, true) == 0; } else { attributes.log( "You must set fork=\"yes\" to use the external Error Prone compiler", Project.MSG_ERR);