Update Library Model Loader to mark fields Nullable #220
Conversation
annotator-core/src/main/java/edu/ucr/cs/riple/core/injectors/VirtualInjector.java
Outdated
Show resolved
Hide resolved
annotator-core/src/main/java/edu/ucr/cs/riple/core/injectors/VirtualInjector.java
Outdated
Show resolved
Hide resolved
| Path path, | ||
| Function<AddAnnotation, Stream<String>> mapper) { | ||
| try (BufferedOutputStream os = new BufferedOutputStream(new FileOutputStream(path.toFile()))) { | ||
| Set<String> rows = annotations.flatMap(mapper).collect(Collectors.toSet()); | ||
| for (String row : rows) { | ||
| os.write(row.getBytes(Charset.defaultCharset()), 0, row.length()); | ||
| } | ||
| os.flush(); |
There was a problem hiding this comment.
Not a big deal, but I don't think this call is needed, since the stream will immediately be closed afterward
There was a problem hiding this comment.
Sure, fixed in f775354
There was a problem hiding this comment.
I still see the call to os.flush()? Why is this needed?
There was a problem hiding this comment.
Is the call to os.flush() redundant (or guaranteed to be executed after the block) in try-with-resources block ? IntelliJ only grays out call to os.close() but not for os.flush().
There was a problem hiding this comment.
try-with-resources guarantees the stream will be closed, and I believe it will be flushed automatically before it is closed.
There was a problem hiding this comment.
Ok thanks for that. Then we can remove it. Fixed in 24f0a24
| @@ -82,6 +83,23 @@ public static void executeCommand(Config config, String command) { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Clears the content of the file at the given path if the file exists. | |||
There was a problem hiding this comment.
Why don't you just delete the file?
There was a problem hiding this comment.
Sure, that sounds better. Fixed in 6dd203e
library-model-loader/src/main/java/edu/ucr/cs/riple/librarymodel/LibraryModelLoader.java
Show resolved
Hide resolved
| @Override | ||
| public ImmutableList<StreamTypeRecord> customStreamNullabilitySpecs() { | ||
| return LibraryModels.super.customStreamNullabilitySpecs(); | ||
| } |
There was a problem hiding this comment.
Isn't this override unnecessary? It just calls the super method?
There was a problem hiding this comment.
Thanks for catching this, I really don't know what I was thinking 🤦🏻♂️ de61967
library-model-loader/src/main/java/edu/ucr/cs/riple/librarymodel/LibraryModelLoader.java
Show resolved
Hide resolved
…irtualInjector.java Co-authored-by: Manu Sridharan <[email protected]>
…irtualInjector.java Co-authored-by: Manu Sridharan <[email protected]>
@msridhar Thank you very much for the review, this is ready for another round. Regarding
We weren't creating / deleting files in resource directory (which we are now 6dd203e). We used to assume the file exists and clear contents instead of deleting. With the new approach, we no longer need the file. |
|
@msridhar I don't see the |
Yes, you're right. For some reason GitHub shows the change strangely. Locally git shows the file as removed. Sorry for the confusion. |
Followup on an update in NullAway recent change #878 which enables NullAway to mark fields as
@Nullable. This PR updates ourLibraryModelsLoaderto enable communication of Annotator and NullAway to mark fields@Nullablewhile analyzing downstream dependencies. This is required to prepare the code for the upcoming change which enables Annotator to be informed of impacts of making fields@Nullableon downstream dependencies.