Add an initial annotations artifact (with sample @Initializer implementation) #709
Conversation
|
As a follow up diff, we could consider having our own implementations of |
Pull Request Test Coverage Report for Build #1031
💛 - Coveralls |
| * framework events such as {@code onCreate}). | ||
| */ | ||
| @Retention(RetentionPolicy.CLASS) | ||
| @Target({ElementType.METHOD}) |
There was a problem hiding this comment.
One thing to note: this prevents @Initializer in constructors, which was valid before and could be used to mean "ignore all other constructors, if this one initializes it, then we are good!". Not sure how useful that was as a feature, though, and it wasn't exactly intended...
There was a problem hiding this comment.
Are you saying NullAway actually supports this interpretation of @Initializer on constructors? If so maybe we should remove that at some point? In any case I agree we don't want it.
There was a problem hiding this comment.
It does, unless something changed and we haven't checked: #540. It's not high priority to fix, but no reason why we should perpetuate it on our own annotation :)
annotations/gradle.properties
Outdated
| # | ||
|
|
||
| POM_NAME=NullAway | ||
| POM_ARTIFACT_ID=annotations |
There was a problem hiding this comment.
Does this mean the Gradle identifier for the artifact will be :com.uber.nullaway:annotations:X.Y.Z? If so I think I prefer nullaway-annotations as the artifact ID.
There was a problem hiding this comment.
Sounds good to me! Do we want to also change the dir name within our repo? Or are we fine with annotations/ (I think the later, but would be fine either way)
There was a problem hiding this comment.
I think the directory name is fine
| * framework events such as {@code onCreate}). | ||
| */ | ||
| @Retention(RetentionPolicy.CLASS) | ||
| @Target({ElementType.METHOD}) |
There was a problem hiding this comment.
Are you saying NullAway actually supports this interpretation of @Initializer on constructors? If so maybe we should remove that at some point? In any case I agree we don't want it.
| @@ -48,7 +49,6 @@ dependencies { | |||
| testImplementation deps.test.cfCompatQual | |||
| testImplementation deps.build.jspecify | |||
| testImplementation project(":test-java-lib") | |||
| testImplementation deps.test.inferAnnotations | |||
There was a problem hiding this comment.
Should we just remove inferAnnotations entirely from dependencies.gradle? Or is it still used somewhere?
…'t like it there. Official docs use it only on its own line.
lazaroclapp
force-pushed
the
lazaro_nullaway_lib_initial
branch
from
2d1fb94
to
afbb2f6
Compare
The main purpose of this change is to introduce our own annotations artifact, which
can be leveraged to implement more general contracts and other annotations of
interest to NullAway.
Additionally, we start by having our own
@Initializerannotation "implementation"in this annotations jar. I believe Facebook/Meta's Eradicate is being sunset in favor of
Nullsafe (internally at Meta) and NullAway (for OSS), and either way recommending
their annotation jar for NullAway is more of a historical artifact of nullness checking at
Uber and our previous use of Infer/Eradicate than anything else. NullAway will still
acknowledge any annotation with simple name
@Initializer, but now we canrecommend a canonical alternative which we can make sure remains supported.
Additionally, we take the opportunity to make our
@Initializervalid only on methoddeclarations (which is the only place NullAway checks for it).
We use
annotationsrather thanannotation, because that's the choice JSpecify0.3.0 went with in the end, and I refuse to spend another year discussing that 😉