RFR: 8295024: Cyclic constructor error is non-deterministic and inconsistent [v2]

[Vicente Romero]

On Thu, 13 Oct 2022 20:33:36 GMT, Archie L. Cobbs <duke at openjdk.org> wrote:

>> This patch addresses two related issues that cause javac's reporting of recursive constructor errors to be non-deterministic.
>> 
>> The first issue is caused by the use of a `HashMap` when finding cycles in the graph of constructor invocations. This causes non-determinism in which constructor is identified as the "culprit" for the purposes of error reporting. The obvious fix here is to use a `LinkedHashMap` instead.
>> 
>> The second problem is caused by how the error's source code location is found via `TreeInfo.diagnosticPositionFor()`. That method searches the AST for a node matching the constructor's `Symbol`, but both the constructor declaration and all of its invocations will match that `Symbol`, and so whichever of those happens first in the source code wins.
>> 
>> The fix adds an optional filter parameter to `TreeInfo.diagnosticPositionFor()`, and then uses it to match only the constructor invocation. Reporting the invocation instead of the declaration is preferred because it provides more specific information about exactly how the recursion occurs. Put another way, when identifying a cycle in a graph, reporting an edge is more helpful than reporting a node.
>
> Archie L. Cobbs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Apply several improvements suggested in review.
>   
>   - Add constructor & method variants to lessen impact on existing code.
>   - Remove unnecessary @compile and @run tags from unit test.
>   - Use hasTag(IDENT) instead of JCIdent::isInstance.

looks good to me

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

Marked as reviewed by vromero (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10679