[20498] Remove SuppressWarnings nullness from SelectByteBuddyHelpers

[Damon Douglas <da...@apache.org>]

Hello Everyone,

*For those experienced with Beam*:

PR/26413 [1] begins the long journey of addressing [2] which is a subtask
of [3].  千里之行，始於足下 :-)

*For those beginning on the learning path*:

The pull request referenced in the email is not directly related to Beam
but involves a Java annotation called SuppressWarnings.  Per its namesake,
it suppresses warnings according to the values you set in the annotation.
In [2] and [3]'s context the particular setting is "nullness" that
suppresses any warnings related to potentially null references in the code.

In the Beam repository, we utilize The Checker Framework [4] to help
maintain a clean codebase.  When such annotation applies, it will
essentially mask these warnings and let the code pass without errors.  When
removing the SuppressWarnings with "nullness" annotation, the checks fail.

Why is this a big deal?  The danger of nullness and the validity of the
warnings depend on the code context.  However, as of this writing there are
many Java classes in our repository with the SuppressWarnings annotation.
What is the likelihood they are masking something serious?

Here in our discussion would be a great place to be practical and relay
what can be done when receiving a null-related error from the checker
framework.  However, I want to wait after I do more of these and find
validation through peer review.  Then, I will follow-up with the dev list.

Meanwhile, you can take a look at my attempt in the PR [1].

Best,

Damon

*References*:
1. https://github.com/apache/beam/pull/26413
2. https://github.com/apache/beam/issues/20498
3. https://github.com/apache/beam/issues/20497
4. https://checkerframework.org/

[TANGENT] Beam 3.0 / DoFn API thoughts

[Kenneth Knowles <ke...@apache.org>]

Appropriately changed subject as it is just random thoughts :-)

On Thu, Apr 27, 2023 at 9:37 AM Vincent Marquez <vi...@gmail.com>
wrote:

>
>> As an example of both of the above: it can be very hard to fix API
>> designs like DoFn (or JUnit classes, etc) of the start/doSomething/end
>> variety that rely on a caller to invoke methods in a particular order and
>> have significant inter-method stateful dependencies that are not at all
>> guaranteed by their design.
>>
>
> Maybe warrants a new topic, but has there been any talk about what a
> newer, more principled Beam 3.0 user facing API might look like and if
> not,  is it something you've given some thought to?
>

I don't think Beam 3.0 will be any time soon, but the community could prove
me wrong. I think the case for user value is just not there, so we would
have to figure that out. Ideally most of the new things would be available
in Beam 2.x under a --beam-3 flag so users can opt in, and then Beam 3.0 is
just "flip the switch". This may not be realistic. Actually my favorite
versioning model is the old Linux Kernel model where every odd version is
another 0.x round of new changes, and even versions are perfectly stable.
This allows rapid development to occur alongside more conservative
maintenance.

About the DoFn API: yes, there are very simple reliable patterns for things
like this. You don't do things like setup(); startBundle(); process();
finishBundle(); teardown() all on one object. Instead, you have an object
with setup() that returns a closeable thing that has startBundle() that
returns a closeable thing that has process() method. Now naming can get
challenging, since you may not like names like DoFnFactoryFactory (with
setup() method) and DoFnFactory (with startBundle() method) and then DoFn
(with process() method) :-). Fixing DoFn doesn't require anything fancy,
really.

And there is another very real problem with DoFn because the annotation
driven API is (in theory) good for users but bad for automation and
composing. The DoFnInvoker class was meant for internal use only but it is
the right sort of API for automated building of DoFns that wrap other DoFns.

Kenn



>
>
>
>
>
>> Such APIs are extremely fragile and the source of a huge number of bugs
>> because it is very easy and common to invoke methods in an order or
>> multiplicity that was not designed for. Not at all coincidentally, it is
>> quite hard and annoying to add the needed "Requires" and "Ensures"
>> annotations to get them to type check, and also very hard and annoying to
>> satisfy those requirements when calling the methods. This is all a feature
>> - the design was bad, and the difficulty setting up and satisfying the
>> invariants helps you realize it.
>>
>> /lecture :-)
>>
>> Kenn
>>
>> On Tue, Apr 25, 2023 at 9:56 AM Damon Douglas <da...@apache.org>
>> wrote:
>>
>>> Hello Everyone,
>>>
>>> *For those experienced with Beam*:
>>>
>>> PR/26413 [1] begins the long journey of addressing [2] which is a
>>> subtask of [3].  千里之行，始於足下 :-)
>>>
>>> *For those beginning on the learning path*:
>>>
>>> The pull request referenced in the email is not directly related to Beam
>>> but involves a Java annotation called SuppressWarnings.  Per its namesake,
>>> it suppresses warnings according to the values you set in the annotation.
>>> In [2] and [3]'s context the particular setting is "nullness" that
>>> suppresses any warnings related to potentially null references in the code.
>>>
>>> In the Beam repository, we utilize The Checker Framework [4] to help
>>> maintain a clean codebase.  When such annotation applies, it will
>>> essentially mask these warnings and let the code pass without errors.  When
>>> removing the SuppressWarnings with "nullness" annotation, the checks fail.
>>>
>>> Why is this a big deal?  The danger of nullness and the validity of the
>>> warnings depend on the code context.  However, as of this writing there are
>>> many Java classes in our repository with the SuppressWarnings annotation.
>>> What is the likelihood they are masking something serious?
>>>
>>> Here in our discussion would be a great place to be practical and relay
>>> what can be done when receiving a null-related error from the checker
>>> framework.  However, I want to wait after I do more of these and find
>>> validation through peer review.  Then, I will follow-up with the dev list.
>>>
>>> Meanwhile, you can take a look at my attempt in the PR [1].
>>>
>>> Best,
>>>
>>> Damon
>>>
>>> *References*:
>>> 1. https://github.com/apache/beam/pull/26413
>>> 2. https://github.com/apache/beam/issues/20498
>>> 3. https://github.com/apache/beam/issues/20497
>>> 4. https://checkerframework.org/
>>>
>>>
>>>
> *~Vincent*
>

Re: [20498] Remove SuppressWarnings nullness from SelectByteBuddyHelpers

[Vincent Marquez <vi...@gmail.com>]

On Tue, Apr 25, 2023 at 12:26 PM Kenneth Knowles <ke...@apache.org> wrote:

> This is great work. Thanks for writing about it.
>
> I hold a stronger opinion than "the danger of nullness and the validity of
> the warnings depend on the code context": nullness is a solved problem, and
> type systems solved it, decades ago in some languages and more recently in
> Java. Nullness errors are a choice, and we should not choose to have them.
> Treat them as you would treat any other type error.
>
> And I hold even strong opinions than that! Almost all the time, unclear
> nullness/typing has an underlying cause of lack of clarity in design and
> thinking. This is why fixing nullness almost always improves the code, but
> also why it can be difficult to do after the fact, because it can require
> fixing a bad or broken design. But sometimes it is easy, and you should do
> it with gusto in either way :-)
>
> And more! If checkerframework cannot determine the correctness of
> something, probably neither can you. You may think you can, but you are
> probably wrong. And even if you are correct today, the next person coming
> along will not have your internal mental state and will easily break it.
>
> As an example of both of the above: it can be very hard to fix API designs
> like DoFn (or JUnit classes, etc) of the start/doSomething/end variety that
> rely on a caller to invoke methods in a particular order and have
> significant inter-method stateful dependencies that are not at all
> guaranteed by their design.
>

Maybe warrants a new topic, but has there been any talk about what a newer,
more principled Beam 3.0 user facing API might look like and if not,  is it
something you've given some thought to?





> Such APIs are extremely fragile and the source of a huge number of bugs
> because it is very easy and common to invoke methods in an order or
> multiplicity that was not designed for. Not at all coincidentally, it is
> quite hard and annoying to add the needed "Requires" and "Ensures"
> annotations to get them to type check, and also very hard and annoying to
> satisfy those requirements when calling the methods. This is all a feature
> - the design was bad, and the difficulty setting up and satisfying the
> invariants helps you realize it.
>
> /lecture :-)
>
> Kenn
>
> On Tue, Apr 25, 2023 at 9:56 AM Damon Douglas <da...@apache.org>
> wrote:
>
>> Hello Everyone,
>>
>> *For those experienced with Beam*:
>>
>> PR/26413 [1] begins the long journey of addressing [2] which is a subtask
>> of [3].  千里之行，始於足下 :-)
>>
>> *For those beginning on the learning path*:
>>
>> The pull request referenced in the email is not directly related to Beam
>> but involves a Java annotation called SuppressWarnings.  Per its namesake,
>> it suppresses warnings according to the values you set in the annotation.
>> In [2] and [3]'s context the particular setting is "nullness" that
>> suppresses any warnings related to potentially null references in the code.
>>
>> In the Beam repository, we utilize The Checker Framework [4] to help
>> maintain a clean codebase.  When such annotation applies, it will
>> essentially mask these warnings and let the code pass without errors.  When
>> removing the SuppressWarnings with "nullness" annotation, the checks fail.
>>
>> Why is this a big deal?  The danger of nullness and the validity of the
>> warnings depend on the code context.  However, as of this writing there are
>> many Java classes in our repository with the SuppressWarnings annotation.
>> What is the likelihood they are masking something serious?
>>
>> Here in our discussion would be a great place to be practical and relay
>> what can be done when receiving a null-related error from the checker
>> framework.  However, I want to wait after I do more of these and find
>> validation through peer review.  Then, I will follow-up with the dev list.
>>
>> Meanwhile, you can take a look at my attempt in the PR [1].
>>
>> Best,
>>
>> Damon
>>
>> *References*:
>> 1. https://github.com/apache/beam/pull/26413
>> 2. https://github.com/apache/beam/issues/20498
>> 3. https://github.com/apache/beam/issues/20497
>> 4. https://checkerframework.org/
>>
>>
>>
*~Vincent*

Re: [20498] Remove SuppressWarnings nullness from SelectByteBuddyHelpers

[Kenneth Knowles <ke...@apache.org>]

This is great work. Thanks for writing about it.

I hold a stronger opinion than "the danger of nullness and the validity of
the warnings depend on the code context": nullness is a solved problem, and
type systems solved it, decades ago in some languages and more recently in
Java. Nullness errors are a choice, and we should not choose to have them.
Treat them as you would treat any other type error.

And I hold even strong opinions than that! Almost all the time, unclear
nullness/typing has an underlying cause of lack of clarity in design and
thinking. This is why fixing nullness almost always improves the code, but
also why it can be difficult to do after the fact, because it can require
fixing a bad or broken design. But sometimes it is easy, and you should do
it with gusto in either way :-)

And more! If checkerframework cannot determine the correctness of
something, probably neither can you. You may think you can, but you are
probably wrong. And even if you are correct today, the next person coming
along will not have your internal mental state and will easily break it.

As an example of both of the above: it can be very hard to fix API designs
like DoFn (or JUnit classes, etc) of the start/doSomething/end variety that
rely on a caller to invoke methods in a particular order and have
significant inter-method stateful dependencies that are not at all
guaranteed by their design. Such APIs are extremely fragile and the source
of a huge number of bugs because it is very easy and common to invoke
methods in an order or multiplicity that was not designed for. Not at all
coincidentally, it is quite hard and annoying to add the needed "Requires"
and "Ensures" annotations to get them to type check, and also very hard and
annoying to satisfy those requirements when calling the methods. This is
all a feature - the design was bad, and the difficulty setting up and
satisfying the invariants helps you realize it.

/lecture :-)

Kenn

On Tue, Apr 25, 2023 at 9:56 AM Damon Douglas <da...@apache.org>
wrote:

> Hello Everyone,
>
> *For those experienced with Beam*:
>
> PR/26413 [1] begins the long journey of addressing [2] which is a subtask
> of [3].  千里之行，始於足下 :-)
>
> *For those beginning on the learning path*:
>
> The pull request referenced in the email is not directly related to Beam
> but involves a Java annotation called SuppressWarnings.  Per its namesake,
> it suppresses warnings according to the values you set in the annotation.
> In [2] and [3]'s context the particular setting is "nullness" that
> suppresses any warnings related to potentially null references in the code.
>
> In the Beam repository, we utilize The Checker Framework [4] to help
> maintain a clean codebase.  When such annotation applies, it will
> essentially mask these warnings and let the code pass without errors.  When
> removing the SuppressWarnings with "nullness" annotation, the checks fail.
>
> Why is this a big deal?  The danger of nullness and the validity of the
> warnings depend on the code context.  However, as of this writing there are
> many Java classes in our repository with the SuppressWarnings annotation.
> What is the likelihood they are masking something serious?
>
> Here in our discussion would be a great place to be practical and relay
> what can be done when receiving a null-related error from the checker
> framework.  However, I want to wait after I do more of these and find
> validation through peer review.  Then, I will follow-up with the dev list.
>
> Meanwhile, you can take a look at my attempt in the PR [1].
>
> Best,
>
> Damon
>
> *References*:
> 1. https://github.com/apache/beam/pull/26413
> 2. https://github.com/apache/beam/issues/20498
> 3. https://github.com/apache/beam/issues/20497
> 4. https://checkerframework.org/
>
>
>