google / google/error-prone

AppliedFix sometimes lies about removing lines

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

Description

In v2.36.0, given

```java
package example;

public final class Foo {
public static void bar() {}
}
```

`PrivateConstructorForUtilityClass` suggests that I delete the class outright:

```
$ ./mvnw clean test-compile -DepFlags='-XepDisableAllChecks -Xep:PrivateConstructorForUtilityClass:ERROR'
[INFO] Scanning for projects...
...
[ERROR] /.../src/main/java/example/Foo.java:[3,14] [PrivateConstructorForUtilityClass] Classes which are not intended to be instantiated should be made non-instantiable with a private constructor. This includes utility classes (classes with only static members), and the main class.
(see https://errorprone.info/bugpattern/PrivateConstructorForUtilityClass)
Did you mean to remove this line?
```

But if I actually apply `PrivateConstructorForUtilityClass`'s fix, fortunately, it correctly inserts a private constructor and the code subsequently passes the check:

```
$ ./mvnw clean test-compile -DepFlags='-XepDisableAllChecks -Xep:PrivateConstructorForUtilityClass:ERROR' -DepPatchChecks=PrivateConstructorForUtilityClass
[INFO] Scanning for projects...
...
Refactoring changes were successfully applied to file:///.../src/main/java/example/Foo.java, please check the refactored code and recompile.
[INFO] /.../src/main/java/example/Foo.java:[3,14] [PrivateConstructorForUtilityClass] Classes which are not intended to be instantiated should be made non-instantiable with a private constructor. This includes utility classes (classes with only static members), and the main class.
(see https://errorprone.info/bugpattern/PrivateConstructorForUtilityClass)
Did you mean to remove this line?
```

```java
package example;

public final class Foo {
public static void bar() {}

private Foo() {}
}
```

I believe this happens because of an interaction between some of the general helpers used by `PrivateConstructorForUtilityClass`'s fix and a heuristic in `AppliedFix` that determines whether a fix is a "removal". Consider the following debugger state from a variation of `com.google.errorprone.bugpatterns.PrivateConstructorForUtilityClassTest#b30170662` where `Foo` is already `final`:

![Image](https://github.com/user-attachments/assets/bef77bc4-b7fd-4f98-a6df-859ba7ad96f1)

Because `snippet` will eventually evaluate to empty, the resulting `AppliedFix` will have `isRemoveLine=true`, hence the fix suggestion.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.