Problems with new BooleanLiteral warning

[norrisjeremy]

The new BooleanLiteral warning can lead to code change recommendations that would result in NPE's at runtime.

For example, see the following code that Error Prone will now issue a warning for:

Boolean foo = null;
Boolean bar = 1 > 0 ? foo : Boolean.TRUE;

If you follow naively follow Error Prone's suggestion to turn Boolean.TRUE into true here, this will actually result in an NPE at runtime, as the JVM will attempt to unbox the variable foo (by calling foo.booleanValue()).

My suggestion would be to simply remove this new BooleanLiteral warning entirely from Error Prone, as it seems to be more coding flavor based as opposed to actually identifying real bugs in random code bases, and can instead actually introduce bugs instead of fixing anything.

[norrisjeremy]

To demonstrate in jshell:

|  Welcome to JShell -- Version 21.0.7
|  For an introduction type: /help intro

jshell> Boolean foo = null;
foo ==> null

jshell> Boolean bar = 1 > 0 ? foo : true;
|  Exception java.lang.NullPointerException: Cannot invoke "java.lang.Boolean.booleanValue()" because "REPL.$JShell$11.foo" is null
|        at (#2:1)

[cpovirk]

Yikes, thanks.

Coincidentally, I had just recently relearned this autounboxing horror when IntelliJ pointed it out to me in some code in Truth. I had started poking at trying to make https://errorprone.info/bugpattern/NullTernary recognize it, but the conditions required to trigger it seem to more subtle that I expected, like maybe not triggering if the boxed type is an inferred type, as in a lambda return type?

But it's a lot easier to at least not create the problem. I'm surprised that we don't seem to have heard reports of problems inside Google, as we've had a large BooleanLiteral migration underway. Maybe any code that would have been broken had enough tests to catch the issue? Or maybe we just haven't hit the problems yet: The migration is still underway. We should probably stop it.

I don't want to make further promises on behalf of the actual Error Prone team, but I find this pretty alarming and would encourage them to shut it off until we can avoid causing this problem. (The larger question of which checks should be on by default is a good one that I don't think we have ever had a good answer to.)

[norrisjeremy]

2.40.0 appears to have been released last week with no signs of this being addressed: are there any updates that the Error Prone team can provide?

[norrisjeremy]

Any news on this?
It's been three weeks and not even an acknowledgement from the Error Prone team.

[mfvanek]

Faced with the same false positive in mfvanek/salary-calc#352

[cushon]

This should be fixed in the next release by 6161d4e