Fix PreconditionsNotNullPrimitive checker

Fixes a bad assertion error in the PreconditionsNotNullPrimitive bug checker.  ASTHelpers.getRootIdentifier() is documented as returning null in several legitimate cases, so it shouldn't be treated as a fatal error when it happens.

Additionally, this patch adds some unit tests for ASTHelpers.getRootIdentifier() based on the javadoc examples, and fixes the implementation to pass.

[supertri, 2013-10-25 19:12:50]

https://codereview.appspot.com/17220043/diff/20001/core/src/main/java/com/goo...
File core/src/main/java/com/google/errorprone/util/ASTHelpers.java (right):

https://codereview.appspot.com/17220043/diff/20001/core/src/main/java/com/goo...
core/src/main/java/com/google/errorprone/util/ASTHelpers.java:167: methodSelect
= true;
Why set to true again here? Why not just deleted the entire if-block? expr as
defined here is not used since we return null below.

https://codereview.appspot.com/17220043/diff/20001/core/src/test/java/com/goo...
File core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java (right):

https://codereview.appspot.com/17220043/diff/20001/core/src/test/java/com/goo...
core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java:127:
assertCompiles(expressionStatementMatches("x.trim().intern()",
expressionHasRootIdentifier("x")));
Some of these lines are more than 100 chars.

[mdempsky, 2013-10-25 20:22:05]

https://codereview.appspot.com/17220043/diff/20001/core/src/main/java/com/goo...
File core/src/main/java/com/google/errorprone/util/ASTHelpers.java (right):

https://codereview.appspot.com/17220043/diff/20001/core/src/main/java/com/goo...
core/src/main/java/com/google/errorprone/util/ASTHelpers.java:167: methodSelect
= true;
On 2013/10/25 19:12:51, supertri wrote:
> Why set to true again here? Why not just deleted the entire if-block? expr as
> defined here is not used since we return null below.

The original logic was manually unwrapping "x()" into "x" and "y.z" into "y",
and to get "intern().intern()" to return null, we needed to know whether the
last unwrap operation was from a MethodInvocationTree or a MemberSelectTree.

But I realized it's actually redundant with the VarSymbol check (because a
variable can't be used in a MethodInvocationTree anyway), and it misses a lot of
other cases like array indexing, etc.

I've replaced the code with a TreeScanner to be as general as possible: now we
find all variables not just if they're the "root" variable, so
checkNonNull(f(arg)) will also recommend using checkArgument(f(arg)), which I
think is desirable.

https://codereview.appspot.com/17220043/diff/20001/core/src/test/java/com/goo...
File core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java (right):

https://codereview.appspot.com/17220043/diff/20001/core/src/test/java/com/goo...
core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java:127:
assertCompiles(expressionStatementMatches("x.trim().intern()",
expressionHasRootIdentifier("x")));
On 2013/10/25 19:12:51, supertri wrote:
> Some of these lines are more than 100 chars.

Done.

[eaftan, 2013-10-29 00:56:26]

I'm back from jury duty!

https://codereview.appspot.com/17220043/diff/60001/core/src/main/java/com/goo...
File core/src/main/java/com/google/errorprone/util/ASTHelpers.java (right):

https://codereview.appspot.com/17220043/diff/60001/core/src/main/java/com/goo...
core/src/main/java/com/google/errorprone/util/ASTHelpers.java:155: *   
x.y.z(s.t) ==> {x,s}
Why is this needed?  If you need the root identifier from the argument, couldn't
you just pass the argument into a separate call of this method?

Edit: from the comment, I think I see -- it results in better suggestions for
the PreconditionsCheckNotNullPrimitive check.  Are there other users of this
method (please check inside Google as well)?  If not, maybe this version is
specific to the check and should be moved there.

[mdempsky, 2013-10-29 15:47:54]

Welcome back!

https://codereview.appspot.com/17220043/diff/60001/core/src/main/java/com/goo...
File core/src/main/java/com/google/errorprone/util/ASTHelpers.java (right):

https://codereview.appspot.com/17220043/diff/60001/core/src/main/java/com/goo...
core/src/main/java/com/google/errorprone/util/ASTHelpers.java:155: *   
x.y.z(s.t) ==> {x,s}
On 2013/10/29 00:56:26, eaftan wrote:
> Why is this needed?  If you need the root identifier from the argument,
couldn't
> you just pass the argument into a separate call of this method?

The function call might be more complicated; e.g., something like
"x.y(a.b).z(c.d)[e]" where we'd want "{x,a,c,e}" as the free variables used by
the expression.  I'm assuming if any of those were function parameters, we'd
want to use checkArgument instead of checkState.

> Are there other users of this
> method (please check inside Google as well)?  If not, maybe this version is
> specific to the check and should be moved there.

PreconditionsCheckNotNullPrimitive is the only use I find across error-prone and
internal Google code.

[eaftan, 2013-10-29 21:01:09]

Looks good aside from my one request.  After that, please merge it into trunk.

https://codereview.appspot.com/17220043/diff/60001/core/src/main/java/com/goo...
File core/src/main/java/com/google/errorprone/util/ASTHelpers.java (right):

https://codereview.appspot.com/17220043/diff/60001/core/src/main/java/com/goo...
core/src/main/java/com/google/errorprone/util/ASTHelpers.java:155: *   
x.y.z(s.t) ==> {x,s}
> The function call might be more complicated; e.g., something like
> "x.y(a.b).z(c.d)[e]" where we'd want "{x,a,c,e}" as the free variables used by
> the expression.  I'm assuming if any of those were function parameters, we'd
> want to use checkArgument instead of checkState.

SGTM.

> > Are there other users of this
> > method (please check inside Google as well)?  If not, maybe this version is
> > specific to the check and should be moved there.
> 
> PreconditionsCheckNotNullPrimitive is the only use I find across error-prone
and
> internal Google code.

Then lets move this helper method into PreconditionsCheckNotNullPrimitive
itself.

[mdempsky, 2013-10-29 22:33:21]

On 2013/10/29 21:01:09, eaftan wrote:
> Then lets move this helper method into PreconditionsCheckNotNullPrimitive
> itself.

Done.  I had to change PreconditionsCheckNotNullPrimitiveTest a bit to keep the
unit tests though.  I'm not thrilled about it, but I think it's okay for now? 
I'd prefer to clean it up in a followup patch if that's okay with you.

[eaftan, 2013-10-29 22:40:48]

On 2013/10/29 22:33:21, mdempsky wrote:
> On 2013/10/29 21:01:09, eaftan wrote:
> > Then lets move this helper method into PreconditionsCheckNotNullPrimitive
> > itself.
> 
> Done.  I had to change PreconditionsCheckNotNullPrimitiveTest a bit to keep
the
> unit tests though.  I'm not thrilled about it, but I think it's okay for now? 
> I'd prefer to clean it up in a followup patch if that's okay with you.

That's fine with me.

[mdempsky, 2013-10-29 22:47:41]

On 2013/10/29 22:40:48, eaftan wrote:
> That's fine with me.

Thanks, committed as
https://code.google.com/p/error-prone/source/detail?r=3d327bc182e846f8ba56bc7...