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

8257037: No javac warning when calling deprecated constructor with diamond #1490

Closed
wants to merge 5 commits into from

Conversation

@lgxbslgx
Copy link
Member

@lgxbslgx lgxbslgx commented Nov 28, 2020

Hi all,

When calling deprecated constructor with diamond, the compiler doesn't output warning.
The test case is shown below.

GenericClass<Object> o2 = new GenericClass<>();

public class GenericClass<T> {
    @Deprecated
    public GenericClass() {}
}

This patch solves the bug and adds corresponding test case.
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-8257037: No javac warning when calling deprecated constructor with diamond

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 28, 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.

Loading

@openjdk openjdk bot added the rfr label Nov 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 28, 2020

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

  • compiler

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

Loading

@openjdk openjdk bot added the compiler label Nov 28, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 28, 2020

Loading

@lgxbslgx lgxbslgx closed this Nov 28, 2020
@lgxbslgx lgxbslgx deleted the 8257037 branch Nov 28, 2020
@lgxbslgx lgxbslgx restored the 8257037 branch Nov 28, 2020
@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Nov 28, 2020

Sorry for closing the PR accidentally.

Loading

@lgxbslgx lgxbslgx reopened this Nov 28, 2020
Copy link
Contributor

@mcimadamore mcimadamore left a comment

I think the fix seems in spirit with respect to what happens for ordinary constructor resolution. I guess there is a question as to whether we should check for these warnings during resolution or after (probably checking for warning after resolution might be a better idea) - as there is some duplication between the code in findConstructor and the new findDiamond and what goes on in Attr::checkId which also check for these warnings (and some more) but doesn't apply checks on constructors.

Just for fun, I tried this patch:

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
index df21524229c..05d56136000 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
@@ -4459,15 +4459,11 @@ public class Attr extends JCTree.Visitor {
             }
 
             // Emit a `deprecation' warning if symbol is deprecated.
-            // (for constructors (but not for constructor references), the error
-            // was given when the constructor was resolved)
 
-            if (sym.name != names.init || tree.hasTag(REFERENCE)) {
-                chk.checkDeprecated(tree.pos(), env.info.scope.owner, sym);
-                chk.checkSunAPI(tree.pos(), sym);
-                chk.checkProfile(tree.pos(), sym);
-                chk.checkPreview(tree.pos(), sym);
-            }
+            chk.checkDeprecated(tree.pos(), env.info.scope.owner, sym);
+            chk.checkSunAPI(tree.pos(), sym);
+            chk.checkProfile(tree.pos(), sym);
+            chk.checkPreview(tree.pos(), sym);
 
             // If symbol is a variable, check that its type and
             // kind are compatible with the prototype and protokind.
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
index 727d2989adf..9bb37fc23b8 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
@@ -2859,8 +2859,6 @@ public class Resolve {
                                     names.init, argtypes,
                                     typeargtypes, allowBoxing,
                                     useVarargs);
-        chk.checkDeprecated(pos, env.info.scope.owner, sym);
-        chk.checkPreview(pos, sym);
         return sym;
     }
 
@@ -2937,6 +2935,7 @@ public class Resolve {
                             return sym;
                         }
                     };
+                    newConstr.setAttributes(sym);
                     bestSoFar = selectBest(env, site, argtypes, typeargtypes,
                             newConstr,
                             bestSoFar,

This fixes your issues, and removes the warning code duplication between Attr and Resolve, as well as adding checks for all the remaining kinds of warnings (sunAPI, deprecation, preview and profile) - although I believe most of those are not as important.

I guess I'll leave you to decide how you want to tackle this. As I said, the patch you wrote remains within the spirit of the original code - my only minor quibble is that this "spirit" is questionable, as it involves issuing same warnings in different places, this enforcing developers to replicate code (and forgetting to do so, as it already happened) in multiple places.

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Nov 30, 2020

Thank you for the comment and suggestion.

I had seen and paid attention to the following code in Attr.java when being solved this bug.

            // (for constructors (but not for constructor references), the error
            // was given when the constructor was resolved)

            if (sym.name != names.init || tree.hasTag(REFERENCE)) {
                chk.checkDeprecated(tree.pos(), env.info.scope.owner, sym);
                chk.checkSunAPI(tree.pos(), sym);
                chk.checkProfile(tree.pos(), sym);
                chk.checkPreview(tree.pos(), sym);
            }

These code confused me. Because the comments (for constructors, the error was given when the constructor was resolved) and the conditional expression sym.name != names.init were submitted at 2007(submit log: Initial load). And nobody raises suggestion to it over these years though this code snippet had been revised many times.

In order not to generate unknown issues, I chose not to revise these code when being solved this bug.

@mcimadamore I test your patch locally by using the following command. All the tests passed.

make test TEST="jtreg:langtools/tools/javac" CONF=linux-x86_64-server-release

Your patch is a better solution if no any unknown regression occurs. But I have no idea about the unknown regressions now.
I would like to accept your suggestion and revise my code. And we should think more situations which could generate regressions to avoid unknown issues occur.


Finally, I have a little question.
Is the code newConstr.setAttributes(sym); necessary in Resolve.findDiamond?
I delete it in your patch, and test locally by using the following command. All the tests passed.

make test TEST="jtreg:langtools/tools/javac" CONF=linux-x86_64-server-release

Loading

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Dec 1, 2020

Is the code newConstr.setAttributes(sym); necessary in Resolve.findDiamond?

Sorry, that was a leftover from a previous experiment. That's not necessary. We'll run some more tests on our side and I'll let you know.

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Dec 2, 2020

@mcimadamore Should I add you as a contributor of this patch? Does it need another reviewer to review if I add you as a contributor?

Loading

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Dec 2, 2020

@mcimadamore Should I add you as a contributor of this patch? Does it need another reviewer to review if I add you as a contributor?

As you prefer - I don't mind either way. I don't think adding me as a contributor should prevent the integration, or me being able to sponsor, but I'm not 100% sure.

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Dec 2, 2020

/contributor add @mcimadamore

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2020

@lgxbslgx
Contributor Maurizio Cimadamore <mcimadamore@openjdk.org> successfully added.

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Dec 4, 2020

I revise the test code based on the comment[1] written by Jonathan Gibbons in JDK-8231622[2].

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.

[1] https://mail.openjdk.java.net/pipermail/compiler-dev/2020-December/015542.html
[2] #1626

Loading

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Dec 7, 2020

The extensive testing revealed at least 10 cases where the compiler is issuing a new, resolution-related error when it wasn't doing so before - (the errors seem to have to do with abstract methods). As a result I think it's best to go with your first conservative approach, to avoid troubles. I'll try to investigate further and come up with a reduced test case for the failing condition - but that doesn't have to hold up this fix. Thanks!

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Dec 7, 2020

It is a good news that the trouble is found before the integration instead of after the release of JDK16.
I revise the code to previous version.
Thank your for the review.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

@lgxbslgx Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Dec 7, 2020

/contributor remove @mcimadamore

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

@lgxbslgx
Contributor Maurizio Cimadamore <mcimadamore@openjdk.org> successfully removed.

Loading

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Dec 7, 2020

Actually, after some more analysis, I discovered that many of the errors were unrelated with your fix - and had to do instead with the addition of a new default method on Stream, which is causing overload resolution changes in code that has static imports on Collectors.

Only one issue remains to be fully triaged - but, being annotation related, I think it's unlikely caused by your patch. In other words, I believe both patches are likely to be safe. I'm obviously ok with you going ahead with the more conservative fix, since you already reverted the code to that version.

I apologize for the back and forth!

Loading

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Looks good to go

Loading

@openjdk
Copy link

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

8257037: No javac warning when calling deprecated constructor with diamond

Reviewed-by: mcimadamore

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

  • 637b0c6: 8246778: Compiler implementation for Sealed Classes (Second Preview)
  • 09707dd: 8252807: The jdk.jfr.Recording.getStream does not work when toDisk is disabled
  • 04ce8e3: 8257184: Upstream 8252504: Add a method to MemoryLayout which returns a offset-computing method handle
  • 5a03e47: 8255560: Class::isRecord should check that the current class is final and not abstract
  • 8e8e584: 8257588: Make os::_page_sizes a bitmask
  • 566d77a: 8254802: ThrowingPushPromisesAsStringCustom.java fails in "try throwing in GET_BODY"
  • f5a582c: 8257575: C2: "failed: only phis" assert failure in loop strip mining verification
  • d05401d: 8256679: Update serialization javadoc once JOSS changes for records are complete
  • 7620124: 8257230: assert(InitialHeapSize >= MinHeapSize) failed: Ergonomics decided on incompatible initial and minimum heap sizes
  • 05dac03: 8257803: Add -Xbatch to compiler/blackhole tests
  • ... and 141 more: https://git.openjdk.java.net/jdk/compare/1241f800023996d33b39a2b881b2bf7e3b7201c3...master

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 (@mcimadamore) 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).

Loading

@openjdk openjdk bot added the ready label Dec 7, 2020
@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Dec 7, 2020

/integrate

Loading

@openjdk openjdk bot added the sponsor label Dec 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

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

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 7, 2020

Mailing list message from Remi Forax on compiler-dev:

oops,
you mean Collectors.toList() vs Stream.toList()

R?mi

----- Mail original -----

De: "Maurizio Cimadamore" <mcimadamore at openjdk.java.net>
?: "compiler-dev" <compiler-dev at openjdk.java.net>
Envoy?: Lundi 7 D?cembre 2020 13:18:16
Objet: Re: RFR: 8257037: No javac warning when calling deprecated constructor with diamond [v2]

Loading

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Dec 7, 2020

you mean Collectors.toList() vs Stream.toList()

yes - precisely

Loading

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Dec 7, 2020

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

@mcimadamore Only the author (@lgxbslgx) is allowed to issue the integrate command. As this PR is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the /sponsor command?

Loading

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Dec 7, 2020

/sponsor

Loading

@openjdk openjdk bot closed this Dec 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

@mcimadamore @lgxbslgx Since your change was applied there have been 154 commits pushed to the master branch:

  • 46b35ac: 8257798: [PPC64] undefined reference to Klass::vtable_start_offset()
  • ecd7e47: 8257793: Shenandoah: SATB barrier should only filter out already strongly marked oops
  • e08b9ed: 8257820: Remove gc/ergonomics/TestMinHeapSize.java as it is too brittle
  • 637b0c6: 8246778: Compiler implementation for Sealed Classes (Second Preview)
  • 09707dd: 8252807: The jdk.jfr.Recording.getStream does not work when toDisk is disabled
  • 04ce8e3: 8257184: Upstream 8252504: Add a method to MemoryLayout which returns a offset-computing method handle
  • 5a03e47: 8255560: Class::isRecord should check that the current class is final and not abstract
  • 8e8e584: 8257588: Make os::_page_sizes a bitmask
  • 566d77a: 8254802: ThrowingPushPromisesAsStringCustom.java fails in "try throwing in GET_BODY"
  • f5a582c: 8257575: C2: "failed: only phis" assert failure in loop strip mining verification
  • ... and 144 more: https://git.openjdk.java.net/jdk/compare/1241f800023996d33b39a2b881b2bf7e3b7201c3...master

Your commit was automatically rebased without conflicts.

Pushed as commit 2c04fc0.

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

Loading

@lgxbslgx lgxbslgx deleted the 8257037 branch Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants