8355065: ConcurrentModificationException in RichDiagnosticFormatter

[cushon]

Hi, please consider this fix for JDK-8355065. RichDiagnosticFormatter is comparing type variables by their toString representation, and in this example the comparison fails due to type annotations. Replacing a call to stripMetadataIfNeeded with stripMetadata allows the comparison to succeed, and is consistent with the approach used in other parts of RichDiagnosticFormatter (e.g. for the fix for JDK-8144580).

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8355065: ConcurrentModificationException in RichDiagnosticFormatter (Bug - P2)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24769/head:pull/24769
$ git checkout pull/24769

Update a local copy of the PR:
$ git checkout pull/24769
$ git pull https://git.openjdk.org/jdk.git pull/24769/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24769

View PR using the GUI difftool:
$ git pr show -t 24769

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24769.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back cushon! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

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

[mlbridge]

Webrevs

• 02: Full - Incremental (da1c2f50)
• 01: Full - Incremental (1a1ff0bf)
• 00: Full (f9eaa00c)

[liach]

Seems stripMetadataIfNeeded is only used for types obtained from StructuralTypeMapping or TypeProjection/TypeArgumentProjection. Also not only the base usage, but the other usage of stripping metadata in the base diagnostic formatter uses stripMetadata, so this does make sense.

[liach]

Hmm, now by stripping, now the metadata is no longer processed and the missing enum problem is no longer reported - this seems to just make the root CME more latent and eliminates one location where it can surface.

[cushon]

Thanks @liach for taking a look.

I agree stripMetadataIfNeeded seems like the wrong choice here, even if there are other opportunities to harden RichDiagnosticFormatter against issues like this.

now the metadata is no longer processed and the missing enum problem is no longer reported

I think that's consistent with other errors missing classes, symbols are completed lazily and the errors only get reported if a missing symbol is noticed. The only place this annotation was being completed was unnecessarily during diagnostic formatting, by the call to toString on the unstripped type.

this seems to just make the root CME more latent and eliminates one location where it can surface

The assumption in RichDiagnosticFormatter seems to be that it isn't reentrant, and it only process one diagnostic at a time. If format is called recursively the mutable state in whereClauses gets updated for the new diagnostic and not restored.

It might also be worthwhile to either update RichDiagnosticFormatter to detect if format is called recursively and report an error, or else make it safe and keep a stack of whereClauses state to make that correct.

[cushon]

This got me wondering whether we even need stripMetadataIfNeeded at this point, I filed https://bugs.openjdk.org/browse/JDK-8355204 to investigate removing it.

[cushon]

It might also be worthwhile to either update RichDiagnosticFormatter to detect if format is called recursively and report an error, or else make it safe and keep a stack of whereClauses state to make that correct.

I added a draft of the first approach, which causes it to detect if RichDiagnosticFormatter is used recursively and throws an exception.

[liach]

Looks like a great candidate for EnumMap.

[cushon]

Thanks, fixed. (The outer map was an EnumMap before, this was an oversight.)