Skip to content

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.

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?

@nimakarimipour
Copy link
Member Author

Gitar say hi to me.

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.

3 participants