JDK-8307184: Incorrect/inconsistent specification and implementation for Elements.getDocComment #15062

jddarcy · 2023-07-28T02:24:08Z

Start by just reformatting the existing specs to highlight subsequent spec changes.

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-8313343 to be approved

Issues

JDK-8307184: Incorrect/inconsistent specification and implementation for Elements.getDocComment (Bug - P3)
JDK-8313343: Incorrect/inconsistent specification and implementation for Elements.getDocComment (CSR)

Reviewers

Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to 9b436d26
Jonathan Gibbons (@jonathan-gibbons - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15062

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

Using diff file

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

Webrev

Link to Webrev Comment

…for Elements.getDocComment

bridgekeeper · 2023-07-28T02:25:07Z

👋 Welcome back darcy! 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 · 2023-07-28T02:26:24Z

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

jddarcy · 2023-07-28T02:30:34Z

/csr needed

mlbridge · 2023-07-28T02:31:07Z

Webrevs

openjdk · 2023-07-28T02:31:13Z

@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@jddarcy please create a CSR request for issue JDK-8307184 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

jddarcy · 2023-07-28T05:42:18Z

Once we've settled on the spec changes, I'll populate the CSR accordingly.

jonathan-gibbons · 2023-08-02T21:15:25Z

src/java.compiler/share/classes/javax/lang/model/util/Elements.java

@@ -283,15 +283,32 @@ default Set<? extends ModuleElement> getAllModuleElements() {
     * <p> A documentation comment of an element is a comment that
     * begins with "{@code /**}", ends with a separate
     * "<code>*&#47;</code>", and immediately precedes the element,
-     * ignoring white space.  Therefore, a documentation comment
+     * ignoring white space and annotations and end-of-line-comments ({@code "//"} comments).


(minor/grammar)
instead of A and B and C consider using A, B, and C

jonathan-gibbons · 2023-08-02T21:26:56Z

src/java.compiler/share/classes/javax/lang/model/util/Elements.java

+     * of the doc comment starting after the initial "{@code /**}",
+     * if the lines start with <em>zero</em> or more white space characters followed by
+     * <em>one</em> or more "{@code *}" characters,
+     * those leading white space characters are discarded as are any


FWIW, I checked javac and it allows form-feed in the leading whitespace characters

As an adjective "white space" is normally a single word, at least in JDK.

Hmm. java.lang.Character contains both "whitespace" and "white space" in its textual comments.

Yes, the difference is typically whitespace (adjective) and white space noun. But no matter.

jonathan-gibbons · 2023-08-02T21:33:25Z

test/langtools/tools/javac/processing/model/util/elements/TestGetDocComments.java

+    void stringDiffer(String actual, String expected) {
+        if (actual.length() != expected.length()) {
+            System.out.println("Strings have different lengths");
+        }
+    }


Not critical, but for bonus points, you could identify the first line that is different.

Do you know of an existing utility usable in the JDK that does this?
(I didn't want to write a utility like that for the purpose of this bug.)

ToolBox.checkEqual

jonathan-gibbons · 2023-08-02T21:39:02Z

General feedback: while the text is good at describing when non-newline characters are removed, it is less good at describing the treatment of newlines ... are they line-terminators or line-separators; how do such characters in the result of getDocComment relate to the characters in the source file? For example, does getDocComment "copy" the newline characters found in the source file or are they always normalized to \n ?

jonathan-gibbons · 2023-08-02T21:41:34Z

test/langtools/tools/javac/processing/model/util/elements/TestGetDocComments.java

+     * Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
+     * eiusmod tempor incididunt ut labore et dolore magna aliqua.
+     */
+    @ExpectedComment( // Cannot used a text block here since leading spaces are removed


You could use a text block, by carefully adjusting the indentation of the trailing """

src/java.compiler/share/classes/javax/lang/model/util/Elements.java

vicente-romero-oracle

looks good to me

jddarcy · 2023-08-04T20:27:29Z

General feedback: while the text is good at describing when non-newline characters are removed, it is less good at describing the treatment of newlines ... are they line-terminators or line-separators; how do such characters in the result of getDocComment relate to the characters in the source file? For example, does getDocComment "copy" the newline characters found in the source file or are they always normalized to \n ?

Hmm. Does the precise handling of line terminators matter enough to specify?

mlbridge · 2023-08-05T10:06:13Z

Mailing list message from Alex Buckley on compiler-dev:

On 8/4/2023 1:33 PM, Joe Darcy wrote:

src/java.compiler/share/classes/javax/lang/model/util/Elements.java line 300:

298: * if the lines start with <em>zero</em> or more white space characters followed by
299: * <em>one</em> or more "{@code *}" characters,
300: * those leading white space characters are discarded as are any

FWIW, I checked `javac` and it allows form-feed in the leading whitespace characters

As an adjective "white space" is normally a single word, at least in JDK.

Hmm. java.lang.Character contains both "whitespace" and "white space" in its textual comments.

The JLS is careful to speak only of "white space" as a noun, never as an
adjective for "character". But I think that when the adjective is
needed, it's "whitespace". java.lang.Character follows this rule, e.g.,
"ISO control characters that are not whitespace" (i.e., are not
whitespace characters).

Alex

jddarcy · 2023-08-06T20:43:51Z

General feedback: while the text is good at describing when non-newline characters are removed, it is less good at describing the treatment of newlines ... are they line-terminators or line-separators; how do such characters in the result of getDocComment relate to the characters in the source file? For example, does getDocComment "copy" the newline characters found in the source file or are they always normalized to \n ?

Hmm. Does the precise handling of line terminators matter enough to specify?

PS FWIW, the javac implementation does normalize line terminators, test case added.

jonathan-gibbons · 2023-08-09T16:24:57Z

General feedback: while the text is good at describing when non-newline characters are removed, it is less good at describing the treatment of newlines ... are they line-terminators or line-separators; how do such characters in the result of getDocComment relate to the characters in the source file? For example, does getDocComment "copy" the newline characters found in the source file or are they always normalized to \n ?

Hmm. Does the precise handling of line terminators matter enough to specify?

Medium yes. For anyone parsing the contents of a doc comment, it helps to know whether the terminators are "as in the source file" or "always \n". I guess if unspecified, users will have to assume the worst case (may be as in the source file) even if that is not typically the case.

jonathan-gibbons · 2023-08-09T20:21:20Z

test/langtools/tools/javac/processing/model/util/elements/TestGetDocComments.java

-                            System.out.println("Expected");
-                            System.out.println(expectedCommentStr);
-                            stringDiffer(actualComment, expectedCommentStr);
+                            (new ToolBox()).checkEqual(expectedCommentStr.lines().toList(),


jonathan-gibbons · 2023-08-09T20:25:27Z

General feedback: while the text is good at describing when non-newline characters are removed, it is less good at describing the treatment of newlines ... are they line-terminators or line-separators; how do such characters in the result of getDocComment relate to the characters in the source file? For example, does getDocComment "copy" the newline characters found in the source file or are they always normalized to \n ?

Hmm. Does the precise handling of line terminators matter enough to specify?

Medium yes. For anyone parsing the contents of a doc comment, it helps to know whether the terminators are "as in the source file" or "always \n". I guess if unspecified, users will have to assume the worst case (may be as in the source file) even if that is not typically the case.

I guess for anyone doing character-based parsing, handling line-ending sequences is no biggie. For folk doing line-based parsing String.lines() comes to mind ...

openjdk · 2023-08-09T21:13:46Z

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

8307184: Incorrect/inconsistent specification and implementation for Elements.getDocComment

Reviewed-by: vromero, jjg

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 43 new commits pushed to the master branch:

593ba2f: 8313693: Introduce an internal utility for the Damerau–Levenshtein distance calculation
360f65d: 8314022: Problem-list tests failing with jtreg 7.3
0eb0997: 8288936: Wrong lock ordering writing G1HeapRegionTypeChange JFR event
19ae62a: 8311170: Simplify and modernize equals and hashCode in security area
e9f751a: 8311247: Some cpp files are compiled with -std:c11 flag
213d3c4: 8313891: JFR: Incorrect exception message for RecordedObject::getInt
0e2c72d: 8313796: AsyncGetCallTrace crash on unreadable interpreter method pointer
52ec4bc: 8303056: Improve support for Unicode characters and digits in JavaDoc search
9cf12bb: 8313922: Remove unused WorkerPolicy::_debug_perturbation
6e3cc13: 8312467: relax the builddir check in make/autoconf/basic.m4
... and 33 more: https://git.openjdk.org/jdk/compare/90d795abf10bf8b8b53079c1afd19fee7b4cb6cf...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

jddarcy · 2023-08-09T21:15:57Z

/integrate

openjdk · 2023-08-09T21:17:13Z

Going to push as commit c307391.
Since your change was applied there have been 43 commits pushed to the master branch:

593ba2f: 8313693: Introduce an internal utility for the Damerau–Levenshtein distance calculation
360f65d: 8314022: Problem-list tests failing with jtreg 7.3
0eb0997: 8288936: Wrong lock ordering writing G1HeapRegionTypeChange JFR event
19ae62a: 8311170: Simplify and modernize equals and hashCode in security area
e9f751a: 8311247: Some cpp files are compiled with -std:c11 flag
213d3c4: 8313891: JFR: Incorrect exception message for RecordedObject::getInt
0e2c72d: 8313796: AsyncGetCallTrace crash on unreadable interpreter method pointer
52ec4bc: 8303056: Improve support for Unicode characters and digits in JavaDoc search
9cf12bb: 8313922: Remove unused WorkerPolicy::_debug_perturbation
6e3cc13: 8312467: relax the builddir check in make/autoconf/basic.m4
... and 33 more: https://git.openjdk.org/jdk/compare/90d795abf10bf8b8b53079c1afd19fee7b4cb6cf...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-09T21:17:20Z

@jddarcy Pushed as commit c307391.

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

JDK-8307184: Incorrect/inconsistent specification and implementation …

59737be

…for Elements.getDocComment

openjdk bot added the rfr Pull request is ready for review label Jul 28, 2023

openjdk bot added the compiler compiler-dev@openjdk.org label Jul 28, 2023

openjdk bot added the csr Pull request needs approved CSR before integration label Jul 28, 2023

jddarcy added 2 commits July 27, 2023 19:35

Update non-beginning, non-eneding lines of the spec.

71fb0d1

Add tests; adjust spec.

faeabe3

openjdk bot removed the rfr Pull request is ready for review label Jul 28, 2023

jddarcy added 2 commits July 27, 2023 22:32

Updates.

9ac4610

Appease jcheck.

9b436d2

openjdk bot added the rfr Pull request is ready for review label Jul 28, 2023

jonathan-gibbons reviewed Aug 2, 2023

View reviewed changes

src/java.compiler/share/classes/javax/lang/model/util/Elements.java Outdated Show resolved Hide resolved

vicente-romero-oracle approved these changes Aug 2, 2023

View reviewed changes

jddarcy added 3 commits August 4, 2023 13:49

Merge branch 'master' into JDK-8307184

eb86a9b

Respond to review comments.

79b1131

Augment test.

32c99b2

jddarcy added 2 commits August 5, 2023 13:21

Use "whitespace" as an adjective.

34aa6ed

Add test case for newlines.

99fae19

jonathan-gibbons approved these changes Aug 9, 2023

View reviewed changes

jddarcy added 2 commits August 9, 2023 12:01

Merge branch 'master' into JDK-8307184

4eae54a

Implement review feedback.

9e5c7b0

jonathan-gibbons reviewed Aug 9, 2023

View reviewed changes

jonathan-gibbons approved these changes Aug 9, 2023

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Aug 9, 2023

openjdk bot added the integrated Pull request has been integrated label Aug 9, 2023

openjdk bot closed this Aug 9, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 9, 2023

jddarcy deleted the JDK-8307184 branch October 17, 2024 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8307184: Incorrect/inconsistent specification and implementation for Elements.getDocComment #15062

JDK-8307184: Incorrect/inconsistent specification and implementation for Elements.getDocComment #15062

jddarcy commented Jul 28, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 28, 2023

openjdk bot commented Jul 28, 2023

jddarcy commented Jul 28, 2023

mlbridge bot commented Jul 28, 2023 •

edited

Loading

openjdk bot commented Jul 28, 2023

jddarcy commented Jul 28, 2023

jonathan-gibbons Aug 2, 2023

jonathan-gibbons Aug 2, 2023

jddarcy Aug 4, 2023

jonathan-gibbons Aug 9, 2023

jonathan-gibbons Aug 2, 2023

jddarcy Aug 4, 2023

jonathan-gibbons Aug 9, 2023

jonathan-gibbons commented Aug 2, 2023

jonathan-gibbons Aug 2, 2023

vicente-romero-oracle left a comment

jddarcy commented Aug 4, 2023

mlbridge bot commented Aug 5, 2023

jddarcy commented Aug 6, 2023

jonathan-gibbons commented Aug 9, 2023

jonathan-gibbons Aug 9, 2023

jonathan-gibbons commented Aug 9, 2023

openjdk bot commented Aug 9, 2023

jddarcy commented Aug 9, 2023

openjdk bot commented Aug 9, 2023

openjdk bot commented Aug 9, 2023

JDK-8307184: Incorrect/inconsistent specification and implementation for Elements.getDocComment #15062

JDK-8307184: Incorrect/inconsistent specification and implementation for Elements.getDocComment #15062

Conversation

jddarcy commented Jul 28, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 28, 2023

openjdk bot commented Jul 28, 2023

jddarcy commented Jul 28, 2023

mlbridge bot commented Jul 28, 2023 • edited Loading

Webrevs

openjdk bot commented Jul 28, 2023

jddarcy commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-gibbons commented Aug 2, 2023

Choose a reason for hiding this comment

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

jddarcy commented Aug 4, 2023

mlbridge bot commented Aug 5, 2023

jddarcy commented Aug 6, 2023

jonathan-gibbons commented Aug 9, 2023

Choose a reason for hiding this comment

jonathan-gibbons commented Aug 9, 2023

openjdk bot commented Aug 9, 2023

jddarcy commented Aug 9, 2023

openjdk bot commented Aug 9, 2023

openjdk bot commented Aug 9, 2023

jddarcy commented Jul 28, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 28, 2023 •

edited

Loading