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

[Semyon Sadetsky]

Hi Luke,

I think a newly created method that accepts the ButtonModel instance 
argument and calls setGroup/getGroup on it should always check the 
getGroup() return for null before using it just from general 
consideration because the real implementation is unknown. Your 
suggestion to highlight the legacy implementation as a possible source 
for NPE seems unreasonable in that it suggests an assumption that after 
ButtonModel implementations cannot return null from getGroup(), which is 
not true.

--Semyon

On 06/29/2017 05:50 AM, Luke wrote:

> 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
>>>
>>
>