RFR: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields [v5]

[Joe Darcy]

On Wed, 29 Sep 2021 23:44:55 GMT, Joe Darcy <darcy at openjdk.org> wrote:

>> This is an initial PR for expanded lint warnings done under two bugs:
>> 
>> 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields
>> 8160675: Issue lint warning for non-serializable non-transient instance fields in serializable type
>> 
>> to get feedback on the general approach and test strategy before further polishing the implementation.
>> 
>> The implementation initially started as an annotation processor I wrote several years ago. The refined version being incorporated into Attr has been refactored, had its checks expanded, and been partially ported to idiomatic javac coding style rather than using the javax.lang.model API from annotation processing.
>> 
>> Subsequent versions of this PR are expected to move the implementation closer to idiomatic javac, in particular to use javac flags rather than javax.lang.model.Modifier's. Additional resources keys will be defined for the serialization-related fields and methods not having the expected modifiers, types, etc. The resource keys for the existing checks related to serialVersionUID and reused.
>> 
>> Please also review the corresponding CSRs:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8274335
>> https://bugs.openjdk.java.net/browse/JDK-8274336
>> 
>> Informative serialization-related warning messages must take into account whether a class, interface, annotation, record, and enum is being analyzed. Enum classes and record classes have special handling in serialization. This implementation under review has been augmented with checks for interface types recommended by Chris Hegarty in an attachment on 8202056.
>> 
>> The JDK build has the Xlint:serial check enabled. The build did not pass with the augmented checks. For most modules, this PR contains the library changes necessary for the build to pass. I will start separate PRs in those library areas to get the needed SuppressWarning("serial") or other changes in place. For one module, I temporarily disabled the Xlint:serial check.
>> 
>> In terms of performance, I have not done benchmarks of the JDK build with and without these changes, but informally the build seems to take about as long as before.
>
> Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 36 commits:
> 
>  - Merge branch 'master' into JDK-8202056
>  - Fix whitespace.
>  - Merge branch 'master' into JDK-8202056
>  - Merge branch 'master' into JDK-8202056
>  - Merge branch 'master' into JDK-8202056
>  - Restore serial checks on java.xml module.
>  - Merge branch 'master' into JDK-8202056
>  - Appease jcheck
>  - Implement checks chegar recommended for interfaces.
>  - Update comment.
>  - ... and 26 more: https://git.openjdk.java.net/jdk/compare/355356c4...33504e85

Responding to some off-list comments, I wanted to provide some additional framing and context for this PR.

Serialization is a tricky area to get right. Augmented compile-time checks can help catch code with unexpected (and unwanted) semantics. This existing javac Xlint:serial checks help in this regard with the serialVersionUID field. Before the compile-time checks were enabled in the build, there were a handful of unintentionally introduced serial incompatibilities we had to retroactively fix. Also before the check was in the build, when I did a code review or ccc/CSR review of a Serializable type (which includes all exceptions!), I'd try to remember to check for the declaration of a serialVersionUID. I did catch a few omissions, but I much prefer the current situation where javac will always remember to check and developers don't have to worry about that detail anymore. 

The other fields and methods used in serialization can also be mis-declared in various ways. The proposed checks examine fields and methods in serializable types whose names match names of fields and methods, respectively, used in serialization. The declarations are then examined for well-formedness: 

* are all required modifiers present
* are inappropriate modifiers absent
* for fields, is the correct type used
* for methods are the return type, type and number of parameters, and throws clause correct

A warning will be generated if any of these aspects is mismatched.

Also when reviewing a serializable type , I think it worth noting if any of the non-transient instance fields -- the fields whose values will be attempted to be serialized by default -- are not declared to have serializable types. Checking for that condition is the subject of JDK-8160675, also out for review under this PR. Having such fields isn't *necessary* a problem. Dynamically all the objects pointed to by the field may actually be serializable, a that fact is not captured in type system. Or the type may be conditionally serializable based on if its field values are, as commonly found in collections.  However, it could also be an actual oversight worth correcting or at least noting in a comment as an intentional design.

Assuming a form of this PR gets integrated, there are several possible extensions:

* Checking the bodies of readObject and writeObject for one of the mandated method calls (JDK-7019074)
* Checking that the java.io.Serial annotation is only applied to serialization-related fields and methods. Assuming the other checks on structural well-formedness are in place, this additional check is straightforward to implement.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5709