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

8231622: SuppressWarning("serial") ignored on field serialVersionUID #1626

Closed
wants to merge 5 commits into from

Conversation

lgxbslgx
Copy link
Member

@lgxbslgx lgxbslgx commented Dec 4, 2020

Hi all,

Currently, a warning is generated when compiling the following code by using the option -Xlint.

import java.io.Serializable;
class B implements Serializable {
    @SuppressWarnings("serial")
    private static final byte serialVersionUID = 5;
}

output:
B.java:4: warning: [serial] serialVersionUID must be of type long in class B
    private static final byte serialVersionUID = 5;
                              ^
1 warning

The annotation @SuppressWarnings("serial") on field serialVersionUID is ignored by the compiler.
This patch fixes it and adds some tests.
Thank you for taking the time to review.

Best Regards.


Progress

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

Issue

  • JDK-8231622: SuppressWarning("serial") ignored on field serialVersionUID

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1626/head:pull/1626
$ git checkout pull/1626

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 4, 2020

👋 Welcome back lgxbslgx! 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.

@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 4, 2020
@openjdk
Copy link

openjdk bot commented Dec 4, 2020

@lgxbslgx The following labels will be automatically applied to this pull request:

  • build
  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added build build-dev@openjdk.org compiler compiler-dev@openjdk.org labels Dec 4, 2020
@lgxbslgx lgxbslgx changed the title 8231622: SuppressWarning(serial) ignored on field serialVersionUID 8231622: SuppressWarning("serial") ignored on field serialVersionUID Dec 4, 2020
@lgxbslgx
Copy link
Member Author

lgxbslgx commented Dec 4, 2020

/label remove build

@openjdk openjdk bot removed the build build-dev@openjdk.org label Dec 4, 2020
@openjdk
Copy link

openjdk bot commented Dec 4, 2020

@lgxbslgx
The build label was successfully removed.

@mlbridge
Copy link

mlbridge bot commented Dec 4, 2020

Webrevs

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

Choose a reason for hiding this comment

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

For tests that use /ref=file -XDrawDiagnostics the conventions are:

  1. Omit the complete legal header, including the copyright and license
  2. After @test add the text /nodynamiccopyright/

#1 protects the file against any future changes in the length of the legal header, that might affect line numbers, and #2 is for use by automated scripts that may check for the presence of the legal header.

@lgxbslgx
Copy link
Member Author

lgxbslgx commented Dec 4, 2020

Thank you for your comment. I revise my code according to your suggestion.

And I found some previous tests use the wrong style which are like this patch. I want to modify them so that these tests won't mislead other developers, especially the new developers. Could I get your help to open a new issue about it in the bug tracker? Thanks a lot.

@jonathan-gibbons
Copy link
Contributor

Thank you for your comment. I revise my code according to your suggestion.

And I found some previous tests use the wrong style which are like this patch. I want to modify them so that these tests won't mislead other developers, especially the new developers. Could I get your help to open a new issue about it in the bug tracker? Thanks a lot.

Can you give me a list of tests that you think need to be addressed?

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

Choose a reason for hiding this comment

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

I approve these changes.

That being said, a more modern idiom for negative tests involving small source files and corresponding small golden files, is to write a single file that uses library code like toolbox.ToolBox to write files on the fly, perhaps from content in a text block, and to compare the output against another text block. For slightly more advanced usage, you can compile strings directly using API like SimpleJavaFileObject, and avoid the file system altogether.

@openjdk
Copy link

openjdk bot commented Dec 7, 2020

@lgxbslgx 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:

8231622: SuppressWarning("serial") ignored on field serialVersionUID

Reviewed-by: jjg

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 153 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jonathan-gibbons) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 7, 2020
@lgxbslgx
Copy link
Member Author

lgxbslgx commented Dec 8, 2020

Can you give me a list of tests that you think need to be addressed?

I write a python script to help me to find the possible wrong style tests. The script finds string Copyright, ref= and -XDrawDiagnostics in the files of directory test. If a file contains these three strings, it will print the full path name of the file to result.txt.

As you can see the entries in the annex result.txt, for example, the test/langtools/tools/doclint/multipackage/MultiPackage.java is not a wrong style test and test/langtools/tools/javac/AnonStaticMember_1.java is a wrong style test.
Later, I will check all the entries manually and revise the wrong style tests because I can't find a better way to do that.

The content of the python script is shown below. I don't attach it as an annex because GitHub doesn't support the python file type.

#!/usr/bin/python
import os


def clean(output_file):
    if os.path.exists(output_file):
        os.remove(output_file)


def outputFileName(full_path, output_file):
    output_file = open(output_file, 'a')
    output_file.write(full_path)
    output_file.write("\n")
    output_file.close()


def findStrListInTestFile(full_path, find_str_list):
    search_file = open(full_path, 'r')
    content = search_file.read()
    search_file.close()
    for find_str in find_str_list:
        is_found = content.find(find_str)
        if is_found == -1:
            return False
    return True


def findWrongStyleTests(find_dir, find_str_list, output_file):
    for dir_path, dir_names, file_names in os.walk(find_dir):
        for file_name in file_names:
            full_path = os.path.join(dir_path, file_name)
            is_found = findStrListInTestFile(full_path, find_str_list)
            if is_found:
                outputFileName(full_path, output_file)


if __name__ == "__main__":
    findDir = "test"
    findStrList = ["Copyright", "ref=", "-XDrawDiagnostics"]
    outputFile = "result.txt"
    clean(outputFile)
    findWrongStyleTests(findDir, findStrList, outputFile)

The steps to run the script are shown below.

  • Enter the jdk main directory: cd path/to/jdk
  • Create a new python script: vim test.py
  • Copy the code above to the file test.py
  • Use python command to run the script: python test.py

The file result.txt is shown below.
result.txt

Anyway, I don't think we should continue to discuss the concrete content in this patch.

@lgxbslgx
Copy link
Member Author

lgxbslgx commented Dec 8, 2020

@jonathan-gibbons I revise the test code by using the library toolbox. Thank you for taking the time to review.

@lgxbslgx
Copy link
Member Author

lgxbslgx commented Dec 8, 2020

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 8, 2020
@openjdk
Copy link

openjdk bot commented Dec 8, 2020

@lgxbslgx
Your change (at version 752ccba) is now ready to be sponsored by a Committer.

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

Choose a reason for hiding this comment

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

Nice improvement. You can make it even better in 3 places by using ToolBox.checkEquals, which provides more detailed reporting in the case of a difference being found.

Comment on lines 77 to 79
if (!Objects.equals(output, expected)) {
throw new AssertionError("incorrect output\nactual=" + output + "\nexpected=" + expected);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

ToolBox provides checkEquals which provides a more detailed error message in the case of a difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. I fixed it.

@openjdk openjdk bot removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Dec 8, 2020
@lgxbslgx
Copy link
Member Author

Anyway, I don't think we should continue to discuss the concrete content in this patch.

@jonathan-gibbons I submit a patch[1] to solve the previous bad style tests. We can continue the discussion there.

[1] #1732

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

Choose a reason for hiding this comment

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

Latest change looks good

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 17, 2020
@lgxbslgx
Copy link
Member Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 17, 2020
@openjdk
Copy link

openjdk bot commented Dec 17, 2020

@lgxbslgx
Your change (at version 8714ccf) is now ready to be sponsored by a Committer.

@lgxbslgx
Copy link
Member Author

Could I ask your help to sponsor this patch? Thanks a lot.

@jonathan-gibbons
Copy link
Contributor

/sponsor

@openjdk openjdk bot closed this Dec 21, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 21, 2020
@openjdk
Copy link

openjdk bot commented Dec 21, 2020

@jonathan-gibbons @lgxbslgx Since your change was applied there have been 210 commits pushed to the master branch:

  • a06cea5: 8258396: SIGILL in jdk.jfr.internal.PlatformRecorder.rotateDisk()
  • d234388: Merge
  • e680ebe: 8258007: Add instrumentation to NativeLibraryTest
  • c04c7e1: 8258002: Update "type" terminology in generated docs
  • 45bd3b9: 8223607: --override-methods=summary ignores some signature changes
  • 59ae054: 8258687: Build broken on Windows after fix for JDK-8258134
  • 1cc98bd: 8256693: getAnnotatedReceiverType parameterizes types too eagerly
  • 1ce2e94: 8256843: [PPC64] runtime/logging/RedefineClasses.java fails with assert: registers not saved on stack
  • 45a150b: 8258134: assert(size == calc_size) failed: incorrect size calculation on x86_32 with AVX512 machines
  • 38593a4: 8257974: Regression 21% in DaCapo-lusearch-large after JDK-8236926
  • ... and 200 more: https://git.openjdk.java.net/jdk/compare/cef606feca805c614004357ca691418521f207e1...master

Your commit was automatically rebased without conflicts.

Pushed as commit 01d51a1.

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

@lgxbslgx lgxbslgx deleted the JDK-8231622 branch December 22, 2020 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler compiler-dev@openjdk.org integrated Pull request has been integrated
2 participants