I agree that there are _some_ added annotations at _some_, that are
useful - most notably @NonNull on method arguments, possibly return
values. Adding @NonNull into exception type being thrown seems awkward.
The @UnknownKeyFor probably should not be there, as it brings no value.
Did we raise the issue with the checkerframework? It seems to me, that
the biggest problem lies there. It might have two modes of operation -
after the check it could have a way of specifying which (and where)
annotations should be in the compiled byte-code and which should be
removed. Or can we post-process that with some different tool?
Jan
On 4/5/21 6:03 PM, Kenneth Knowles wrote:
On Thu, Apr 1, 2021 at 9:57 AM Brian Hulette <[email protected]
<mailto:[email protected]>> wrote:
What value does it add? Is it that it enables them to use
checkerframework with our interfaces?
Actually if they are also using checkerframework the defaults are the
same so it is not usually needed (though some defaults can be
changed). Making defaults explicit is most useful for interfacing with
other tools with different defaults, such as Spotbugs [1], IDEs [2]
[3], or JVM languages with null safety bult-in, etc [4] [5].
Kenn
[1]
https://spotbugs.readthedocs.io/en/stable/annotations.html#edu-umd-cs-findbugs-annotations-nullable
<https://spotbugs.readthedocs.io/en/stable/annotations.html#edu-umd-cs-findbugs-annotations-nullable>
[2]
https://www.jetbrains.com/help/idea/2021.1/nullable-and-notnull-annotations.html
<https://www.jetbrains.com/help/idea/2021.1/nullable-and-notnull-annotations.html>
[3] https://wiki.eclipse.org/JDT_Core/Null_Analysis
<https://wiki.eclipse.org/JDT_Core/Null_Analysis>
[4] https://kotlinlang.org/docs/null-safety.html
<https://kotlinlang.org/docs/null-safety.html>
[5]
https://kotlinlang.org/docs/java-interop.html#null-safety-and-platform-types
<https://kotlinlang.org/docs/java-interop.html#null-safety-and-platform-types>
On Thu, Apr 1, 2021 at 8:54 AM Kenneth Knowles <[email protected]
<mailto:[email protected]>> wrote:
Thanks for filing that. Once it is fixed in IntelliJ, the
annotations actually add value for downstream users.
Kenn
On Thu, Apr 1, 2021 at 1:10 AM Jan Lukavský <[email protected]
<mailto:[email protected]>> wrote:
Hi,
I created the issue in JetBrains tracker [1]. I'm still
not 100% convinced that it is correct for the checker to
actually modify the bytecode. An open questions is - in
guava this does not happen. Does guava apply the check on
code being released? From what is in this thread is seems
to me, that the answer is no.
Jan
[1] https://youtrack.jetbrains.com/issue/IDEA-265658
<https://youtrack.jetbrains.com/issue/IDEA-265658>
On 4/1/21 6:15 AM, Kenneth Knowles wrote:
Hi all,
About the IntelliJ automatic method stub issue: I raised
it to the checkerframework list and got a helpful
response:
https://groups.google.com/g/checker-framework-discuss/c/KHQdjF4lesk/m/dJ4u1BBNBgAJ
<https://groups.google.com/g/checker-framework-discuss/c/KHQdjF4lesk/m/dJ4u1BBNBgAJ>
It eventually reached back to Jetbrains and they would
appreciate a detailed report with steps to reproduce,
preferably a sample project. Would you - Jan or Ismaël or
Reuven - provide them with this issue report? It sounds
like Jan you have an example ready to go.
Kenn
On Mon, Mar 15, 2021 at 1:29 PM Jan Lukavský
<[email protected] <mailto:[email protected]>> wrote:
Yes, annotations that we add to the code base on
purpose (like @Nullable or @SuppressWarnings) are
aboslutely fine. What is worse is that the checked is
not only checked, but a code generator. :)
For example when one wants to implement Coder by
extending CustomCoder and use auto-generating the
overridden methods, they look like
@Override public void encode(Long value, @UnknownKeyFor
@NonNull @Initialized OutputStream outStream)throws
@UnknownKeyFor@NonNull@Initialized CoderException,
@UnknownKeyFor@NonNull@Initialized IOException {
}
Which is really ugly. :-(
Jan
On 3/15/21 6:37 PM, Ismaël Mejía wrote:
+1
Even if I like the strictness for Null checking, I also think
that
this is adding too much extra time for builds (that I noticed
locally
when enabled) and also I agree with Jan that the annotations are
really an undesired side effect. For reference when you try to
auto
complete some method signatures on IntelliJ on downstream
projects
with C-A-v it generates some extra Checkers annotations like
@NonNull
and others even if the user isn't using them which is not
desirable.
On Mon, Mar 15, 2021 at 6:04 PM Kyle Weaver<[email protected]>
<mailto:[email protected]> wrote:
Big +1 for moving this to separate CI job. I really don't like
what annotations are currently added to the code we ship. Tools like Idea add
these annotations to code they generate when overriding classes and that's very
annoying. Users should not be exposed to internal tools like nullability
checking.
I was only planning on moving this to a separate CI job. The
job would still be release blocking, so the same annotations would still be
required.
I'm not sure which annotations you are concerned about. There
are two annotations involved with nullness checking, @SuppressWarnings and
@Nullable. @SuppressWarnings has retention policy SOURCE, so it shouldn't be
exposed to users at all. @Nullable is not just for internal tooling, it also
provides useful information about our APIs to users. The user should not have
to guess whether a method argument etc. can be null or not, and for better or
worse, these annotations are the standard way of expressing that in Java.
