From e1278751371929691935b3806b5a51d818e5a18d Mon Sep 17 00:00:00 2001 From: asm0dey Date: Fri, 11 Sep 2026 21:26:33 +0200 Subject: [PATCH] fix: keep the comment prefix when wrapping a markdown docstring From JDK 23 on, javac returns a run of `///` lines (JEP 467) as a single comment token, so every line after the first still carries its source indentation when `JavaCommentsHelper.wrapLineComments` sees it. The prefix was read from index 0 of that untrimmed line, came back empty, and the wrapped remainder was emitted with no `///` at all -- as bare code. The same indentation was then counted twice in the width test, so lines under the column limit were wrapped as well. In Palantir style the result compiles, which is the dangerous part: class P1 { /// Summary line. /// This second markdown line is long enough to overflow the palantir one hundred twenty column limit @Deprecated void f() {} } formats to the comment, then `@Deprecated` on a line of its own, then the method. `javap -v` goes from zero Deprecated markers to four, `javac` exits 0, and nothing downstream notices that the formatter deprecated a method. In Google style the same input produces output that does not compile (`illegal start of type`), and formatting it again fails to parse. Both need the `///` block to be indented, which is why a `column0 == 0` case does not reproduce it. The prefix and the width budget now come from the trimmed line, which `indentLineComments` trims and re-indents anyway. An unbreakable token is left long instead of producing an empty `///` followed by bare text, and the missing-space rule (`///foo` -> `/// foo`) now applies to every line of the run, so the same source no longer formats differently depending on whether the running JDK hands the run over as one token or as one token per line. JavaCommentsHelperTest builds the multi-line comment token directly, so the JDK 21 test task covers the bug that only a JDK 23 or later parser can produce; four of its six tests fail without this change. The end-to-end test in FormatterTest is gated at 23. Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit 0f3a437490644dbe9b84ac57d06458ff0c1a1ad0) --- .../javaformat/java/JavaCommentsHelper.java | 6 + .../javaformat/java/FormatterTest.java | 23 ++++ .../java/JavaCommentsHelperTest.java | 114 ++++++++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 open-java-format/src/test/java/com/palantir/javaformat/java/JavaCommentsHelperTest.java diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/JavaCommentsHelper.java b/open-java-format/src/main/java/com/palantir/javaformat/java/JavaCommentsHelper.java index 5f4419a76..a31fb7c25 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/JavaCommentsHelper.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/JavaCommentsHelper.java @@ -183,6 +183,12 @@ private static String lineCommentPrefix(String line) { private List wrapLineComments(List lines, int column0) { List result = new ArrayList<>(); for (String line : lines) { + // From JDK 23 on, javac returns a run of `///` markdown lines as a single comment tok, so + // every line after the first still carries its source indentation. indentLineComments + // trims and re-indents all of them, so both the slash prefix and the width budget have to + // be read from the trimmed text: otherwise the prefix comes back empty and the wrapped + // remainder is emitted as bare code. + line = CharMatcher.whitespace().trimLeadingFrom(line); // Add missing leading spaces to line comments: `//foo` -> `// foo`. Matcher matcher = LINE_COMMENT_MISSING_SPACE_PREFIX.matcher(line); if (matcher.find()) { diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java index 73d290b1e..46be7a302 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java @@ -35,6 +35,7 @@ import java.nio.file.Path; import java.time.Duration; import java.util.List; +import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.api.parallel.Execution; @@ -418,6 +419,28 @@ void wrapMarkdownDocstringComment() throws Exception { assertThat(Formatter.create().formatSource(input)).isEqualTo(expected); } + @Test + void wrapMarkdownDocstringRunKeepsTheSlashPrefix() throws Exception { + // javac returns a run of `///` lines as one comment token from JDK 23 on; before that each line is its + // own token and the wrapped line is always the first of its token, which is never misread. + Assumptions.assumeTrue( + Formatter.getRuntimeVersion() >= 23, "a `///` run is one comment token only from JDK 23 on"); + String input = "class T {\n" + + " /// Summary line.\n" + + " /// one long incredibly unbroken sentence moving from topic to topic so that no-one had a" + + " chance to interrupt; @Deprecated\n" + + " void m() {}\n" + + "}\n"; + String expected = "class T {\n" + + " /// Summary line.\n" + + " /// one long incredibly unbroken sentence moving from topic to topic so that no-one had a" + + " chance\n" + + " /// to interrupt; @Deprecated\n" + + " void m() {}\n" + + "}\n"; + assertThat(Formatter.create().formatSource(input)).isEqualTo(expected); + } + @Test void dontWrapMoeLineComments() throws Exception { assertThat(Formatter.create() diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/JavaCommentsHelperTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/JavaCommentsHelperTest.java new file mode 100644 index 000000000..f3d15f6cf --- /dev/null +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/JavaCommentsHelperTest.java @@ -0,0 +1,114 @@ +/* + * (c) Copyright 2026 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.javaformat.java; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.palantir.javaformat.java.JavaFormatterOptions.Style; +import org.junit.jupiter.api.Test; + +/** + * Tests comment rewriting against the comment toks javac produces, without needing the JDK that produces them. From + * JDK 23 on, a run of {@code ///} markdown lines (JEP 467) arrives as a single comment tok whose lines after the first + * carry their source indentation; before 23 each {@code //} line is its own tok. Building the tok directly covers the + * multi-line shape on any JDK. + */ +final class JavaCommentsHelperTest { + + private static final int COLUMN = 4; + + /** Rewrites a comment tok that sits at column {@link #COLUMN}, as javac 23 or later would report it. */ + private static String rewrite(String commentText, Style style) throws FormatterException { + JavaFormatterOptions options = + JavaFormatterOptions.builder().style(style).build(); + JavaInput.Tok tok = new JavaInput.Tok(0, commentText, commentText, 0, COLUMN, false, null); + // The helper reads the line separator and the index of the first code token from the input. Any input whose + // first token has index 0 will do: the tok above then counts as a comment inside the code, which is wrapped. + JavaInput input = new JavaInput("class T {}\n"); + return new JavaCommentsHelper(input, options).rewrite(tok, options.maxLineLength(), COLUMN); + } + + /** A run of {@code ///} lines, indented as javac reports it, with {@code body} as the second line's text. */ + private static String markdownRun(String body) { + return "/// Summary line.\n" + " ".repeat(COLUMN) + "/// " + body; + } + + private static String wordsOfLength(int length) { + StringBuilder sb = new StringBuilder(); + while (sb.length() < length) { + sb.append(sb.length() == 0 ? "" : " ").append("word"); + } + return sb.substring(0, length); + } + + @Test + void everyWrappedLineKeepsTheSlashPrefix() throws Exception { + // Before the fix the prefix was read from the untrimmed continuation line, came back empty, and the + // overflow was emitted as bare code -- which stops being a comment at all. + String rewritten = rewrite(markdownRun(wordsOfLength(140) + " @Deprecated"), Style.OJF); + + assertThat(rewritten).contains("\n"); + assertThat(rewritten.lines()).allSatisfy(line -> assertThat(line.trim()).startsWith("///")); + assertThat(rewritten).doesNotContain("\n @Deprecated"); + } + + @Test + void aContinuationLineAtTheLimitIsNotWrapped() throws Exception { + // The line's visual width is COLUMN + its trimmed length, so the budget is maxLineLength - COLUMN. + int budget = Style.OJF.maxLineLength() - COLUMN; + String body = wordsOfLength(budget - "/// ".length()); + + assertThat(rewrite(markdownRun(body), Style.OJF).lines()).hasSize(2); + } + + @Test + void aContinuationLineOverTheLimitIsWrapped() throws Exception { + int budget = Style.OJF.maxLineLength() - COLUMN; + String body = wordsOfLength(budget - "/// ".length() + 1); + + assertThat(rewrite(markdownRun(body), Style.OJF).lines()).hasSize(3); + } + + @Test + void anUnbreakableTokenIsLeftLong() throws Exception { + // There is nowhere to break, so the line stays over the limit rather than becoming an empty `///` + // followed by a bare URL. + String url = "https://example.com/" + "a".repeat(Style.OJF.maxLineLength()); + + String rewritten = rewrite(markdownRun(url), Style.OJF); + + assertThat(rewritten.lines()).hasSize(2); + assertThat(rewritten).contains("/// " + url); + } + + @Test + void theMissingSpaceRuleAppliesToEveryLineOfTheRun() throws Exception { + // Otherwise the same source formats differently depending on whether the running JDK hands the run + // over as one tok (23 and later) or as one tok per line. + String rewritten = rewrite("///Summary line.\n" + " ".repeat(COLUMN) + "///More text.", Style.OJF); + + assertThat(rewritten).isEqualTo("/// Summary line.\n" + " ".repeat(COLUMN) + "/// More text."); + } + + @Test + void anOrdinaryLineCommentStillWraps() throws Exception { + String rewritten = rewrite("// " + wordsOfLength(140), Style.OJF); + + assertThat(rewritten.lines()).hasSize(2); + assertThat(rewritten.lines()).allSatisfy(line -> assertThat(line.trim()).startsWith("// ")); + } +}