<Swing Dev> [10] RFR: 8182577: Crash when Tab key moves focus to a JCheckbox with a custom ButtonModel

[Luke]

Hi Semyon,

On 29/06/2017 02:51, Semyon Sadetsky wrote:
> Hello Luke,
>
> DefaultButtonModel ::getGroup has never been being required to return 
> not-null. Can you clarify which new pitfall is introduced by the fix, 
> so that we should specify it explicitly?
>
> --Semyon

Code using a ButtonModel might assume that after calling setGroup that 
the same instance would thereafter be returned by getGroup. However any 
code taking advantage of getGroup having being "pulled up" into 
ButtonModel should not make that natural assumption, as a legacy 
ButtonModel may still return null.

So this forms part of the method's contract for the caller too. If the 
caller is expected to read and understand the implications of the 
implSpec, then what is written already may be sufficient. I guess that's 
the question.

How about these changes (based on webrev.05)?

      /**
       * Returns the group that the button belongs to.
       * Normally used with radio buttons, which are mutually
       * exclusive within their group.
       *
       * @implSpec The default implementation of this method returns {@code null}.
-     * Subclasses should return the group set by setGroup() to avoid NPE.
+     * Subclasses should return the group set by setGroup().
       *
-     * @return the <code>ButtonGroup</code> that the button belongs to
+     * @return the <code>ButtonGroup</code> that the button belongs to, if any.
+     * Null may also be returned where a legacy ButtonGroup inherits the
+     * default implementation.
       * @since 10
       */
      default ButtonGroup getGroup() {
          return null;
      }

I think this makes the contract more explicit.

Kind regards,
Luke

> On 06/28/2017 08:29 AM, Luke wrote:
>> Hi Semyon,
>>
>> On 28/06/2017 16:59, Semyon Sadetsky wrote:
>>>
>>>>
>>>> On one hand, it seems the best default methods are the ones able to 
>>>> produce correct behaviours w.r.t. the other interface methods when 
>>>> retrofitted, to avoid any 'surprises'. For example if some other 
>>>> code decides to switch from DefaultButtonModel to accept any 
>>>> ButtonModel - that would happily compile but it might be easy to 
>>>> miss the changed semantics: that calling getGroup may not 
>>>> necessarily give back a value passed in to setGroup any more, and 
>>>> hence there's scope for a subtle runtime crash bug to be introduced.
>>> Not sure that I fully understood this. According to the spec there 
>>> is the only group the button belongs to, and this group should be 
>>> returned in getGroup(). 
>>
>> The spec can of course only apply to code written in Java 10+, legacy 
>> implementations ButtonModel will still return null.
>>
>> So say a helper function in a shared library does an "instanceof 
>> DefaultButtonModel" check, and that it calls both 
>> setGroup(ButtonGroup) and getGroup() - and for whatever reason pulls 
>> the group back out rather than storing it in a local variable.
>>
>> When porting that library to Java 10, a programmer might think 
>> "great, now I can support all ButtonModels" and drop the instanceof 
>> check.
>>
>> Some other app not yet ported to Java 10 passes that utility method a 
>> custom ButtonModel, as it has always done. Instead of the utility 
>> function doing nothing with it, it now throws a NPE.
>>
>> I just raised this because I know how ultra-concerned the JDK is with 
>> compatibility - but even by the JDK's high standards it does still 
>> seem like a fairly theoretical-only issue.
>>
>> Is the @implSpec already sufficient warning to a programmer porting 
>> code to Java 10 to always deal with the null possibility? Perhaps it is.
>>
>> Kind regards,
>> Luke
>>
>