-
Notifications
You must be signed in to change notification settings - Fork 5
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 #55 by supporting @NullUnmarked #69
Conversation
@lazaroclapp Thank you for the review, this is ready for another review. |
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.
Nice. I appreciate the refactoring into AddMarkerAnnotation
and AddSingleElementAnnotation
. Some (hopefully minor) comments below. Please note that there are also a few - even smaller - nits in the replies to comment threads from the last pass.
new OnMethod("Super.java", "com.edu.Super", "test(java.lang.Object)"), | ||
"edu.ucr.CustomNull", | ||
"arg3", | ||
true)) |
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.
Appreciate the comprehensive test suite here, btw!
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.
Sure 🙂
* If true, if an annotation with the same name exists, the existing annotation should include the | ||
* passed argument, otherwise, a new annotation will be injected to the node. | ||
*/ | ||
private final boolean collapseArguments; |
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.
Why call it this and not just repeatable
to follow JLS terminology? (It would imply reversing the logic, of course, if repeatable=true
then a new annotation is added, if it's false, then the expression inside is expanded into an array expression).
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.
Thanks for the suggestion, that makes sense. d3c427c
super(location, annotation); | ||
Preconditions.checkArgument( | ||
argument != null && !argument.equals(""), | ||
"argument cannot be null or empty, use AddAnnotation instead."); |
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.
I understand not allowing null (Is this code being checked by NullAway, though? Because if so, we don't need the runtime check 😉 ). But, why is empty string not allowed? @Foo("")
is a valid annotation if Foo
is a Single-Element Annotation.
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.
Not now, but It will be checked by NullAway
as a long-term plan mentioned in #21 in future. I think it does not make sense to add the annotation as @Foo("")
. Maybe we can force it to use MarkerAnnotation
in such cases ? However, if you think still it might be useful in some scenarios to support such injections, please let me know and I will remove the guard. Thank you.
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.
Let's leave it like this for now. I don't think there is anything in the Java spec that prevents you from having a single element annotation with the empty string as its value
field, and there is definitely a semantic difference between empty-string vs no-value, but I don't think we care for any of the foreseeable use cases of the auto-annotator, so let's leave the check in for now.
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.
Ok great sounds great. Will update it in future if needed.
.anyMatch( | ||
annot -> { | ||
if (!(annot.getNameAsString().equals(annotation) | ||
|| annot.getNameAsString().equals(annotationSimpleName))) { |
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.
Let's not change this in this PR, but it's a bit weird that annotationSimpleName
(and annotation
) are fields of Change
, rather than of AddAnnotation
.
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.
Sure, will do that.
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.
(SingleMemberAnnotationExpr) existingAnnotation; | ||
ArrayInitializerExpr updatedMemberValue = new ArrayInitializerExpr(); | ||
NodeList<Expression> nodeList = new NodeList<>(); | ||
nodeList.add(argumentExp); |
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.
Nit: I'd move this below the two ifs below, that way we are appending to argument lists, rather than prepending
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.
@lazaroclapp Thanks for the review, this is ready for another review. |
} | ||
|
||
@Override | ||
@SuppressWarnings("unchecked") |
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 one should still be removed.
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.
super(location, annotation); | ||
Preconditions.checkArgument( | ||
argument != null && !argument.equals(""), | ||
"argument cannot be null or empty, use AddAnnotation instead."); |
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.
Let's leave it like this for now. I don't think there is anything in the Java spec that prevents you from having a single element annotation with the empty string as its value
field, and there is definitely a semantic difference between empty-string vs no-value, but I don't think we care for any of the foreseeable use cases of the auto-annotator, so let's leave the check in for now.
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.
Very minor nits remaining: changes to comments and one unnecessary suppression
@@ -95,13 +95,13 @@ public void visit(NodeWithAnnotations<?> node) { | |||
return; | |||
} | |||
|
|||
// Annotation with the same name exists, if collapse is off, add it directly. | |||
if (!collapseArguments) { | |||
// Annotation with the same name exists, if repeatable is off, add it directly. |
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.
// Annotation with the same name exists, but the annotation is repeatable, add it directly.
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.
Updated it. Thanks for the suggestion.
addAnnotationExpressionOnNode(node, argumentExp); | ||
return; | ||
} | ||
|
||
// Annotation with the same name exists, collapse is on, update it. | ||
// Annotation with the same name exists, repeatable is on, update it. |
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.
// Annotation with the same name exists and is not repeatable, update it.
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.
Updated it.
@lazaroclapp Thank you for the review, will land it once the CI pass. |
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.
LGTM! :)
This PR is built upon GH-68.GH-68 is landed.This PR resolves #55 by adding a configuration mode which if activated, all remaining errors will be resolved with following the rules below:
@NullUnmarked
.@SuppressWarnings("NullAway.Init")
.@Nullable
assignments to fields will be annotated as@SuppressWarnings("NullAway")
.With this PR, the target module will enroll into NullAway with no triggered errors.
To enable this mode,
cli
flag below must be passed to Annotator: