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

Skip serialization for elements in generated classes in Scanner #231

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nimakarimipour
Copy link
Member

@nimakarimipour nimakarimipour commented May 13, 2024

NullAway does not report any error within classes that are generated (annotated as @Generated). This PR update all scanner serialization outputs to skip all serializations within a generated class file to reduce I/O and memory usage.

@nimakarimipour nimakarimipour added the enhancement New feature or request label May 13, 2024
@nimakarimipour nimakarimipour self-assigned this May 13, 2024
@nimakarimipour nimakarimipour changed the title Skip serialization for generated elements in Scanner Skip serialization for elements in generated classes in Scanner May 13, 2024
Copy link
Member

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

Couple of minor comments. But I have a higher-level question. Technically, whether NullAway checks @Generated code or not is configurable via flags, see here. In fact, by default, I think NullAway will check code inside @Generated classes. This PR seems to unconditionally assume that @Generated code will not be checked, is this correct? If so, we might want to make this configurable via a flag, and probably that flag should have the same default as NullAway itself.

@@ -122,14 +131,15 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) {
if (methodSymbol == null) {
throw new RuntimeException("not expecting unresolved method here");
}
if (methodSymbol.owner.enclClass().getSimpleName().isEmpty()) {
if (methodSymbol.owner.enclClass().getSimpleName().isEmpty()
|| isInGeneratedClass(methodSymbol, state.context)) {
// An anonymous class cannot declare its own constructors, so we do not need to serialize it.
Copy link
Member

Choose a reason for hiding this comment

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

comment needs an update?

}

/**
* Retrieve the (outermostClass, isNullMarked) record for a given class symbol.
Copy link
Member

Choose a reason for hiding this comment

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

I think this copy-pasted comment is wrong, since the code around null-markedness has been removed

@nimakarimipour
Copy link
Member Author

@msridhar Yes this PR unconditionally skips all serializations in generated classes. I will work on adding a controlling option for that. I think this should be an option that is provided to Annotator, and Annotator pass that to Scanner in config file rather than directly providing to Scanner. But what I am not sure of is that how we can describe this option to avoid confusion. Anything similar to ignore-generated is a bit confusing since it is not really up to Annotator, if this option is provided to Annotator and NullAway still suggest a fix on a generated class, we will still modify that. Another alternative is that we update NullAway serialization that it serialize how it treats generated code in the same file it serializes its serialization version, then Annotator read that file. This way Annotator is just following the configuration on NullAway. Would you please let me know what you think of this ?

@msridhar
Copy link
Member

@nimakarimipour I think it makes sense for NullAway for serialize the options it used to handle generated code; this would certainly be easiest. But to pass this information to the scanner, we would need to run NullAway once before running the scanner, correct? Not sure what order things are run right now.

Taking a step back, #230 seems to have yielded a big performance improvement for the use case we cared about. So maybe this change can be lower priority?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants