<AWT Dev> <Beans Dev> JDK 14 RFR of JDK-8231334: Suppress warnings on non-serializable instance fields in client libs serializable classes

[Joe Darcy]

Hi Phil,

I've taken another look over the classes modified in this patch. Most of 
the classes define neither a readObject nor writeObject method and thus 
use the default serialization mechanism of reading/writing all the 
non-transient instance fields (as long as serialPersistentFields is 
absent, etc.). Most of the rest call 
defaultReadObject/defaultWriteObject wrapped in some light supporting 
logic in a readObject/writeObject method. In these cases, all the 
non-transient instance fields and read/written as well.

The only class which doesn't directly or indirectly use the default 
serialization mechanism is java.awt.Container, which uses putFields in 
its writeObject method. I've filed JDK-8231437: "Review serial fields of 
java.awt.Container" to track the follow-up work.

Thanks for the review,

-Joe

On 9/24/2019 4:07 PM, Philip Race wrote:
> OK. Approved by me .. but .. it would be good if you can point out any 
> other cases
> where you think we can compatibly make changes to get rid of the need 
> for suppressing this new warning.
>
> -phil
>
> On 9/23/19, 12:54 PM, Joe Darcy wrote:
>>
>> Hi Phil,
>>
>> On 9/22/2019 1:25 PM, Philip Race wrote:
>>>
>>> + @SuppressWarnings("serial") // Not statically typed as 
>>> Serializable So is the comment being used to distinguish this 
>>> overloading of what "serial" means ? Why not introduce a new warning 
>>> category ?
>>
>>
>> The -Xlint:serial check in javac has had an operational meaning of 
>> "does a serializable type define a serialVersionUID?" The 
>> work-in-progress is aiming to expand this to cover other aspect of 
>> declaring serializable (and externalizable) types.
>>
>> It would be possible to put the new checks in their own category, but 
>> that would limit their use and some of new checks find what are most 
>> likely semantic errors, such as declaring a serialVersionUID in an 
>> enum type, which gets silently ignored.
>>
>>
>>> Randomly looking at
>>> ====
>>> src/java.desktop/share/classes/java/awt/Container.java
>>>
>>> @@ -3849,10 +3849,11 @@
>>>
>>>          /**
>>>           * The handler to fire {@code PropertyChange}
>>>           * when children are added or removed
>>>           */
>>> +        @SuppressWarnings("serial") // Not statically typed as 
>>> Serializable
>>>          protected ContainerListener accessibleContainerHandler = null;
>>> ===
>>>
>>> I see that Container has a writeObject method which doesn't write 
>>> this field, so it is effectively transient.
>>>
>>> I recognise that it is difficult for the compiler to figure this 
>>> out, so perhaps there should just be a policy
>>> not to check classes that have writeObject methods ?
>>
>>
>> Yes, it is not feasible for this level of analysis to decode the 
>> semantics of writeObject and related methods. The analysis does skip 
>> over classes using serialPersistentFields, which is an alternative 
>> mechanism to indicate which fields are used for serialization.
>>
>> In terms of possible false positives, I think it is acceptable to 
>> keep doing the checks in the presence of a writeObject method since a 
>> writeObject can be used to make alterations to serialization process 
>> other than changing the set of fields written out.
>>
>>
>>>
>>> Also in such a case, would it be an effectively compatible change to 
>>> add transient to the field, so that
>>> it presumably would no longer need this warning.
>>
>> And the class does define a serialVersionUID so adding transient to 
>> the field should preserve serial compatibility.
>>
>>
>>>
>>> I haven't looked but presumably there could be other such cases.
>>>
>>> Will you be filing bugs for all the fixable cases ?
>>
>> Someone should ;-)
>>
>> To make sure my intentions are clear, nothing in this overall cleanup 
>> effort should be construed as seeking to assume ownership of all the 
>> serialization in the JDK. The primary ownership will remain with the 
>> component team in question. The new checks are meant to the an aid, 
>> especially to writing new serializable types, while also prompting 
>> some examination of the existing types in an effort to allow the 
>> warning to enabled by default  in the build.
>>
>> Thanks,
>>
>> -Joe
>>
>>
>>>
>>> -phil
>>>
>>> On 9/21/19, 12:48 PM, Joe Darcy wrote:
>>>>
>>>> Hello,
>>>>
>>>> Quick background, I'm working on expanding the compile-time 
>>>> serialization checks of javac's -Xlint:serial option. Ahead of that 
>>>> work going back, I'm analyzing the JDK sources and plan to 
>>>> pre-suppress the coming-soon new warnings, fixing or at least 
>>>> filing follow-up bugs for any problems that are found. 
>>>> Corresponding suppression bugs are already out for review against 
>>>> core libs (JDK-8231202) and security libs (JDK-8231262).
>>>>
>>>> The new check in development is if a serializable class has an 
>>>> instance field that is not declared to be a serializable type. This 
>>>> might actually be fine in practice, such as if the field in 
>>>> question always points to a serializable object at runtime, but it 
>>>> is arguably worth noting as an item of potential concern. This 
>>>> check is skipped if the class using the serialPersistentFields 
>>>> mechanism.
>>>>
>>>> For the client libs, the webrev with the new @SuppressedWarnings 
>>>> annotations is:
>>>>
>>>>     JDK-8231334: Suppress warnings on non-serializable instance 
>>>> fields in client libs serializable classes
>>>> http://cr.openjdk.java.net/~darcy/8231334.0/
>>>>
>>>> The changes are mostly in awt, but also some in beans, a few in 
>>>> printing, and one in sound.
>>>>
>>>> As discussed with Phil off-line, the new checks also found an 
>>>> existing known issue, the auxiliary class java.awt.ImageMediaEntry 
>>>> declared in MediaTracker.java is not serializable/deserializable in 
>>>> practice (JDK-4397681).
>>>>
>>>> Thanks,
>>>>
>>>> -Joe
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/awt-dev/attachments/20190924/8fa25729/attachment-0001.html>