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
