RFR: 8288130: compiler error with AP and explicit record accessor [v3]

[Jan Lahoda]

On Wed, 22 Jun 2022 15:00:37 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> Please review this PR which is fixing a tricky bug in records. Right now this code is trivial to compile by javac:
>> 
>> record TestRecord<T>(T someValue) {
>>     public T someValue() {
>>         return this.someValue;
>>     }
>> }
>> 
>> but in the presence of an annotation processor this code is entered at least two times. Every one of those times a new type variable is created. Now for records this have some implications. Record components are created once, in most cases unless there is an error for example, meaning that the type of the record component in this case was not in sync with the type of the accessor, and this is a mismatch reported as an error by the compiler. Note that if the accessor was not explicitly declared but generated this wouldn't happen as the accessor is generated from the info in the record component so a generated accessor is always in sync with its corresponding record component.
>> 
>> A possible solution to this issue could be to recreate the record component if there is at least a type variable in its type. But I decided to go for an all in solution recreating the record component always so if there are annotation processor rounds, a new record component would be created for each of them. This way we can future-proof this area of records which has seen bugs in the past and be sure that we are always dealing with a fresh instance of the record components.
>> 
>> TIA
>
> Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8288130
>  - adding some documentation
>  - adding accompanying regression test
>  - 8288130: compiler error with AP and explicit record accessor

Overall, the approach seems fine to me. Added some comments for consideration.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java line 1510:

> 1508:             for (RecordComponent rc : recordComponents) {
> 1509:                 /* it could be that a record erroneously declares two record components with the same name, in that
> 1510:                  * case we need to use the position to disambiguate

I'd suggest to consider renaming this method. It does not "get" the record component, it (re-)creates it The `addIfMissing` parameter appears to be always `true`, and can be removed.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java line 1518:

> 1516:             RecordComponent rc = null;
> 1517:             if (toRemove != null) {
> 1518:                 // Found a record component with an erroneous type: remove it and create a new one

Maybe update this comment?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TypeEnter.java line 1254:

> 1252:                 RecordComponent rec = tree.sym.getRecordComponent(field.sym);
> 1253:                 TreeCopier<JCTree> tc = new TreeCopier<>(make.at(field.pos));
> 1254:                 List<JCAnnotation> originalAnnos = rec.getOriginalAnnos().isEmpty() ?

For you consideration:
-when instead of `rec.getOriginalAnnos().isEmpty() ...`, maybe just `tc.copy(rec.getOriginalAnnos())` (with a possible tweak to the copy method to not create the `ListBuffer` when the input is empty?)
-when setting the annotations, `fields.mods.annotations = originalAnnos` might be better, as it will keep other metadata of the modifiers (if needed/useful)

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

Marked as reviewed by jlahoda (Reviewer).

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