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

[Guoxiong Li]

On Mon, 30 Nov 2020 12:33:15 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> 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.

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

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

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