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
