Create com.uber.nullaway.generics package #855
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #855 +/- ##
============================================
- Coverage 86.95% 86.95% -0.01%
- Complexity 1890 1919 +29
============================================
Files 74 77 +3
Lines 6216 6215 -1
Branches 1209 1209
============================================
- Hits 5405 5404 -1
Misses 405 405
Partials 406 406 ☔ View full report in Codecov by Sentry. |
|
There is no reduction in code coverage due to this PR; it's just @codecov getting a bit confused since code is moving around into new files. |
| @@ -92,7 +92,7 @@ public class ErrorBuilder { | |||
| * expression into a @NonNull target, and this parameter is the Symbol for that target. | |||
| * @return the error description | |||
| */ | |||
| Description createErrorDescription( | |||
| public Description createErrorDescription( | |||
There was a problem hiding this comment.
I assume this is for future uses?
There was a problem hiding this comment.
This is because the PR moves the generics code into its own new package, so it cannot see this method anymore (default visibility for Java methods is "package private").
| * Visitor that checks equality of nullability annotations for all nested generic type arguments | ||
| * within a type. Compares the Type it is called upon, i.e. the LHS type and the Type passed as an |
There was a problem hiding this comment.
This is the only part that got updated, where the rest of the code is copied verbatim from the original file.
Makes sense to me!
There was a problem hiding this comment.
Yes, exactly, we extracted this nested class to a new source file
| * <p>This code is a modified and extended version of code in {@link | ||
| * com.google.errorprone.util.Signatures} | ||
| */ | ||
| final class GenericTypePrettyPrintingVisitor extends Types.DefaultTypeVisitor<String, Void> { |
There was a problem hiding this comment.
Same here, I think this is strictly extracting code from the original file. LGTM
There was a problem hiding this comment.
Yup, no code changes, just moving code to a top-level source file
| static final String NULLABLE_NAME = "org.jspecify.annotations.Nullable"; | ||
|
|
||
| private static final Supplier<Type> NULLABLE_TYPE_SUPPLIER = | ||
| Suppliers.typeFromString(NULLABLE_NAME); | ||
| static final Supplier<Type> NULLABLE_TYPE_SUPPLIER = Suppliers.typeFromString(NULLABLE_NAME); |
There was a problem hiding this comment.
This might be me not too familiar with the rest of the codebase, but shouldn't these two actually be shared across NullAway?
Also, I think NullAway supports more than one nullable type suppliers, so this confuses me a little (that we should just rely on whatever set of suppliers we already have in the rest of NullAway instead of defining it again here)
But I think I'm missing something 🤔
There was a problem hiding this comment.
So this code is about getting a Type object for org.jspecify.annotations.Nullable. Some code in this file specifically checks for @org.jspecify.annotations.Nullable annotations, and only implements JSpecify behavior if that exact annotation is present. This was to be conservative; I think it's still an open question as to how we will handle other type use annotations and also declaration annotations (see #853). I will rename these variables and add some comments to indicate what is going on here.
And, BTW, the reason we code in this manner is to avoid comparing Types by their String representation, see this Error Prone check.
There was a problem hiding this comment.
Actually, I'll do those changes in a follow up to keep this PR simple
Fixes #817. This is a refactoring with no semantic changes.