8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation #20247

liach · 2024-07-19T01:55:57Z

TypeAnnotation is not an annotation, as it should not be used in places like AnnotationValue.ofAnnotation. Thus it's remodeled to contain an annotation at a given location instead of to be an annotation.

Depends on #20205.

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
Change requires CSR request JDK-8336794 to be approved

Issues

JDK-8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation (Bug - P4)
JDK-8336794: Remodel TypeAnnotation to "have" instead of "be" an Annotation (CSR)

Reviewers

Adam Sotona (@asotona - Reviewer)

Contributors

Alex Buckley <abuckley@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20247

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-07-19T01:56:41Z

👋 Welcome back liach! A progress list of the required criteria for merging this PR into pr/20205 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-07-19T01:58:03Z

@liach 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:

8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation

Co-authored-by: Alex Buckley <abuckley@openjdk.org>
Reviewed-by: asotona

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-07-19T01:58:50Z

@liach The following labels will be automatically applied to this pull request:

compiler
core-libs
javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-07-19T02:01:51Z

Webrevs

asotona · 2024-07-19T13:30:44Z

src/java.base/share/classes/jdk/internal/classfile/impl/AnnotationReader.java

+        return lr.labelToBci(label);
+    }
+
+    public static void writeTypeAnnotation(BufWriterImpl buf, TypeAnnotation ta) {


Is there any reason to move writeTypeAnnotation from UnboundAttribute?

This is for consistency with reading annotations: they are now defined in the same file.

asotona

Looks good to me.

mlbridge · 2024-07-20T04:34:25Z

Mailing list message from Alex Buckley on compiler-dev:

Logically, an annotation is either a declaration annotation or a type
annotation. A declaration annotation _is_ an annotation. A type
annotation _is_ an annotation. These terms come from JLS 9.7.4.

In the Class File API, `Annotation` is specified as "Models an
annotation on a declaration." That's fair: you're using the "good" name
`Annotation` for declaration annotations because they are more common
than type annotations.

(It's not what the Core Reflection API does --
`java.lang.reflect.Annotation` denotes annotations at a very high level
of abstraction, independent of appearance in declaration contexts or
type contexts -- but that's a different API, so OK.)

However, I think it's wrong in Class File for `TypeAnnotation` to
have/contain an `Annotation`. Logically, a type annotation does not
have/contain a declaration annotation.

I don't know much the Class File API supports writing a `TypeAnnotation`
into a class file (especially since WritableElement is on the way out),
but the problem would be reifying a type annotation on (say) the type
used in an `extends` clause, but with the annotation itself only being
permitted on (say) a field declaration.

Alex

On 7/18/2024 7:01 PM, Chen Liang wrote:

openjdk-notifier · 2024-07-23T12:17:21Z

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout fix/typeanno-model
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2024-07-23T12:18:17Z

@liach this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout fix/typeanno-model
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2024-07-23T12:18:18Z

⚠️ @liach This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk · 2024-07-23T15:40:39Z

@liach Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

liach · 2024-07-23T15:40:40Z

Unfortunately had to force push: the old commit histories now cause conflicts, so I had squashed them into one and applied onto the latest master.

liach · 2024-07-23T21:39:52Z

/contributor add abuckley

openjdk · 2024-07-23T21:40:39Z

@liach
Contributor Alex Buckley <abuckley@openjdk.org> successfully added.

liach · 2024-08-02T00:02:08Z

I have implemented the suggestions (compiled to -> represented by, key-value -> element value) and tried to simplify the wording. Please review the current version:

 * <p>
 * In an annotation in Java source code, elements in the annotation interface
 * with default values may not be represented unless an explicit value is provided.
 * ({@jls 9.6.2}) The default value is derived from the {@link AnnotationDefaultAttribute
 * AnnotationDefault} attribute on the method representing the annotation
 * interface element, in the class file representing the annotation interface.
 * <p id="repeatable">
 * Multiple annotations of the same interface <i>A</i> in Java source code
 * ({@jls 9.7.5}) are represented by the {@linkplain AnnotationValue.OfAnnotation
 * annotation-valued} array elements of the {@linkplain AnnotationValue.OfArray
 * array-valued} element named {@code value} of a container annotation, whose
 * interface is the containing annotation interface of <i>A</i>. ({@jls 9.6.3})

// location in class files to source code construct/type section
 * If this annotation is of a {@linkplain ##repeatable repeatable} annotation
 * interface <i>A</i>, is an array element of an array-valued element named
 * {@code value} in a container annotation, and the interface of the container
 * annotation is the containing annotation interface <i>AC</i> of <i>A</i>,
 * this annotation represents a base annotation of type <i>A</i>, which applies
 * to the same source code construct or type as the container annotation of
 * type <i>AC</i>.

// On AnnotationElement::name
     * @apiNote
     * In Java source code, by convention, the name of the sole element in a
     * single-element annotation interface is {@code value}. ({@jls 9.6.1})
     * A single-element annotation ({@jls 9.7.3}) declares the element value
     * for the {@code value} element. The single element of a containing
     * annotation interface that holds {@linkplain Annotation##repeatable
     * multiple} base annotations is also named {@code value}. ({@jls 9.6.3})

mlbridge · 2024-08-02T02:04:25Z

Mailing list message from Alex Buckley on compiler-dev:

On 8/1/2024 1:02 PM, Chen Liang wrote:

* In an annotation in Java source code, elements in the annotation interface
* with default values may not be compiled into element-value pairs unless an
* explicit value is provided. ({@jls 9.6.2}) The default value is derived from
* the {@link AnnotationDefaultAttribute AnnotationDefault} attribute on the
* method representing the annotation interface element in the class file
* representing the annotation interface.
* 
* Multiple annotations of the same interface A in Java source code
* ({@jls 9.7.5}) are represented by the {@linkplain AnnotationValue.OfAnnotation
* annotation-valued} array elements of the {@linkplain AnnotationValue.OfArray
* array-valued} element named {@code value} of a container annotation of the
* containing annotation interface of A. ({@jls 9.6.3})

Very good and precise usage of "annotation" versus "annotation
interface". The last sentence is rather long; you could break it as "...
of a container annotation. The interface of the container annotation is
the containing annotation interface of A."

  \* \<p>
  \* A single\-element annotation \(\{\@<!-- -->jls 9\.7\.3\}\) in Java source code is
  \* compiled to an \{\@<!-- -->link Annotation\} with exactly one \{\@<!-- -->linkplain
  \* Annotation\#elements key\-value pair\} where the element has the name
  \* \{\@<!-- -->code value\}\.

I take issue with "compiled to ...". The annotation is *compiled* to a
Runtime...Annotations attribute, but it's *represented* by an Annotation
with exactly one ... (Just as multiple annotations of the same
interface are *represented* by the annotation-valued ...)

  \* \<p>
  \* Multiple annotations of the same interface \<i>A\<\/i> in Java source code
  \* is compiled to an implicitly declared container annotation \(\{\@<!-- -->jls 9\.7\.5\}\)
  \* with exactly one \{\@<!-- -->linkplain Annotation\#elements key\-value pair\} where
  \* the element has the name \{\@<!-- -->code value\} and the type of the element is the
  \* \{\@<!-- -->linkplain AnnotationValue\.OfArray array\} whose component type is the
  \* \{\@<!-- -->linkplain AnnotationValue\.OfAnnotation annotation interface \<i>A\<\/i>\}\.

Multiple annotations ... *are represented* [not "is compiled"] by a
container annotation [drop "implicitly declared", that's a compile time
story but you're discussing run time] with exactly one *element-value*
pair [never seen "key-value pair" used for annotations]. The name of the
element is ... and the type of the element is ...

Alex

mlbridge · 2024-08-02T23:33:25Z

Mailing list message from Chen Liang on core-libs-dev:

One specification question for Alex: we call an annotation interface's element "element". What about the element-value pair that binds a value to an interface element in an annotation? Can we call this element-value pair an "element", or is there any other more proper name that is still concise? For context:

[This annotation] is an array element of an array-valued element named {@code value} in a container annotation

________________________________
From: core-libs-dev <core-libs-dev-retn at openjdk.org> on behalf of Adam Sotona <asotona at openjdk.org>
Sent: Friday, August 2, 2024 8:55 AM
To: compiler-dev at openjdk.org <compiler-dev at openjdk.org>; core-libs-dev at openjdk.org <core-libs-dev at openjdk.org>; javadoc-dev at openjdk.org <javadoc-dev at openjdk.org>
Subject: Re: RFR: 8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation [v8]

On Fri, 2 Aug 2024 00:04:48 GMT, Chen Liang <liach at openjdk.org> wrote:

`TypeAnnotation` is not an annotation, as it should not be used in places like `AnnotationValue.ofAnnotation`. Thus it's remodeled to contain an annotation at a given location instead of to be an annotation.

Depends on #20205.

Chen Liang 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 12 additional commits since the last revision:

- remove compile, use element-value, break long sentences
- Merge branch 'master' of https://github.com/openjdk/jdk into fix/typeanno-model
- Improve docs for repeating, default, and value name
- Merge branch 'master' of https://github.com/openjdk/jdk into fix/typeanno-model
- Merge branch 'master' of https://github.com/openjdk/jdk into fix/typeanno-model
- More refinements from alex
- Artifact -> construct
- More about Annotation, add equals note
- Further refine wording
- Refine the spec of TypeAnnotation per Alex feedback
- ... and 2 more: https://git.openjdk.org/jdk/compare/f3707e9b...3a91a3a

Marked as reviewed by asotona (Reviewer).

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

PR Review: https://git.openjdk.org/jdk/pull/20247#pullrequestreview-2215642863
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240802/7dc6a2d5/attachment.htm>

mlbridge · 2024-08-02T23:33:26Z

Mailing list message from Alex Buckley on core-libs-dev:

In the annotation `@Foo(a=1)`, the left hand side `a` is an _element_
(as declared in the annotation interface Foo) and the right hand side
`1` is an _element value_ (per the first line of JLS 9.7.1).

Syntactically the `a` is an identifier but that's almost never relevant
what discussing element-value pairs. In the text you quoted, "an array
element of an array-valued element `value`" is the right way to say it.

Alex

On 8/2/2024 8:45 AM, Chen Liang wrote:

mlbridge · 2024-08-02T23:33:27Z

Mailing list message from Chen Liang on core-libs-dev:

Thanks for the confirmation. I will submit this finalized specification for CSR review.
________________________________
From: javadoc-dev <javadoc-dev-retn at openjdk.org> on behalf of Alex Buckley <alex.buckley at oracle.com>
Sent: Friday, August 2, 2024 10:53 AM
To: compiler-dev at openjdk.org <compiler-dev at openjdk.org>; core-libs-dev at openjdk.org <core-libs-dev at openjdk.org>; javadoc-dev at openjdk.org <javadoc-dev at openjdk.org>
Subject: Re: RFR: 8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation [v8]

In the annotation `@Foo(a=1)`, the left hand side `a` is an _element_
(as declared in the annotation interface Foo) and the right hand side
`1` is an _element value_ (per the first line of JLS 9.7.1).

Syntactically the `a` is an identifier but that's almost never relevant
what discussing element-value pairs. In the text you quoted, "an array
element of an array-valued element `value`" is the right way to say it.

Alex

On 8/2/2024 8:45 AM, Chen Liang wrote:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240802/328908ae/attachment.htm>

…anno-model

liach · 2024-08-16T15:24:44Z

Swallowed the 2 conflicts from #20513: The tags are already correct in this patch.

liach · 2024-08-16T15:48:01Z

Thanks for the reviews!

/integrate

openjdk · 2024-08-16T15:48:55Z

Going to push as commit 961e944.

openjdk · 2024-08-16T15:49:01Z

@liach Pushed as commit 961e944.

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

openjdk bot added csr rfr labels Jul 19, 2024

openjdk bot added javadoc core-libs compiler labels Jul 19, 2024

asotona reviewed Jul 19, 2024

View reviewed changes

asotona approved these changes Jul 19, 2024

View reviewed changes

openjdk-notifier bot changed the base branch from pr/20205 to master July 23, 2024 12:17

openjdk bot added the merge-conflict label Jul 23, 2024

liach added 2 commits July 23, 2024 08:35

8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation

Verified

This commit was signed with the committer’s verified signature.

IvanGoncharov Ivan Goncharov

GPG key ID: 565A593B3E13C939

Verified
Learn about vigilant mode

Loading
Loading status checks…

b5df684

SealedGraph now redundant

Verified

This commit was signed with the committer’s verified signature.

IvanGoncharov Ivan Goncharov

GPG key ID: 565A593B3E13C939

Verified
Learn about vigilant mode

Loading
Loading status checks…

6e31c48

liach force-pushed the fix/typeanno-model branch from 877b938 to 6e31c48 Compare July 23, 2024 15:39

openjdk bot removed the merge-conflict label Jul 23, 2024

liach added 4 commits July 23, 2024 14:07

Refine the spec of TypeAnnotation per Alex feedback

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

Loading
Loading status checks…

416890a

Further refine wording

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

beb9215

More about Annotation, add equals note

84c4bfc

Artifact -> construct

0e9a571

More refinements from alex

cf16e7e

remove compile, use element-value, break long sentences

3a91a3a

asotona approved these changes Aug 2, 2024

View reviewed changes

liach added 7 commits August 12, 2024 14:37

Merge branch 'master' of https://github.com/openjdk/jdk into fix/type…

db1df98

…anno-model

Stage

e85eb40

Merge branch 'master' of https://github.com/openjdk/jdk into fix/type…

88f64d0

…anno-model

Merge branch 'master' of https://github.com/openjdk/jdk into fix/type…

ef279ac

…anno-model

Fix terminology

92b56db

More bad terms and redundancy

c66c7d2

Missed JLS prefix from web review

8af0778

asotona approved these changes Aug 14, 2024

View reviewed changes

openjdk bot added merge-conflict and removed csr labels Aug 16, 2024

Merge branch 'master' of https://github.com/openjdk/jdk into fix/type…

802775e

…anno-model

liach requested a review from asotona August 16, 2024 15:23

openjdk bot removed the merge-conflict label Aug 16, 2024

asotona approved these changes Aug 16, 2024

View reviewed changes

openjdk bot added the ready label Aug 16, 2024

openjdk bot added the integrated label Aug 16, 2024

openjdk bot closed this Aug 16, 2024

openjdk bot removed ready rfr labels Aug 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation #20247

8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation #20247

liach commented Jul 19, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 19, 2024

openjdk bot commented Jul 19, 2024 •

edited

Loading

openjdk bot commented Jul 19, 2024

mlbridge bot commented Jul 19, 2024 •

edited

Loading

asotona Jul 19, 2024

liach Jul 19, 2024

asotona left a comment

mlbridge bot commented Jul 20, 2024

openjdk-notifier bot commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

liach commented Jul 23, 2024

liach commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

liach commented Aug 2, 2024

mlbridge bot commented Aug 2, 2024

mlbridge bot commented Aug 2, 2024

mlbridge bot commented Aug 2, 2024

mlbridge bot commented Aug 2, 2024

liach commented Aug 16, 2024

liach commented Aug 16, 2024

openjdk bot commented Aug 16, 2024

openjdk bot commented Aug 16, 2024

8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation #20247

8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation #20247

Conversation

liach commented Jul 19, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Jul 19, 2024

openjdk bot commented Jul 19, 2024 • edited Loading

openjdk bot commented Jul 19, 2024

mlbridge bot commented Jul 19, 2024 • edited Loading

Webrevs

asotona Jul 19, 2024

Choose a reason for hiding this comment

liach Jul 19, 2024

Choose a reason for hiding this comment

asotona left a comment

Choose a reason for hiding this comment

mlbridge bot commented Jul 20, 2024

openjdk-notifier bot commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

liach commented Jul 23, 2024

liach commented Jul 23, 2024

openjdk bot commented Jul 23, 2024

liach commented Aug 2, 2024

mlbridge bot commented Aug 2, 2024

mlbridge bot commented Aug 2, 2024

mlbridge bot commented Aug 2, 2024

mlbridge bot commented Aug 2, 2024

liach commented Aug 16, 2024

liach commented Aug 16, 2024

openjdk bot commented Aug 16, 2024

openjdk bot commented Aug 16, 2024

liach commented Jul 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 19, 2024 •

edited

Loading

mlbridge bot commented Jul 19, 2024 •

edited

Loading