Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields #5709

Closed
wants to merge 86 commits into from

Conversation

@jddarcy
Copy link
Member

@jddarcy jddarcy commented Sep 27, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

  • JDK-8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields
  • JDK-8160675: Issue lint warning for non-serializable non-transient instance fields in serializable type

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5709/head:pull/5709
$ git checkout pull/5709

Update a local copy of the PR:
$ git checkout pull/5709
$ git pull https://git.openjdk.java.net/jdk pull/5709/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5709

View PR using the GUI difftool:
$ git pr show -t 5709

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5709.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 27, 2021

👋 Welcome back darcy! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 14, 2021

I updated the PR to implement a refined check on the type of fields of a serializable class: if the type of the field is an array, if the base component type of an array is not serializable, a warning is issued. For example, both Object[] and Object[][] would generate warnings despite the JLS mandating array types are serializable. When an array is serialized, eventually its elements are serialized so a non-serializable component type is worth mentioning.

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 18, 2021

From an off-list discussion with Roger and Stuart, I've changed the checks to not warm about the presence of a serialVersionUID field in an interface. There are specialized situations where the serialVersionUID of an interface are used.

Copy link
Contributor

@lahodaj lahodaj left a comment

Looks reasonable to me. Left a few comments inline.

class ImproperSerialPF implements Serializable {
// Proper declaration of serialPersistentFields is:
// private static final ObjectStreamField[] serialPersistentFields = ...
public /*instance*/ Object serialPersistentFields = Boolean.TRUE;
Copy link
Contributor

@lahodaj lahodaj Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to have tests that would verify every check made for the field - i.e. taking a field named "serialPersistentFields", which should be (I guess) private, static, final, and have type ObjectStreamField[], and have a series of fields that fulfill all these properties except one, and verify the warning is produced. The test could be a some sort of a combo test, or not, that is less important, I think.

Copy link
Member Author

@jddarcy jddarcy Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged; this consider this for a future refinement.

@@ -29,6 +29,16 @@
import java.util.function.BiConsumer;

import javax.lang.model.element.ElementKind;
import javax.lang.model.element.Element;
Copy link
Contributor

@lahodaj lahodaj Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these new imports are probably unused.

Copy link
Member Author

@jddarcy jddarcy Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned up imports in subsequent push; thanks.

Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment

looks good to me

@openjdk openjdk bot removed the csr label Oct 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

@jddarcy This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: erikj, sspitsyn, jlahoda, vromero, rriggs, smarks

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 21, 2021
@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 21, 2021

/reviewer credit rriggs

@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

@jddarcy
Reviewer rriggs successfully credited.

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 21, 2021

/reviewer credit smarks

@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

@jddarcy
Reviewer smarks successfully credited.

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 21, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

Going to push as commit 6a466fe.
Since your change was applied there has been 1 commit pushed to the master branch:

  • 4e9dd4b: 8275384: Change nested classes in jdk.jconsole to static nested classes

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 21, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

@jddarcy Pushed as commit 6a466fe.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@jddarcy jddarcy deleted the JDK-8202056 branch Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants