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

[Maurizio Cimadamore]

On Tue, 24 Nov 2020 15:32:31 GMT, Guoxiong Li <github.com+13688759+lgxbslgx at openjdk.org> wrote:

>> 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](https://bugs.openjdk.java.net/browse/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.
>
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Unify error messages for inaccessible constructor

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.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 1590:

> 1588:                     return new InapplicableSymbolError(currentResolutionContext);
> 1589:                 case HIDDEN:
> 1590:                     if (bestSoFar instanceof AccessError) {

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.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 1609:

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

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

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

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