-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix NullPointerException in cquery --output=graph. #13002
Conversation
} | ||
String checksum1 = ct1.getConfigurationChecksum(); | ||
String checksum2 = ct2.getConfigurationChecksum(); | ||
if (Objects.equals(checksum1, checksum2)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as the compareTo below, and the checksum2 null check isn't needed, either. I'd simplify this as:
if (checksum1 == null) {
return -1;
} else {
return checksum1.compareTo(checksum2);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How's this? I left the double null check because of this comment from https://docs.oracle.com/javase/8/docs/api/java/lang/Comparable.html:
Note that null is not an instance of any class, and e.compareTo(null) should throw a NullPointerException even though e.equals(null) returns false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or am I missing something about the null check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I hadn't realized that. Okay, the second null check is necessary, then.
Also note I don't know how to add a test since I don't know how to invoke a dep simultaneously in a null and non-null configuration. It's probably worth exploring how the query logic decides a |
Fixes #12915.
The repro from the bug crashes because these two deps are compared:
Target: @remote_java_tools_linux//:java_tools/src/tools/singlejar/singlejar_local
Config: d8d0eb07eb92791668ac973282be1523379b0025b22f0ade56b13d519f2feb2a
Target: @remote_java_tools_linux//:java_tools/src/tools/singlejar/singlejar_local
Config: null
This is a
cc_binary
:bazel/src/tools/singlejar/BUILD
Lines 92 to 93 in 0c1257e
I have no idea why the graph visitor tags one of its instances with a null configuration. Nevertheless, this code is still safer.