google / google/error-prone

New check request: OutputStream subclasses that inherit the per-byte write(byte[], int, int)

Open
#6,106 0 comments 1 reaction 0 assignees View on GitHub
Dominant language
Java
Stars
7.2k
Forks
820
Avg merge
5h 9m
Merged PRs (30d)
50

Description

I wrapped an `OutputStream` in a `FilterOutputStream` subclass that overrides only `flush()`. Every `write(byte[], off, len)` through it reached the wrapped stream as one `write(int)` call per byte, and Error Prone reported nothing. A direct `OutputStream` subclass that implements `write(int)` only is not reported either. The `InputStream` counterpart, a class that implements `read()` without `read(byte[], int, int)`, is reported by `InputStreamSlowMultibyteRead`.

The JDK documents both inherited implementations as ones to replace. [`OutputStream.write(byte[], int, int)`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/OutputStream.html#write(byte%5B%5D,int,int)) encourages subclasses to override it, and [`FilterOutputStream.write(byte[], int, int)`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/FilterOutputStream.html#write(byte%5B%5D,int,int)) says "Subclasses of FilterOutputStream should provide a more efficient implementation".

### Example

```java
package demo;

import java.io.FilterOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;

public class Demo {
/** A decorator that only suppresses flush(). Not reported. */
static final class NoFlush extends FilterOutputStream {
NoFlush(OutputStream out) {
super(out);
}

@Override
public void flush() {}
}

/** An OutputStream that implements write(int) only. Not reported. */
static final class ByteSink extends OutputStream {
private final OutputStream out;

ByteSink(OutputStream out) {
this.out = out;
}

@Override
public void write(int b) throws IOException {
out.write(b);
}
}

/** The InputStream counterpart. Reported by InputStreamSlowMultibyteRead. */
static final class ByteSource extends InputStream {
private final InputStream in;

ByteSource(InputStream in) {
this.in = in;
}

@Override
public int read() throws IOException {
return in.read();
}
}
}
```

### Actual output, Error Prone 2.50.0

Compiled with Error Prone 2.50.0 (Gradle 9.6.1, `net.ltgt.errorprone` 5.1.1, JDK 21.0.9). The path is shortened, and this is the whole compiler output. `NoFlush` (line 10) and `ByteSink` (line 28) are not reported; the warning on `ByteSource` is the control that shows the check runs, not the problem.

```text
src/main/java/demo/Demo.java:42: warning: [InputStreamSlowMultibyteRead] Please also override int read(byte[], int, int), otherwise multi-byte reads from this input stream are likely to be slow.
public int read() throws IOException {
^
(see https://errorprone.info/bugpattern/InputStreamSlowMultibyteRead)
1 warning
```

### Expected output

The check name and the message below are placeholders; what I am asking for is a warning on line 10 and on line 28, next to the existing one on line 42.

```text
src/main/java/demo/Demo.java:10: warning: [OutputStreamSlowMultibyteWrite]
static final class NoFlush extends FilterOutputStream {
^
src/main/java/demo/Demo.java:28: warning: [OutputStreamSlowMultibyteWrite]
public void write(int b) throws IOException {
^
src/main/java/demo/Demo.java:42: warning: [InputStreamSlowMultibyteRead] Please also override int read(byte[], int, int), otherwise multi-byte reads from this input stream are likely to be slow.
public int read() throws IOException {
^
(see https://errorprone.info/bugpattern/InputStreamSlowMultibyteRead)
3 warnings
```

A `FilterOutputStream` or `OutputStream` subclass that declares `write(byte[], int, int)` stays unreported.

### Where it came up

pgjdbc had the `NoFlush` shape: an anonymous `FilterOutputStream` in `PGStream.getEncodingWriter()` that overrides only `flush()`. An ArchUnit rule found it, and pgjdbc/pgjdbc#4391 fixes it. That is the only case I know of; I have not measured how often the pattern occurs in other code bases.

### One possible shape

The requirement is the two new warnings in the expected output; the rest is yours to choose.

- A mirror of `InputStreamSlowMultibyteRead` covers `ByteSink`: a subtype of `OutputStream` that declares `write(int)` while `write(byte[], int, int)` resolves to `OutputStream` or `FilterOutputStream`.
- It does not cover `NoFlush`, which declares no `write` method at all. The input side has no such case, because `FilterInputStream.read(byte[], int, int)` passes the range to the wrapped stream.
- An automatic fix that forwards the range to `out` would be wrong for a subclass whose `write(int)` transforms each byte, so a warning without a fix seems safer.

### Searched before filing

Issues and pull requests in google/error-prone, open and closed, for `FilterOutputStream`, `OutputStream multibyte`, `write(byte[], int, int)`, `slow write OutputStream`, `OutputStream single byte`, and `InputStreamSlowMultibyteRead`: nothing related. On `master` at 80c91b3fe1, the only checks under `core/src/main/java` that mention `FilterOutputStream` or `OutputStream.class` are `CloseableDecoratorTypes` and `DefaultCharset`.

### Alternative in use

pgjdbc checks this with an ArchUnit rule over compiled classes at test time. It works for one project, but every project has to write and maintain its own rule, and the rule runs in tests rather than at compile time.

If the check sounds useful, I am happy to draft a PR.

Contributor guide

Open the contributing guide

Research direction

Start by reading the existing InputStreamSlowMultibyteRead check and the checks under core/src/main/java, including the references to OutputStream.class. Define the new warning around OutputStream and FilterOutputStream subclasses that inherit the range-writing implementation, while leaving subclasses that declare it unreported; validate the two requested warnings against the Java example.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
devtools, tooling
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.