-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8339852: Fix typos in java.compiler documentation #20937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,5 @@ | ||||||
| /* | ||||||
| * Copyright (c) 2013, 2022, Oracle and/or its affiliates. All rights reserved. | ||||||
| * Copyright (c) 2013, 2024, Oracle and/or its affiliates. All rights reserved. | ||||||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||||||
| * | ||||||
| * This code is free software; you can redistribute it and/or modify it | ||||||
|
|
@@ -45,14 +45,14 @@ | |||||
| * <em>type annotation</em>. | ||||||
| * | ||||||
| * The terms <em>directly present</em>, <em>present</em>, | ||||||
| * <em>indirectly present</em>, and <em>associated </em> are used | ||||||
| * <em>indirectly present</em>, and <em>associated</em> are used | ||||||
| * throughout this interface to describe precisely which annotations, | ||||||
| * either declaration annotations or type annotations, are returned by | ||||||
| * the methods in this interface. | ||||||
| * | ||||||
| * <p>In the definitions below, an annotation <i>A</i> has an | ||||||
| * annotation interface <i>AI</i>. If <i>AI</i> is a repeatable annotation | ||||||
| * interface, the type of the containing annotation is <i>AIC</i>. | ||||||
| * interface, the type of the container annotation is <i>AIC</i>. | ||||||
| * | ||||||
| * <p>Annotation <i>A</i> is <em>directly present</em> on a construct | ||||||
| * <i>C</i> if either: | ||||||
|
|
@@ -78,7 +78,7 @@ | |||||
| * Specification</cite> (JLS {@jls 8.10.1}). | ||||||
| * | ||||||
| * If there are multiple annotations of type <i>AI</i> present on | ||||||
| * <i>C</i>, then if <i>AI</i> is repeatable annotation interface, an | ||||||
| * <i>C</i>, then if <i>AI</i> is a repeatable annotation interface, an | ||||||
| * annotation of type <i>AIC</i> is {@linkplain javax.lang.model.util.Elements#getOrigin(AnnotatedConstruct, AnnotationMirror) implicitly declared} on <i>C</i>. | ||||||
| * <li> A representation of <i>A</i> appears in the executable output | ||||||
| * for <i>C</i>, such as the {@code RuntimeVisibleAnnotations} (JVMS {@jvms 4.7.16}) or | ||||||
|
|
@@ -189,10 +189,11 @@ public interface AnnotatedConstruct { | |||||
| <A extends Annotation> A getAnnotation(Class<A> annotationType); | ||||||
|
|
||||||
| /** | ||||||
| * Returns annotations that are <em>associated</em> with this construct. | ||||||
| * Returns annotations of the specified type that are <em>associated</em> | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please see the entire javadoc, not just the parts ended up in this diff. There's one more method that uses this term. In general, "type" should not always be changed to "interface" in the context of annotations. |
||||||
| * with this construct. | ||||||
| * | ||||||
| * If there are no annotations associated with this construct, the | ||||||
| * return value is an array of length 0. | ||||||
| * If there are no annotations of the specified type associated with this | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the above comment on "type". |
||||||
| * construct, the return value is an array of length 0. | ||||||
| * | ||||||
| * The order of annotations which are directly or indirectly | ||||||
| * present on a construct <i>C</i> is computed as if indirectly present | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -202,7 +202,7 @@ public enum SourceVersion { | |
| * @see <a href="https://openjdk.org/jeps/213"> | ||
| * JEP 213: Milling Project Coin</a> | ||
| */ | ||
| RELEASE_9, | ||
| RELEASE_9, | ||
|
|
||
| /** | ||
| * The version introduced by the Java Platform, Standard Edition | ||
|
|
@@ -470,7 +470,7 @@ private static SourceVersion getLatestSupported() { | |
| * | ||
| * @apiNote This method is included alongside {@link latest} to | ||
| * allow identification of situations where the language model API | ||
| * is running on a platform version different than the latest | ||
| * is running on a platform version different from the latest | ||
| * version modeled by the API. One way that sort of situation can | ||
| * occur is if an IDE or similar tool is using the API to model | ||
| * source version <i>N</i> while running on platform version | ||
|
|
@@ -496,8 +496,7 @@ public static SourceVersion latestSupported() { | |
| * followed only by characters for which {@link | ||
| * Character#isJavaIdentifierPart(int)} returns {@code true}. | ||
| * This pattern matches regular identifiers, keywords, contextual | ||
| * keywords, and the literals {@code "true"}, | ||
| * {@code "false"}, {@code "null"}. | ||
| * keywords, boolean literals, and the null literal. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is necessarily more clear remove the explicit listing of the strings in question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps. I just thought that it's strange to spell them out only here, but refer to them through "x literal(s)" everywhere else in that file. |
||
| * | ||
| * The method returns {@code false} for all other strings. | ||
| * | ||
|
|
@@ -590,14 +589,14 @@ public static boolean isName(CharSequence name, SourceVersion version) { | |
| } | ||
|
|
||
| /** | ||
| * Returns whether or not {@code s} is a keyword, boolean literal, | ||
| * or null literal in the latest source version. | ||
| * Returns whether or not {@code s} is a keyword, a boolean literal, | ||
| * or the null literal in the latest source version. | ||
| * This method returns {@code false} for <i>contextual | ||
| * keywords</i>. | ||
| * | ||
| * @param s the string to check | ||
| * @return {@code true} if {@code s} is a keyword, or boolean | ||
| * literal, or null literal, {@code false} otherwise. | ||
| * @return {@code true} if {@code s} is a keyword, boolean | ||
| * literal, or the null literal, {@code false} otherwise. | ||
| * @jls 3.9 Keywords | ||
| * @jls 3.10.3 Boolean Literals | ||
| * @jls 3.10.8 The Null Literal | ||
|
|
@@ -607,15 +606,15 @@ public static boolean isKeyword(CharSequence s) { | |
| } | ||
|
|
||
| /** | ||
| * Returns whether or not {@code s} is a keyword, boolean literal, | ||
| * or null literal in the given source version. | ||
| * Returns whether or not {@code s} is a keyword, a boolean literal, | ||
| * or the null literal in the given source version. | ||
| * This method returns {@code false} for <i>contextual | ||
| * keywords</i>. | ||
| * | ||
| * @param s the string to check | ||
| * @param version the version to use | ||
| * @return {@code true} if {@code s} is a keyword, or boolean | ||
| * literal, or null literal, {@code false} otherwise. | ||
| * @return {@code true} if {@code s} is a keyword, boolean | ||
| * literal, or the null literal, {@code false} otherwise. | ||
| * @jls 3.9 Keywords | ||
| * @jls 3.10.3 Boolean Literals | ||
| * @jls 3.10.8 The Null Literal | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the annotation interface of the container annotation is AIC"
or
"the containing annotation interface is AIC"
To align with the JLS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Type" is appropriate here and in some other locations in the
AnnotatedConstructjavadoc. I paid attention to the difference between "contain-ing annotation interface" and "contain-er annotation".@dansmithcode, please correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For convenience, here's the rendered documentation in question for JDK 23: https://download.java.net/java/early_access/jdk23/docs/api/java.compiler/javax/lang/model/AnnotatedConstruct.html#:~:text=containing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "type" terminology is outdated and is already migrated to "class or interface" or the applicable subtype elsewhere: https://bugs.openjdk.org/browse/JDK-8257636 and commit 952dc70
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liach, I respect you as a fellow JLS reader. Not only am I aware of that JBS issue, but I also remember the respective PR being reviewed.
Unsurprisingly, I had a similar knee-jerk reaction to the word "type". So, before publishing this PR, I asked a JLS expert on its use in this context. He replied that although in general "type" should not to be conflated with "interface"/"class", in this context it is reasonable to use "type".
However, if we do decide to abandon "type", we need to inspect every use of it in this class very carefully. The closest terminology that I could find is JLS 9.7.1
and JLS 9.6.3
Note how slightly dizzying the terminology is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separately, maybe JLS hasn't done its homework in full? See 9.6.4.1.
@Target:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that use of "type" in 9.6.4.1. feels like an overlook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chen, I suggest we integrate this PR as is but ask JLS experts/authors on the issue. In fact, I've already asked. I suspect that JLS 9.6.4.1 is germane to this discussion. If JLS changes "type" to "interface" in JLS 9.6.4.1, we could change these occurrences of "type" to "interface" in
AnnotatedConstructtoo.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's really nothing wrong with talking about the type of an annotation, just like you can talk about the type of a variable. If it's the most intuitive way to express what you're trying to say, go for it!