8332858: References with escapes have broken positions after they are transformed #19387

lahodaj · 2024-05-24T09:05:20Z

If the javadoc comment contains a (Markdown) link like:

[java.util.Arrays#asList(Object\[\])]

The transformer that converts this link into the Javadoc link will not find the reference, as it is looking for java.util.Arrays#asList(Object[]) (note the missing escapes), which is not present in the original text.

This patch tries to fix that by permitting optional escapes for all escapable character when searching for the reference, in case the literal search fails. This is done using regexp, although could presumably be done using a manual search.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8332858: References with escapes have broken positions after they are transformed (Bug - P3)

Reviewers

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19387

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

Using diff file

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

Webrev

Link to Webrev Comment

… transformed

bridgekeeper · 2024-05-24T09:05:58Z

👋 Welcome back jlahoda! 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 · 2024-05-24T09:06:33Z

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

8332858: References with escapes have broken positions after they are transformed

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

789ac8b: 8333189: Make sure clang on linux uses lld as linker
c8eea59: 8332919: SA PointerLocation needs to print a newline after dumping java thread info for JNI Local Ref
bc7d9e3: 8333013: Update vmTestbase/nsk/share/LocalProcess.java to don't use finalization
03b7a85: 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state
43a2f17: 8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage
fed2b56: 8320999: RISC-V: C2 RotateLeftV
6cda4c5: 8321543: Update NSS to version 3.96
c003c12: 8331865: Consolidate size and alignment checks in LayoutPath
6d718ae: 8324341: Remove redundant preprocessor #if's checks
9b64ece: 8332904: ubsan ppc64le: c1_LIRGenerator_ppc.cpp:581:21: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long int'
... and 77 more: https://git.openjdk.org/jdk/compare/2581935b47afaf661a94c8a8e50ce08065d632f6...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.

openjdk · 2024-05-24T09:07:03Z

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

compiler
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-05-24T09:10:10Z

Webrevs

01: Full - Incremental (5871d111)
00: Full (b75f896f)

vicente-romero-oracle · 2024-05-25T11:06:51Z

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java

+
+                    for (char c : ref.toCharArray()) {
+                        if (Escaping.ESCAPABLE.indexOf(c) >= 0) {
+                            pattern.append("\\\\?");


suggestion, dunno how critical is this code but regex usually are a tax on performance, I would consider not using them

Sorry for the belated answer. Yes, regexps are usually not very performant, but it is only happening when the exact match fails. And, hopefully, the regexp should not be too difficult to handle. I was considering writing the search by hand, and I can, but it seems like a lot of code to handle a case like this. I can write the search manually, though.

ok if you think they won't be a problem in this case, I'm fine with it, we can always refactor the code if needed

I think the use of regex is overkill. The only characters that need to be escaped, and must be escaped are \[\]

Unescaped square bracket characters are not allowed inside the opening and closing square brackets of link labels.

and, [] can only occur as such in adjacent pairs, so you ought to be able to get away with a simple ref.replaceAll("[]", \[\]")`

While Markdown may permit these of escapes for other characters in other kinds of links, for the extended form of links (that map to {@link[plain]...}) we specify the string must be the signature, with the only exception of needing to escape the square brackets.

Note in particular, this line, at about line 555 in this file.

var l = label.replace("\\[\\]", "[]");

All we need to do here is effectively "revert" the effect of this transformation before searching for the input text.

vicente-romero-oracle

looks good

jonathan-gibbons · 2024-05-28T21:52:04Z

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java

@@ -35,6 +35,7 @@
 import com.sun.source.doctree.DocCommentTree;
 import com.sun.source.doctree.DocTree;
 import com.sun.source.doctree.DocTreeVisitor;
+import com.sun.source.doctree.EscapeTree;


Not sure why you need this.

jonathan-gibbons · 2024-05-28T21:52:16Z

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java

+import java.util.regex.Matcher;
+import jdk.internal.org.commonmark.internal.util.Escaping;


imports out of order

jonathan-gibbons · 2024-05-28T22:05:24Z

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java

+
+                    for (char c : ref.toCharArray()) {
+                        if (Escaping.ESCAPABLE.indexOf(c) >= 0) {
+                            pattern.append("\\\\?");


I think the use of regex is overkill. The only characters that need to be escaped, and must be escaped are \[\]

Unescaped square bracket characters are not allowed inside the opening and closing square brackets of link labels.

and, [] can only occur as such in adjacent pairs, so you ought to be able to get away with a simple ref.replaceAll("[]", \[\]")`

While Markdown may permit these of escapes for other characters in other kinds of links, for the extended form of links (that map to {@link[plain]...}) we specify the string must be the signature, with the only exception of needing to escape the square brackets.

Note in particular, this line, at about line 555 in this file.

var l = label.replace("\\[\\]", "[]");

All we need to do here is effectively "revert" the effect of this transformation before searching for the input text.

jonathan-gibbons · 2024-05-28T22:10:41Z

/reviewers 2 reviewer

openjdk · 2024-05-28T22:10:53Z

@jonathan-gibbons
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

lahodaj · 2024-05-31T05:52:55Z

/integrate

openjdk · 2024-05-31T05:54:40Z

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

1b7d59f: 8333303: Issues with DottedVersion class
e304a8a: 8333307: Don't suppress jpackage logging in tests when it is detecting packaging tools in the system
3634a91: 8332751: Broken link in VirtualMachine.html
ffb0867: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent
79f4998: 8321314: Reinstate disabling the compiler's default active annotation processing
ec88c6a: 8332917: failure_handler should execute gdb "info threads" command on linux
b3e29db: 8333108: Update vmTestbase/nsk/share/DebugeeProcess.java to don't use finalization
11e926c: 8332777: Update JCStress test suite
44c1845: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly
922e312: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject
... and 103 more: https://git.openjdk.org/jdk/compare/2581935b47afaf661a94c8a8e50ce08065d632f6...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-31T05:54:46Z

@lahodaj Pushed as commit 2ab8ab5.

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

lahodaj added 2 commits May 24, 2024 08:05

8332858: References with escapes have broken positions after they are…

a90d9ae

… transformed

Fixing prefix offset.

b75f896

openjdk bot added the rfr Pull request is ready for review label May 24, 2024

openjdk bot added javadoc javadoc-dev@openjdk.org compiler compiler-dev@openjdk.org labels May 24, 2024

openjdk bot mentioned this pull request May 24, 2024

8325168: JShell should support Markdown comments #17686

Closed

3 tasks

vicente-romero-oracle reviewed May 25, 2024

View reviewed changes

vicente-romero-oracle approved these changes May 28, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 28, 2024

jonathan-gibbons suggested changes May 28, 2024

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label May 28, 2024

lahodaj added 2 commits May 29, 2024 13:26

Adjusting according to the review suggestions.

e9fac5f

Fixing incorrect position.

5871d11

jonathan-gibbons approved these changes May 29, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 29, 2024

openjdk bot added the integrated Pull request has been integrated label May 31, 2024

openjdk bot closed this May 31, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8332858: References with escapes have broken positions after they are transformed #19387

8332858: References with escapes have broken positions after they are transformed #19387

lahodaj commented May 24, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 24, 2024

openjdk bot commented May 24, 2024 •

edited

Loading

openjdk bot commented May 24, 2024

mlbridge bot commented May 24, 2024 •

edited

Loading

vicente-romero-oracle May 25, 2024

lahodaj May 28, 2024

vicente-romero-oracle May 28, 2024

jonathan-gibbons May 28, 2024 •

edited

Loading

vicente-romero-oracle left a comment

jonathan-gibbons May 28, 2024

jonathan-gibbons May 28, 2024

jonathan-gibbons May 28, 2024 •

edited

Loading

jonathan-gibbons commented May 28, 2024

openjdk bot commented May 28, 2024

lahodaj commented May 31, 2024

openjdk bot commented May 31, 2024

openjdk bot commented May 31, 2024

		import java.util.regex.Matcher;
		import jdk.internal.org.commonmark.internal.util.Escaping;

8332858: References with escapes have broken positions after they are transformed #19387

8332858: References with escapes have broken positions after they are transformed #19387

Conversation

lahodaj commented May 24, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 24, 2024

openjdk bot commented May 24, 2024 • edited Loading

openjdk bot commented May 24, 2024

mlbridge bot commented May 24, 2024 • edited Loading

Webrevs

vicente-romero-oracle May 25, 2024

Choose a reason for hiding this comment

lahodaj May 28, 2024

Choose a reason for hiding this comment

vicente-romero-oracle May 28, 2024

Choose a reason for hiding this comment

jonathan-gibbons May 28, 2024 • edited Loading

Choose a reason for hiding this comment

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

jonathan-gibbons May 28, 2024

Choose a reason for hiding this comment

jonathan-gibbons May 28, 2024

Choose a reason for hiding this comment

jonathan-gibbons May 28, 2024 • edited Loading

Choose a reason for hiding this comment

jonathan-gibbons commented May 28, 2024

openjdk bot commented May 28, 2024

lahodaj commented May 31, 2024

openjdk bot commented May 31, 2024

openjdk bot commented May 31, 2024

lahodaj commented May 24, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 24, 2024 •

edited

Loading

mlbridge bot commented May 24, 2024 •

edited

Loading

jonathan-gibbons May 28, 2024 •

edited

Loading

jonathan-gibbons May 28, 2024 •

edited

Loading