RFR: 8257037: No javac warning when calling deprecated constructor with diamond [v2]

[Maurizio Cimadamore]

On Sat, 28 Nov 2020 11:46:08 GMT, Guoxiong Li <github.com+13688759+lgxbslgx at openjdk.org> wrote:

>> 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.
>
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Polish

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.

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

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