Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
New check for subclasses of InputStream
if a subclasser of InputStream implements int read(), they should also override int read(byte[], int, int), otherwise the performance of the stream is likely to be slow. RELNOTES: New Check: InputStream overrides should also override int read(byte[], int, int) to improve the speed of multibyte reads. MOE_MIGRATED_REVID=137551912
- Loading branch information
1 parent
b030043
commit acea257
Showing
6 changed files
with
252 additions
and
28 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
100 changes: 100 additions & 0 deletions
100
core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<ClassTree> IS_INPUT_STREAM = isSubtypeOf(InputStream.class); | ||
|
|
||
| private static final Matcher<MethodTree> 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); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
78 changes: 78 additions & 0 deletions
78
core/src/test/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteReadTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |