diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java index 50dac40bcee..8232d543fc3 100644 --- a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java +++ b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java @@ -56,6 +56,8 @@ import com.sun.tools.javac.code.Type.TypeVar; import com.sun.tools.javac.code.TypeTag; import com.sun.tools.javac.code.Types; +import com.sun.tools.javac.comp.Enter; +import com.sun.tools.javac.comp.Resolve; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree.JCClassDecl; import com.sun.tools.javac.tree.JCTree.JCFieldAccess; @@ -67,6 +69,8 @@ import com.sun.tools.javac.tree.JCTree.JCPackageDecl; import com.sun.tools.javac.tree.JCTree.JCVariableDecl; import com.sun.tools.javac.util.Filter; +import com.sun.tools.javac.util.Log; +import com.sun.tools.javac.util.Log.DeferredDiagnosticHandler; import com.sun.tools.javac.util.Name; import java.io.IOException; import java.lang.annotation.Annotation; @@ -857,4 +861,57 @@ public static String getFileNameFromUri(URI uri) { return null; } } + + /** + * Given a Type ({@code base}), find the method named {@code name}, with the appropriate {@code + * argTypes} and {@code tyargTypes} and return its MethodSymbol. + * + *

Ex: + * + *

{@code
+   * .....
+   * class A {}
+   * class B {
+   *   public int hashCode() { return 42; }
+   * }
+   * .....
+   *
+   * MethodSymbol meth =  ASTHelpers.resolveExistingMethod(
+   *    state,
+   *    symbol,
+   *    state.getName("hashCode"),
+   *    ImmutableList.of(),
+   *    ImmutableList.of());
+   * 
+ * + * {@code meth} could be different MethodSymbol's depending on whether {@code symbol} + * represented {@code B} or {@code A}. (B's hashCode method or Object#hashCode). + * + *

Do NOT call this method unless the method you're looking for is guaranteed to exist. A fatal + * error will result otherwise. + * + * @return a MethodSymbol representing the method symbol resolved from the context of this type + */ + public static MethodSymbol resolveExistingMethod( + VisitorState state, + TypeSymbol base, + Name name, + Iterable argTypes, + Iterable tyargTypes) { + Resolve resolve = Resolve.instance(state.context); + Enter enter = Enter.instance(state.context); + Log log = Log.instance(state.context); + DeferredDiagnosticHandler handler = new DeferredDiagnosticHandler(log); + try { + return resolve.resolveInternalMethod( + /*pos*/ null, + enter.getEnv(base), + base.type, + name, + com.sun.tools.javac.util.List.from(argTypes), + com.sun.tools.javac.util.List.from(tyargTypes)); + } finally { + log.popDiagnosticHandler(handler); + } + } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsHashCode.java b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsHashCode.java index bb705ab9acc..c729508682f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsHashCode.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsHashCode.java @@ -42,11 +42,6 @@ import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.TypeSymbol; import com.sun.tools.javac.code.Type; -import com.sun.tools.javac.comp.Enter; -import com.sun.tools.javac.comp.Resolve; -import com.sun.tools.javac.util.Log; -import com.sun.tools.javac.util.Log.DeferredDiagnosticHandler; -import com.sun.tools.javac.util.Name; import javax.lang.model.element.ElementKind; /** @@ -92,7 +87,7 @@ public Description matchClass(ClassTree classTree, VisitorState state) { } MethodSymbol hashCodeSym = - resolveMethod( + ASTHelpers.resolveExistingMethod( state, symbol, state.getName("hashCode"), @@ -105,26 +100,4 @@ public Description matchClass(ClassTree classTree, VisitorState state) { return Description.NO_MATCH; } - public static MethodSymbol resolveMethod( - VisitorState state, - TypeSymbol base, - Name name, - Iterable argTypes, - Iterable tyargTypes) { - Resolve resolve = Resolve.instance(state.context); - Enter enter = Enter.instance(state.context); - Log log = Log.instance(state.context); - DeferredDiagnosticHandler handler = new DeferredDiagnosticHandler(log); - try { - return resolve.resolveInternalMethod( - /*pos*/ null, - enter.getEnv(base), - base.type, - name, - com.sun.tools.javac.util.List.from(argTypes), - com.sun.tools.javac.util.List.from(tyargTypes)); - } finally { - log.popDiagnosticHandler(handler); - } - } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java b/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java new file mode 100644 index 00000000000..7f70e35e176 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java @@ -0,0 +1,100 @@ +/* + * Copyright 2016 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.BugPattern.Category.JDK; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.isSubtypeOf; +import static com.google.errorprone.matchers.Matchers.methodHasArity; +import static com.google.errorprone.matchers.Matchers.methodIsNamed; +import static com.google.errorprone.matchers.Matchers.methodReturns; +import static com.google.errorprone.suppliers.Suppliers.INT_TYPE; + +import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.MethodTree; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.TypeSymbol; +import com.sun.tools.javac.code.Type; +import java.io.InputStream; +import javax.lang.model.element.ElementKind; + +/** Checks that InputStreams should override int read(byte[], int, int); */ +@BugPattern( + name = "InputStreamSlowMultibyteRead", + summary = + "Please also override int read(byte[], int, int), otherwise multi-byte reads from this " + + "input stream are likely to be slow.", + category = JDK, + severity = WARNING +) +public class InputStreamSlowMultibyteRead extends BugChecker implements ClassTreeMatcher { + + private static final Matcher IS_INPUT_STREAM = isSubtypeOf(InputStream.class); + + private static final Matcher READ_INT_METHOD = + allOf(methodIsNamed("read"), methodReturns(INT_TYPE), methodHasArity(0)); + + @Override + public Description matchClass(ClassTree classTree, VisitorState state) { + if (!IS_INPUT_STREAM.matches(classTree, state)) { + return Description.NO_MATCH; + } + + TypeSymbol thisClassSymbol = ASTHelpers.getSymbol(classTree); + if (thisClassSymbol.getKind() != ElementKind.CLASS) { + return Description.NO_MATCH; + } + + // Find the method that overrides the single-byte read. It should also override the multibyte + // read. + MethodTree readByteMethod = + classTree + .getMembers() + .stream() + .filter(MethodTree.class::isInstance) + .map(MethodTree.class::cast) + .filter(m -> READ_INT_METHOD.matches(m, state)) + .findFirst() + .orElse(null); + + if (readByteMethod == null) { + return Description.NO_MATCH; + } + + Type byteArrayType = state.getType(state.getSymtab().byteType, true, ImmutableList.of()); + Type intType = state.getSymtab().intType; + MethodSymbol multiByteReadMethod = + ASTHelpers.resolveExistingMethod( + state, + thisClassSymbol, + state.getName("read"), + ImmutableList.of(byteArrayType, intType, intType), + ImmutableList.of()); + + return multiByteReadMethod.owner.equals(thisClassSymbol) + ? Description.NO_MATCH + : describeMatch(readByteMethod); + } +} 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 6e603648a6a..17c82f335f7 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -68,6 +68,7 @@ import com.google.errorprone.bugpatterns.IdentityBinaryExpression; import com.google.errorprone.bugpatterns.IncompatibleModifiersChecker; import com.google.errorprone.bugpatterns.InfiniteRecursion; +import com.google.errorprone.bugpatterns.InputStreamSlowMultibyteRead; import com.google.errorprone.bugpatterns.InsecureCipherMode; import com.google.errorprone.bugpatterns.InvalidPatternSyntax; import com.google.errorprone.bugpatterns.IsInstanceOfClass; @@ -327,6 +328,7 @@ public static ScannerSupplier errorChecks() { GetClassOnEnum.class, IncompatibleModifiersChecker.class, InjectOnConstructorOfAbstractClass.class, + InputStreamSlowMultibyteRead.class, IterableAndIterator.class, JUnit3FloatingPointComparisonWithoutDelta.class, JUnitAmbiguousTestClass.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteReadTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteReadTest.java new file mode 100644 index 00000000000..9bab3c80c98 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteReadTest.java @@ -0,0 +1,78 @@ +/* + * Copyright 2014 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.CompilationTestHelper; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class InputStreamSlowMultibyteReadTest { + + private CompilationTestHelper compilationHelper; + + @Before + public void setUp() { + compilationHelper = + CompilationTestHelper.newInstance(InputStreamSlowMultibyteRead.class, getClass()); + } + + @Test + public void doingItRight() throws Exception { + compilationHelper + .addSourceLines( + "Test.java", + "class Test extends java.io.InputStream {", + " public int read(byte[] b, int a, int c) { return 0; }", + " public int read() { return 0; }", + "}") + .doTest(); + } + + @Test + public void basic() throws Exception { + compilationHelper + .addSourceLines( + "Test.java", + "class Test extends java.io.InputStream {", + " // BUG: Diagnostic contains:", + " public int read() { return 0; }", + "}") + .doTest(); + } + + // Here, the superclass still can't effectively multibyte-read without the underlying + // read() method. + @Test + public void inherited() throws Exception { + compilationHelper + .addSourceLines( + "Super.java", + "abstract class Super extends java.io.InputStream {", + " public int read(byte[] b, int a, int c) { return 0; }", + "}") + .addSourceLines( + "Test.java", + "class Test extends Super {", + " // BUG: Diagnostic contains:", + " public int read() { return 0; }", + "}") + .doTest(); + } +} diff --git a/docs/bugpattern/InputStreamSlowMultibyteRead.md b/docs/bugpattern/InputStreamSlowMultibyteRead.md new file mode 100644 index 00000000000..0eee92edc0a --- /dev/null +++ b/docs/bugpattern/InputStreamSlowMultibyteRead.md @@ -0,0 +1,14 @@ +`java.io.InputStream` defines a single abstract method: `int read()`, which +subclasses implement to return bytes from the logical input stream. + +However, in most circumstances, readers from `InputStreams` use higher-level +methods like `read(byte[], int offset, int length)` to read multiple bytes at a +time into a buffer. The default implementation of this method is to repeatedly +call `read()`. However, most InputStream implementations could do much better if +they can read multiple bytes at once (at the very least, avoiding unneeded +`byte` -> `int` -> `byte` casts that are needed when implementing the read() +method over an underlying `byte` source). + +The class in question implements `int read()` without also overriding `int +read(byte[], int, int)` and will thus be subject to the costs associated with +the default behavior of the multibyte read method.