You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Kenneth Knowles <ke...@apache.org> on 2021/04/01 04:15:17 UTC
Re: Null checking in Beam
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
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ý <je...@seznam.cz> 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
>
> @Overridepublic 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 <kc...@google.com> <kc...@google.com> 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.
>
>
Re: Null checking in Beam
Posted by Jan Lukavský <je...@seznam.cz>.
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 <bhulette@google.com
> <ma...@google.com>> 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 <kenn@apache.org
> <ma...@apache.org>> 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ý <je.ik@seznam.cz
> <ma...@seznam.cz>> 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ý
>> <je.ik@seznam.cz <ma...@seznam.cz>> 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<kc...@google.com> <ma...@google.com> 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.
>>
Re: Null checking in Beam
Posted by Kenneth Knowles <ke...@apache.org>.
On Thu, Apr 1, 2021 at 9:57 AM Brian Hulette <bh...@google.com> 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
[2]
https://www.jetbrains.com/help/idea/2021.1/nullable-and-notnull-annotations.html
[3] https://wiki.eclipse.org/JDT_Core/Null_Analysis
[4] https://kotlinlang.org/docs/null-safety.html
[5]
https://kotlinlang.org/docs/java-interop.html#null-safety-and-platform-types
On Thu, Apr 1, 2021 at 8:54 AM Kenneth Knowles <ke...@apache.org> 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ý <je...@seznam.cz> 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
>>> 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
>>>
>>> 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ý <je...@seznam.cz> 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
>>>>
>>>> @Overridepublic 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 <kc...@google.com> <kc...@google.com> 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.
>>>>
>>>>
Re: Null checking in Beam
Posted by Brian Hulette <bh...@google.com>.
What value does it add? Is it that it enables them to use checkerframework
with our interfaces?
On Thu, Apr 1, 2021 at 8:54 AM Kenneth Knowles <ke...@apache.org> 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ý <je...@seznam.cz> 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
>> 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
>>
>> 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ý <je...@seznam.cz> 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
>>>
>>> @Overridepublic 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 <kc...@google.com> <kc...@google.com> 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.
>>>
>>>
Re: Null checking in Beam
Posted by Kenneth Knowles <ke...@apache.org>.
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ý <je...@seznam.cz> 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
> 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
>
> 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ý <je...@seznam.cz> 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
>>
>> @Overridepublic 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 <kc...@google.com> <kc...@google.com> 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.
>>
>>
Re: Null checking in Beam
Posted by Jan Lukavský <je...@seznam.cz>.
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
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ý <je.ik@seznam.cz
> <ma...@seznam.cz>> 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<kc...@google.com> <ma...@google.com> 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.
>