RFR: 8255968: Confusing error message for inaccessible constructor [v2]

[Guoxiong Li]

On Tue, 24 Nov 2020 12:00:50 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Currently, the compiler output the error message depending on the order of the methods. Please see the below test cases.
>> 
>> - the `private Test(int x) {}` is at the end of other(one or more) method(s).
>> class T8255968 {
>>     Test c = new Test(0);
>> }
>> class Test {
>>     private Test(String[] x) {}
>>     Test(String x) {}
>>     private Test(int x) {}  // at the end
>> }
>> 
>> current output:
>> error: Test(int) has private access in Test
>>     Test c = new Test(0);
>>              ^
>> 1 error
>> - the `private Test(int x) {}` is not at the end of other(only one) method.
>> class T8255968 {
>>     Test c = new Test(0);
>> }
>> class Test {
>>     private Test(int x) {}  // not at the end
>>     Test(String x) {} // can be private, output same error message
>> }
>> 
>> current output:
>> error: incompatible types: int cannot be converted to String
>>     Test c = new Test(0);
>>                       ^
>> Note: Some messages have been simplified; recompile with -Xdiags:verbose to get full output
>> 1 error
>> 
>> - the `private Test(int x) {}` is not at the end of other(more than one) methods.
>> class T8255968 {
>>     Test c = new Test(0);
>> }
>> class Test {
>>     private Test(String x) {}
>>     private Test(int x) {}  // not at the end
>>     Test(String[] x) {}
>> }
>> 
>> current output:
>> error: no suitable constructor found for Test(int)
>>     Test c = new Test(0);
>>              ^
>>     constructor Test.Test(String) is not applicable
>>       (argument mismatch; int cannot be converted to String)
>>     constructor Test.Test(String[]) is not applicable
>>       (argument mismatch; int cannot be converted to String[])
>> 1 error
>> These output messages which depending on the order of the methods would confuse users. Because the order of the methods  should not have any influence.
>> 
>> @mcimadamore According to your comment, I want to unify the output messages as below.
>> 
>> - the `private Test(int x) {}` is at the end of other(one or more) method(s).
>> class T8255968 {
>>     Test c = new Test(0);
>> }
>> class Test {
>>     private Test(String[] x) {}
>>     Test(String x) {}
>>     private Test(int x) {}  // at the end
>> }
>> 
>> expected output:
>> error: no suitable constructor found for Test(int)
>>     Test c = new Test(0);
>>              ^
>>     constructor Test.Test(String[]) is not applicable
>>       (argument mismatch; int cannot be converted to String[])
>>     constructor Test.Test(int) is not applicable
>>       (Test(int) has private access in Test)
>>     constructor Test.Test(String) is not applicable
>>       (argument mismatch; int cannot be converted to String)
>> 1 error
>> - the `private Test(int x) {}` is not at the end of other(only one) method.
>> class T8255968 {
>>     Test c = new Test(0);
>> }
>> class Test {
>>     private Test(int x) {}  // not at the end
>>     Test(String x) {} // can be private, output same error message
>> }
>> 
>> expected output:
>> error: no suitable constructor found for Test(int)
>>     Test c = new Test(0);
>>              ^
>>     constructor Test.Test(int) is not applicable
>>       (Test(int) has private access in Test)
>>     constructor Test.Test(String) is not applicable
>>       (argument mismatch; int cannot be converted to String)
>> 1 error
>> 
>> - the `private Test(int x) {}` is not at the end of other(more than one) methods.
>> class T8255968 {
>>     Test c = new Test(0);
>> }
>> class Test {
>>     private Test(String x) {}
>>     private Test(int x) {}  // not at the end
>>     Test(String[] x) {}
>> }
>> 
>> expected output:
>> error: no suitable constructor found for Test(int)
>>     Test c = new Test(0);
>>              ^
>>     constructor Test.Test(String) is not applicable
>>       (argument mismatch; int cannot be converted to String)
>>     constructor Test.Test(int) is not applicable
>>       (Test(int) has private access in Test)
>>     constructor Test.Test(String[]) is not applicable
>>       (argument mismatch; int cannot be converted to String[])
>> 1 error
>> 
>> And the following two situations are not modified.
>> class T8255968 {
>>     Test c = new Test(0);
>> }
>> class Test {
>>     private Test(int x) {}
>> }
>> 
>> current and expected output:
>> error: Test(int) has private access in Test
>>     Test c = new Test(0);
>>              ^
>> 1 error
>> class T8255968 {
>>     Test c = new Test(0);
>> }
>> class Test {
>>     Test(String x) {}
>> }
>> 
>> current and expected output:
>> error: incompatible types: int cannot be converted to String
>>     Test c = new Test(0);
>>                       ^
>> Note: Some messages have been simplified; recompile with -Xdiags:verbose to get full output
>> 1 error
>> 
>> I implement this unified format and test the feature locally. @mcimadamore If you agree with these unified output messages, I would update my code for reviewing.
>
>> 
>> I implement this unified format and test the feature locally. @mcimadamore If you agree with these unified output messages, I would update my code for reviewing.
> 
> This seems a good direction - but there are few dragons in there. As I said in my previous message, inaccessible members are just not overload resolution candidates and are "skipped" over by the overload selection logic. This is visible in examples like these:
> 
> class Sup {
>    private void m(String s);
> }
> 
> class Outer {
>      void m() { ... }
>      void m(Integer s) { ... }
> 
>      class Inner extends Sup {
>           void test() {
>                m();
>            }
>       }
> }
> In this example, overload selection should look for `m` candidates inside the `Outer` class. If, with a patch, you make access errors more similar to `WRONG_MTH`, or `WRONG_MTHS` kinds (by unifying them) chances are that overload selection will start thinking that it found a candidate in `Sup` and stop there - which would be against the JLS. I'm not saying it cannot be done, but more that it has to be done in a careful way - that is, you can only unify the error messages in situation where you know there's a mix of inaccessible and inapplicable methods, so you know already that overload selection will stop at that class.
> 
> Another aspect to look out for/test is method references - whatever we say here for plain method calls, has to hold for method reference selection - so you'll want to create specific tests which aims at showing that whatever change is applied, it also improves the situation with method references (or at least does not cause unwanted regressions there).

I update the code which unifies the error messages. I use the following command to run test locally and all the test passed.
make test TEST="jtreg:langtools/tools/javac" CONF=linux-x86_64-server-release JOBS=8

output:
==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/langtools/tools/javac                   3503  3503     0     0   
==============================
TEST SUCCESS

Finished building target 'test' in configuration 'linux-x86_64-server-release'

Thank you for taking the time to review.

Best Regards.

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

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