Skip to content
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

Preparing for upcoming release of NullAway (0.9.9) #15

Merged
merged 4 commits into from
Jul 14, 2022

Conversation

nimakarimipour
Copy link
Member

@nimakarimipour nimakarimipour commented Jul 13, 2022

This PR updates Annotator to work with the upcoming release of NullAway 0.9.9.

Through PR621 and PR622 NullAway had the following changes:

  1. Param Protection Test feature is removed from NullAway and the config file for fix serialization has changed.
  2. NullAway will no longer serialize the actual @Nullable annotation, it only suggests the name of the annotation (e.g. nullable) followed by a change in fix serialization config file.

All changes above has been addressed in this PR.

@@ -61,7 +62,7 @@ public static Factory<Fix> factory(FieldDeclarationAnalysis analysis) {
field.variables.addAll(variables);
});
}
return new Fix(new Change(location, infos[7], true), infos[6], infos[8], infos[9]);
return new Fix(new Change(location, config.nullableAnnot, true), info[6], info[8], info[9]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure, all fixes will have "nullable" on their 7th TSV position, right? There aren't currently any "nonnull" fixes or any other kind of fix entries? Maybe worth a Preconditions.checkArgument(info[7].equals("nullable")) before this line, just to guard against that ever changing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly, sure sounds good. This will remind us to change this part if we ever work on new annotations.

09cda4c

Copy link
Member Author

@nimakarimipour nimakarimipour Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the tests on CI are going to fail since it is using the maven central version of NullAway, and the precondition above is causing the termination with error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lazaroclapp For unknown reason, github does nothing when I re-request a review from you, please find the changes applied to your comment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the tests on CI are going to fail since it is using the maven central version of NullAway, and the precondition above is causing the termination with error.

Um, I imagine you want to make other changes to the auto-annotator, so probably we don't want to delay landing this. If we had more than one user for this project, the right call would probably to version our TSV formats and keep support for both versions for a while, but I guess that's overkill here. At the same time, I don't think the preconditions check is worth dealing with a broken CI...

I think my suggestion here would be to comment out the preconditions check with a TODO+Issue to re-enable it once NullAway 0.9.9 has been released. It's a bit hacky, but preferable to all the alternatives above. Apologies for the bikeshedding :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure commented the precondition (cdaf034 ) and added issue :D

Copy link
Collaborator

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@nimakarimipour
Copy link
Member Author

🚀

Encountered an issue #17 while running this annotator version internally. Will resolve it now, however it is irrelevant to NullAway latest changes and it is due to the previous changes in annotator itself.

@nimakarimipour nimakarimipour linked an issue Jul 14, 2022 that may be closed by this pull request
@nimakarimipour nimakarimipour self-assigned this Jul 14, 2022
@nimakarimipour nimakarimipour merged commit 0aaa9e1 into master Jul 14, 2022
@nimakarimipour nimakarimipour deleted the nimak/update-latest-nullaway branch July 14, 2022 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TODO: Add Precondition
2 participants