diff --git a/README.md b/README.md index c957df143..095537c4c 100644 --- a/README.md +++ b/README.md @@ -173,6 +173,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `PreferAssertj`: Prefer AssertJ fluent assertions. - `ThrowError`: Prefer throwing a RuntimeException rather than Error. - `ReverseDnsLookup`: Calling address.getHostName may result in an unexpected DNS lookup. +- `ReadReturnValueIgnored`: The result of a read call must be checked to know if EOF has been reached or the expected number of bytes have been consumed. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReadReturnValueIgnored.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReadReturnValueIgnored.java new file mode 100644 index 000000000..4cbe2f196 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReadReturnValueIgnored.java @@ -0,0 +1,161 @@ +/* + * (c) Copyright 2018 Palantir Technologies 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.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.common.base.Preconditions; +import com.google.common.io.ByteStreams; +import com.google.common.io.CharStreams; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.AbstractReturnValueIgnored; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +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.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; +import java.io.InputStream; +import java.io.RandomAccessFile; +import java.io.Reader; +import java.util.List; +import java.util.Optional; + +@AutoService(BugChecker.class) +@BugPattern( + name = "ReadReturnValueIgnored", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, + severity = BugPattern.SeverityLevel.ERROR, + summary = "The result of a read call must be checked to know if EOF has been reached or the expected number " + + "of bytes have been consumed.") +public final class ReadReturnValueIgnored extends AbstractReturnValueIgnored { + + // MethodMatchers does not support matching arrays + private static final Matcher INPUT_STREAM_BUFFER_READ_MATCHER = Matchers.allOf( + MethodMatchers.instanceMethod() + .onDescendantOf(InputStream.class.getName()) + .named("read"), + Matchers.not( + MethodMatchers.instanceMethod() + .onDescendantOf(InputStream.class.getName()) + .named("read") + .withParameters())); + + private static final Matcher RAF_BUFFER_READ_MATCHER = Matchers.allOf( + MethodMatchers.instanceMethod() + .onDescendantOf(RandomAccessFile.class.getName()) + .named("read"), + Matchers.not( + MethodMatchers.instanceMethod() + .onDescendantOf(RandomAccessFile.class.getName()) + .named("read") + .withParameters())); + + private static final Matcher READER_SKIP_MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(Reader.class.getName()) + .named("skip") + .withParameters(long.class.getName()); + + private static final Matcher INPUT_STREAM_SKIP_MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(InputStream.class.getName()) + .named("skip") + .withParameters(long.class.getName()); + + private static final Matcher RAF_SKIP_MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(RandomAccessFile.class.getName()) + .named("skipBytes") + .withParameters(int.class.getName()); + + private static final Matcher MATCHER = Matchers.anyOf( + MethodMatchers.instanceMethod() + .onDescendantOfAny( + RandomAccessFile.class.getName(), + Reader.class.getName(), + InputStream.class.getName()) + .named("read"), + INPUT_STREAM_SKIP_MATCHER, + RAF_SKIP_MATCHER, + READER_SKIP_MATCHER); + + @Override + public Matcher specializedMatcher() { + return MATCHER; + } + + @Override + public Description describe(MethodInvocationTree methodInvocationTree, VisitorState state) { + Description result = super.describe(methodInvocationTree, state); + if (Description.NO_MATCH.equals(result)) { + return result; + } + if (INPUT_STREAM_BUFFER_READ_MATCHER.matches(methodInvocationTree, state)) { + return buildDescription(methodInvocationTree) + .addFix(replaceWithStatic(methodInvocationTree, state, ByteStreams.class.getName() + ".readFully")) + .build(); + } + if (INPUT_STREAM_SKIP_MATCHER.matches(methodInvocationTree, state)) { + return buildDescription(methodInvocationTree) + .addFix(replaceWithStatic(methodInvocationTree, state, ByteStreams.class.getName() + ".skipFully")) + .build(); + } + if (RAF_BUFFER_READ_MATCHER.matches(methodInvocationTree, state)) { + return buildDescription(methodInvocationTree) + .addFix(SuggestedFixes.renameMethodInvocation(methodInvocationTree, "readFully", state)) + .build(); + } + if (READER_SKIP_MATCHER.matches(methodInvocationTree, state)) { + return buildDescription(methodInvocationTree) + .addFix(replaceWithStatic(methodInvocationTree, state, CharStreams.class.getName() + ".skipFully")) + .build(); + } + return describeMatch(methodInvocationTree); + } + + // The old invocation target is used as the first argument of the new static invocation + private static Optional replaceWithStatic( + MethodInvocationTree tree, VisitorState state, String fullyQualifiedReplacement) { + Tree methodSelect = tree.getMethodSelect(); + if (!(methodSelect instanceof MemberSelectTree)) { + return Optional.empty(); + } + CharSequence sourceCode = state.getSourceCode(); + if (sourceCode == null) { + return Optional.empty(); + } + MemberSelectTree memberSelectTree = (MemberSelectTree) methodSelect; + SuggestedFix.Builder fix = SuggestedFix.builder(); + String qualifiedReference = SuggestedFixes.qualifyType(state, fix, fullyQualifiedReplacement); + CharSequence args = sourceCode.subSequence( + state.getEndPosition(methodSelect) + 1, + state.getEndPosition(lastItem(tree.getArguments()))); + fix.replace(tree, qualifiedReference + '(' + + state.getSourceForNode(memberSelectTree.getExpression()) + ", " + args + ')'); + return Optional.of(fix.build()); + } + + private static T lastItem(List items) { + Preconditions.checkState(!items.isEmpty(), "List must not be empty"); + return items.get(items.size() - 1); + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ReadReturnValueIgnoredTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ReadReturnValueIgnoredTest.java new file mode 100644 index 000000000..b387e4aea --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ReadReturnValueIgnoredTest.java @@ -0,0 +1,256 @@ +/* + * (c) Copyright 2018 Palantir Technologies 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.palantir.baseline.errorprone; + +import com.google.common.io.ByteStreams; +import com.google.common.io.CharStreams; +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.RandomAccessFile; +import java.io.Reader; +import org.junit.jupiter.api.Test; + +class ReadReturnValueIgnoredTest { + + @Test + void testFix_stream_read() { + fix() + .addInputLines( + "Test.java", + "import " + IOException.class.getName() + ';', + "import " + InputStream.class.getName() + ';', + "class Test {", + " byte[] f(InputStream is) throws IOException {", + " byte[] buf = new byte[4];", + " is.read(buf);", + " return buf;", + " }", + "}") + .addOutputLines( + "Test.java", + "import " + ByteStreams.class.getName() + ';', + "import " + IOException.class.getName() + ';', + "import " + InputStream.class.getName() + ';', + "class Test {", + " byte[] f(InputStream is) throws IOException {", + " byte[] buf = new byte[4];", + " ByteStreams.readFully(is, buf);", + " return buf;", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_stream_skip() { + fix() + .addInputLines( + "Test.java", + "import " + IOException.class.getName() + ';', + "import " + InputStream.class.getName() + ';', + "class Test {", + " void f(InputStream is) throws IOException {", + " is.skip(4);", + " }", + "}") + .addOutputLines( + "Test.java", + "import " + ByteStreams.class.getName() + ';', + "import " + IOException.class.getName() + ';', + "import " + InputStream.class.getName() + ';', + "class Test {", + " void f(InputStream is) throws IOException {", + " ByteStreams.skipFully(is, 4);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_stream_complexAccessor() { + fix() + .addInputLines( + "Test.java", + "import " + ByteArrayInputStream.class.getName() + ';', + "import " + IOException.class.getName() + ';', + "import " + InputStream.class.getName() + ';', + "class Test {", + " byte[] f() throws IOException {", + " byte[] buf = new byte[4];", + " getStream().read(buf);", + " return buf;", + " }", + " InputStream getStream() {", + " return new ByteArrayInputStream(new byte[0]);", + " }", + "}") + .addOutputLines( + "Test.java", + "import " + ByteStreams.class.getName() + ';', + "import " + ByteArrayInputStream.class.getName() + ';', + "import " + IOException.class.getName() + ';', + "import " + InputStream.class.getName() + ';', + "class Test {", + " byte[] f() throws IOException {", + " byte[] buf = new byte[4];", + " ByteStreams.readFully(getStream(), buf);", + " return buf;", + " }", + " InputStream getStream() {", + " return new ByteArrayInputStream(new byte[0]);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_randomAccessFile() { + fix() + .addInputLines( + "Test.java", + "import " + IOException.class.getName() + ';', + "import " + RandomAccessFile.class.getName() + ';', + "class Test {", + " byte[] f(RandomAccessFile raf) throws IOException {", + " byte[] buf = new byte[4];", + " raf.read(buf);", + " return buf;", + " }", + "}") + .addOutputLines( + "Test.java", + "import " + IOException.class.getName() + ';', + "import " + RandomAccessFile.class.getName() + ';', + "class Test {", + " byte[] f(RandomAccessFile raf) throws IOException {", + " byte[] buf = new byte[4];", + " raf.readFully(buf);", + " return buf;", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_reader_skip() { + fix() + .addInputLines( + "Test.java", + "import " + IOException.class.getName() + ';', + "import " + Reader.class.getName() + ';', + "class Test {", + " void f(Reader reader) throws IOException {", + " reader.skip(4);", + " }", + "}") + .addOutputLines( + "Test.java", + "import " + CharStreams.class.getName() + ';', + "import " + IOException.class.getName() + ';', + "import " + Reader.class.getName() + ';', + "class Test {", + " void f(Reader reader) throws IOException {", + " CharStreams.skipFully(reader, 4);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testStream_singleByte() { + helper().addSourceLines( + "Test.java", + "import " + IOException.class.getName() + ';', + "import " + InputStream.class.getName() + ';', + "class Test {", + " int getSecondByte(InputStream is) throws IOException {", + " // BUG: Diagnostic contains: The result of a read call must be checked", + " is.read();", + " return is.read();", + " }", + "}").doTest(); + } + + @Test + void testRandomAccessFile_singleByte() { + helper().addSourceLines( + "Test.java", + "import " + IOException.class.getName() + ';', + "import " + RandomAccessFile.class.getName() + ';', + "class Test {", + " int getSecondByte(RandomAccessFile raf) throws IOException {", + " // BUG: Diagnostic contains: The result of a read call must be checked", + " raf.read();", + " return raf.read();", + " }", + "}").doTest(); + } + + @Test + void testReader_single() { + helper().addSourceLines( + "Test.java", + "import " + IOException.class.getName() + ';', + "import " + Reader.class.getName() + ';', + "class Test {", + " int getSecondChar(Reader reader) throws IOException {", + " // BUG: Diagnostic contains: The result of a read call must be checked", + " reader.read();", + " return reader.read();", + " }", + "}").doTest(); + } + + @Test + void testRandomAccessFile_skipBytes() { + helper().addSourceLines( + "Test.java", + "import " + IOException.class.getName() + ';', + "import " + RandomAccessFile.class.getName() + ';', + "class Test {", + " void getSecondByte(RandomAccessFile raf) throws IOException {", + " // BUG: Diagnostic contains: The result of a read call must be checked", + " raf.skipBytes(3);", + " }", + "}").doTest(); + } + + @Test + void testNegative() { + helper().addSourceLines( + "Test.java", + "import " + IOException.class.getName() + ';', + "import " + InputStream.class.getName() + ';', + "class Test {", + " int f(InputStream is) throws IOException {", + " byte[] buf = new byte[4];", + " return is.read(buf);", + " }", + "}").doTest(); + } + + private BugCheckerRefactoringTestHelper fix() { + return BugCheckerRefactoringTestHelper.newInstance(new ReadReturnValueIgnored(), getClass()); + } + + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(ReadReturnValueIgnored.class, getClass()); + } +} diff --git a/changelog/@unreleased/pr-978.v2.yml b/changelog/@unreleased/pr-978.v2.yml new file mode 100644 index 000000000..b22870e5d --- /dev/null +++ b/changelog/@unreleased/pr-978.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: 'ReadReturnValueIgnored: Check that read operation results are not + ignored' + links: + - https://github.com/palantir/gradle-baseline/pull/978 diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index 1530dbb1b..c66be78f5 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -32,6 +32,7 @@ public class BaselineErrorProneExtension { "PreferListsPartition", "PreferSafeLoggableExceptions", "PreferSafeLoggingPreconditions", + "ReadReturnValueIgnored", "Slf4jLevelCheck", "StrictUnusedVariable", "StringBuilderConstantParameters",