AppliedFix sometimes lies about removing lines
- 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`:

Because `snippet` will eventually evaluate to empty, the resulting `AppliedFix` will have `isRemoveLine=true`, hence the fix suggestion.
Contributor guide
Assessment
This issue has not been assessed yet.