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

8255968: Confusing error message for inaccessible constructor #1389

Closed
wants to merge 6 commits into from

Conversation

@lgxbslgx
Copy link
Contributor

@lgxbslgx lgxbslgx commented Nov 23, 2020

Hi all,

When using inaccessible constructor, compiler would output some confusing error message occasionally. These confusing error message is related to the constructor order. See JDK-8255968 for more information.
This patch solves this bug, regardless of the construction order, compiler can always output expected message.
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

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build (6/6 failed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Failed test tasks

Issue

  • JDK-8255968: Confusing error message for inaccessible constructor

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 23, 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 label Nov 23, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 23, 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.

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

@mlbridge mlbridge bot commented Nov 23, 2020

Copy link
Contributor

@mcimadamore mcimadamore left a comment

While I somewhat sympathize with the problem you are trying to solve, note that, by doing this, the output of the compiler will regress in case where not all the overload candidates are not accessible. E.g.

m(1): //client

m();
private m(int);

In this case I guess it's less clear as what "should" happen: does the user want to call the first, or second version of m ? Both cases are possible - e.g. the user might accidentally call the private version of some method (in which the compiler is right in pointing out that the callsite is wrong), or the user indeed might want to access the private method (in which case your patch to shift the focus onto the access error might seem more appropriate).

I'm a bit skeptical that there exist a silver bullet for these kind of mixed cases. Technically speaking, from a JLS perspective, non-accessible methods should not even take place in overload resolution - so the current compiler behavior is cases where both accessible and inaccessible methods are present seems more appropriate.

In other words, if we really want to improve the status quo, we should detect cases where all candidates are not accessible. Or, we should question whether javac's behavior of tagging inaccessible methods with the kind WRONG_MTH/WRONG_MTHS is correct in the first place.

It would be nice, I guess, if we could make the inaccessible error more powerful, so that we could keep adding inaccessble symbols as we go along, so that javac could display something like "cannot resolve symbol m - but some inaccessible symbols have been found ".

@lgxbslgx
Copy link
Contributor Author

@lgxbslgx lgxbslgx commented Nov 24, 2020

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.

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Nov 24, 2020

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

@lgxbslgx
Copy link
Contributor Author

@lgxbslgx lgxbslgx commented Nov 24, 2020

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.

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Nov 24, 2020

/test

@openjdk
Copy link

@openjdk openjdk bot commented Nov 24, 2020

Could not create test job

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Looks a nice generalization. However I still have some concerns:

  • the logic, as I feared, is turning ABSENT into WRONG_MTHS - note that AccessError and InapplicableSymbol/s have different properties when it comes to method lookup - more specifically look at ResolveError::exists which return false in one case and true in the other. This is used, among other things, to shortcircuit method lookups (see Resolve::findFun) - so flipping from AccessError to InapplicableSymbols is almost guaranteed to cause a change in behavior in the compiler. That said, given the nature of the code involved, is not super trivial to come up with an example which demonstrates the difference - but I'll keep thinking.

  • While the tests you add are good, I don't see the corresponding tests for the method reference case.

@@ -1586,16 +1586,46 @@ Symbol selectBest(Env<AttrContext> env,
switch (bestSoFar.kind) {
case ABSENT_MTH:
return new InapplicableSymbolError(currentResolutionContext);
case HIDDEN:
if (bestSoFar instanceof AccessError) {

This comment has been minimized.

@mcimadamore

mcimadamore Nov 24, 2020
Contributor

These changes are a bit convoluted - but that's the nature of the code in here. I also see that you special case AccessError (since now we have, from modules, different kind of HIDDEN error class, which is probably worth another cleanup). If an access error shows up and we already have an access error, you add that to the list of "candidates", and return a WRONG_MTHS kind. Sounds logical.

: bestSoFar;
AccessError curAccessError = new AccessError(env, site, sym);
JCDiagnostic curDiagnostic = curAccessError.getDiagnostic(JCDiagnostic.DiagnosticType.FRAGMENT, null, null, site, null, argtypes, typeargtypes);
if (bestSoFar.kind == ABSENT_MTH) {

This comment has been minimized.

@mcimadamore

mcimadamore Nov 24, 2020
Contributor

This seems like a good generalization of existing code. Give a new access error, we have following cases:

  • best was ABSENT - in which case we return the access error
  • best was WRONG_MTH/WRONG_MTHS/HIDDEN in which case we return WRONG_MTHS with access error added to the list
@lgxbslgx
Copy link
Contributor Author

@lgxbslgx lgxbslgx commented Nov 25, 2020

That said, given the nature of the code involved, is not super trivial to come up with an example which demonstrates the difference

I can't find a example to demonstrate the differences too.

While the tests you add are good, I don't see the corresponding tests for the method reference case.

I add corresponding tests about the method reference cases.

If an access error shows up and we already have an access error, you add that to the list of "candidates", and return a WRONG_MTHS kind.

It should be: If an InapplicableMethodException occurs and we already have an access error

I add some comments to state the situations and improve readability.

Thank you for taking the time to review.

Copy link
Contributor

@mcimadamore mcimadamore left a comment

I left some additional comments on tests. I plan to do some experiments to validate that we are not going to introduce bad regressions.

* questions.
*/

class T8255968 {

This comment has been minimized.

@mcimadamore

mcimadamore Nov 25, 2020
Contributor

Can you please disambiguate all these class names? Having the same class name in multiple tests can create havocs, especially when running in concurrent mode. Please make sure that classes have different names, and that the auxiliary classes defined in these test are, instead, defined as nested classes (so that, again, everything will be encapsulated on a test-by-test basis, w/o any risk of a test picking up a class from another test).

This comment has been minimized.

@lgxbslgx

lgxbslgx Nov 25, 2020
Author Contributor

It sounds a great way to avoid some problem. I would revise my code to meet this style. Thanks for the suggestion.

/*
* @test
* @bug 8255968
* @summary Confusing error message for inaccessible constructor

This comment has been minimized.

@mcimadamore

mcimadamore Nov 25, 2020
Contributor

Any reason as to why you went for this single file which lists all tests rather than adding jtreg tags in all the various test cases? AFAIK, in the javac code base the latter style is often preferred.

This comment has been minimized.

@lgxbslgx

lgxbslgx Nov 25, 2020
Author Contributor

I have seem these two styles in the test code base. I don't prefer any style because these two styles both work well. It is better to have a standard style to guide developers. But now it has no such standard.

AFAIK, in the javac code base the latter style is often preferred.

According to your comment, I would revise the tests to meets the latter style.

@lgxbslgx
Copy link
Contributor Author

@lgxbslgx lgxbslgx commented Nov 25, 2020

The old style which is shown below is not good.

class T8255968 {
    Test c = new Test(0);
}

class Test {
    private Test(int x) {}
}

and that the auxiliary classes defined in these test are, instead, defined as nested classes

I change it to the following style according to the comment. But the compiler compiles it successfully because the outer class can visit private method of its inner class.

class T8255968_1 {
    Test c = new Test(0);

    class Test {
        private Test(int x) {}
    }
}

So I would like to use the following style(don't use nested classes).

class T8255968_1 {
    T8255968Test1 c = new T8255968Test1(0);
}

class T8255968Test1 {
    private T8255968Test1(int x) {}
}

@mcimadamore Do you agree with this style?

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Nov 25, 2020

@mcimadamore Do you agree with this style?

Yeah - of course nesting things is gonna mess up with access checks - sorry for sending you down the wrong direction!

You could prefix every class in the test with T8255968_1 e.g.

class T8255968_1 {
    T8255968Test1 c = new T8255968_1_Test(0);
}

class T8255968_1_Test {
    private T8255968_1_Test(int x) {}
}
@lgxbslgx
Copy link
Contributor Author

@lgxbslgx lgxbslgx commented Nov 26, 2020

I update the code about test cases. Thank you for taking the time to review.

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Nov 27, 2020

I haven't forgotten about this - we're running some extensive corpus analysis to make sure that the patch won't introduce any unwanted regressions. Please bear with us :-)

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Nov 30, 2020

All the internal testing went ok. At this point I see no reason to delay this further. Thanks.

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Looks good

@openjdk
Copy link

@openjdk openjdk bot commented Nov 30, 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:

8255968: Confusing error message for inaccessible constructor

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

  • 962f7a3: 8257162: Initialize ThreadLocalAllocBuffer members
  • 337d7bc: 8257165: C2: Improve box elimination for vector masks and shuffles
  • 4e55d0f: 8257057: C2: Improve safepoint processing during vector scalarization pass
  • e77aed6: 8256754: Deoptimization::revoke_for_object_deoptimization: stack processing start call is redundant
  • 738efea: 8248564: JFR: Remote Recording Stream
  • 9bcd269: 8257221: C2: RegMask::is_bound_set split set handling broken since JDK-8221404
  • 222e943: 8257238: Cleanup include directives for precompiled.hpp
  • fdee70d: 8257237: Cleanup unused imports in the SunJSSE provider implementation
  • 816e8f8: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo
  • c5d9507: 8257220: [JVMCI] option validation should not result in a heavy-weight VM crash
  • ... and 109 more: https://git.openjdk.java.net/jdk/compare/1aa90ac6295e2cdda078eabf0a32e109c2e6c38f...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).

@openjdk openjdk bot added the ready label Nov 30, 2020
@lgxbslgx
Copy link
Contributor Author

@lgxbslgx lgxbslgx commented Nov 30, 2020

/integrate

@openjdk openjdk bot added the sponsor label Nov 30, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 30, 2020

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

@lgxbslgx
Copy link
Contributor Author

@lgxbslgx lgxbslgx commented Nov 30, 2020

Thank you for the review.

@mcimadamore
Could you please sponsor this patch?
Thanks again.

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Dec 1, 2020

/sponsor

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

@openjdk openjdk bot commented Dec 1, 2020

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

  • c5046ca: 8246739: InputStream.skipNBytes could be implemented more efficiently
  • 56b15fb: 8159746: (proxy) Support for default methods
  • 1433baf: 8253751: Dependencies of automatic modules are not propagated through module layers
  • e3d0f27: 8257231: assert(!is_mcall || (call_returns[block->_pre_order] <= (uint) current_offset))
  • eaf4db6: 8257502: Builds fail with new warnings after JDK-8256254
  • 2966d0d: 8257223: C2: Optimize RegMask::is_bound
  • 3a11009: 8256830: misc tests failed with "assert(env->is_enabled(JVMTI_EVENT_OBJECT_FREE)) failed: checking"
  • c859fb0: 8249836: java/io/IOException/LastErrorString.java should have bug-id as 1st word in @ignore
  • e0de28c: 8257424: RecordingStream does not specify the recording name
  • 60f2ba9: 8257487: Include configuration name in summary
  • ... and 144 more: https://git.openjdk.java.net/jdk/compare/1aa90ac6295e2cdda078eabf0a32e109c2e6c38f...master

Your commit was automatically rebased without conflicts.

Pushed as commit 29d90b9.

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

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