Skip to content

Commit e127875

Browse files
asm0deyclaude
authored andcommitted
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) <noreply@anthropic.com> (cherry picked from commit 0f3a437)
1 parent 2fbf175 commit e127875

3 files changed

Lines changed: 143 additions & 0 deletions

File tree

‎open-java-format/src/main/java/com/palantir/javaformat/java/JavaCommentsHelper.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,12 @@ private static String lineCommentPrefix(String line) {
183183
private List<String> wrapLineComments(List<String> lines, int column0) {
184184
List<String> result = new ArrayList<>();
185185
for (String line : lines) {
186+
// From JDK 23 on, javac returns a run of `///` markdown lines as a single comment tok, so
187+
// every line after the first still carries its source indentation. indentLineComments
188+
// trims and re-indents all of them, so both the slash prefix and the width budget have to
189+
// be read from the trimmed text: otherwise the prefix comes back empty and the wrapped
190+
// remainder is emitted as bare code.
191+
line = CharMatcher.whitespace().trimLeadingFrom(line);
186192
// Add missing leading spaces to line comments: `//foo` -> `// foo`.
187193
Matcher matcher = LINE_COMMENT_MISSING_SPACE_PREFIX.matcher(line);
188194
if (matcher.find()) {

‎open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import java.nio.file.Path;
3636
import java.time.Duration;
3737
import java.util.List;
38+
import org.junit.jupiter.api.Assumptions;
3839
import org.junit.jupiter.api.Test;
3940
import org.junit.jupiter.api.io.TempDir;
4041
import org.junit.jupiter.api.parallel.Execution;
@@ -418,6 +419,28 @@ void wrapMarkdownDocstringComment() throws Exception {
418419
assertThat(Formatter.create().formatSource(input)).isEqualTo(expected);
419420
}
420421

422+
@Test
423+
void wrapMarkdownDocstringRunKeepsTheSlashPrefix() throws Exception {
424+
// javac returns a run of `///` lines as one comment token from JDK 23 on; before that each line is its
425+
// own token and the wrapped line is always the first of its token, which is never misread.
426+
Assumptions.assumeTrue(
427+
Formatter.getRuntimeVersion() >= 23, "a `///` run is one comment token only from JDK 23 on");
428+
String input = "class T {\n"
429+
+ " /// Summary line.\n"
430+
+ " /// one long incredibly unbroken sentence moving from topic to topic so that no-one had a"
431+
+ " chance to interrupt; @Deprecated\n"
432+
+ " void m() {}\n"
433+
+ "}\n";
434+
String expected = "class T {\n"
435+
+ " /// Summary line.\n"
436+
+ " /// one long incredibly unbroken sentence moving from topic to topic so that no-one had a"
437+
+ " chance\n"
438+
+ " /// to interrupt; @Deprecated\n"
439+
+ " void m() {}\n"
440+
+ "}\n";
441+
assertThat(Formatter.create().formatSource(input)).isEqualTo(expected);
442+
}
443+
421444
@Test
422445
void dontWrapMoeLineComments() throws Exception {
423446
assertThat(Formatter.create()
Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
/*
2+
* (c) Copyright 2026 Palantir Technologies Inc. All rights reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.palantir.javaformat.java;
18+
19+
import static org.assertj.core.api.Assertions.assertThat;
20+
21+
import com.palantir.javaformat.java.JavaFormatterOptions.Style;
22+
import org.junit.jupiter.api.Test;
23+
24+
/**
25+
* Tests comment rewriting against the comment toks javac produces, without needing the JDK that produces them. From
26+
* JDK 23 on, a run of {@code ///} markdown lines (JEP 467) arrives as a single comment tok whose lines after the first
27+
* carry their source indentation; before 23 each {@code //} line is its own tok. Building the tok directly covers the
28+
* multi-line shape on any JDK.
29+
*/
30+
final class JavaCommentsHelperTest {
31+
32+
private static final int COLUMN = 4;
33+
34+
/** Rewrites a comment tok that sits at column {@link #COLUMN}, as javac 23 or later would report it. */
35+
private static String rewrite(String commentText, Style style) throws FormatterException {
36+
JavaFormatterOptions options =
37+
JavaFormatterOptions.builder().style(style).build();
38+
JavaInput.Tok tok = new JavaInput.Tok(0, commentText, commentText, 0, COLUMN, false, null);
39+
// The helper reads the line separator and the index of the first code token from the input. Any input whose
40+
// first token has index 0 will do: the tok above then counts as a comment inside the code, which is wrapped.
41+
JavaInput input = new JavaInput("class T {}\n");
42+
return new JavaCommentsHelper(input, options).rewrite(tok, options.maxLineLength(), COLUMN);
43+
}
44+
45+
/** A run of {@code ///} lines, indented as javac reports it, with {@code body} as the second line's text. */
46+
private static String markdownRun(String body) {
47+
return "/// Summary line.\n" + " ".repeat(COLUMN) + "/// " + body;
48+
}
49+
50+
private static String wordsOfLength(int length) {
51+
StringBuilder sb = new StringBuilder();
52+
while (sb.length() < length) {
53+
sb.append(sb.length() == 0 ? "" : " ").append("word");
54+
}
55+
return sb.substring(0, length);
56+
}
57+
58+
@Test
59+
void everyWrappedLineKeepsTheSlashPrefix() throws Exception {
60+
// Before the fix the prefix was read from the untrimmed continuation line, came back empty, and the
61+
// overflow was emitted as bare code -- which stops being a comment at all.
62+
String rewritten = rewrite(markdownRun(wordsOfLength(140) + " @Deprecated"), Style.OJF);
63+
64+
assertThat(rewritten).contains("\n");
65+
assertThat(rewritten.lines()).allSatisfy(line -> assertThat(line.trim()).startsWith("///"));
66+
assertThat(rewritten).doesNotContain("\n @Deprecated");
67+
}
68+
69+
@Test
70+
void aContinuationLineAtTheLimitIsNotWrapped() throws Exception {
71+
// The line's visual width is COLUMN + its trimmed length, so the budget is maxLineLength - COLUMN.
72+
int budget = Style.OJF.maxLineLength() - COLUMN;
73+
String body = wordsOfLength(budget - "/// ".length());
74+
75+
assertThat(rewrite(markdownRun(body), Style.OJF).lines()).hasSize(2);
76+
}
77+
78+
@Test
79+
void aContinuationLineOverTheLimitIsWrapped() throws Exception {
80+
int budget = Style.OJF.maxLineLength() - COLUMN;
81+
String body = wordsOfLength(budget - "/// ".length() + 1);
82+
83+
assertThat(rewrite(markdownRun(body), Style.OJF).lines()).hasSize(3);
84+
}
85+
86+
@Test
87+
void anUnbreakableTokenIsLeftLong() throws Exception {
88+
// There is nowhere to break, so the line stays over the limit rather than becoming an empty `///`
89+
// followed by a bare URL.
90+
String url = "https://example.com/" + "a".repeat(Style.OJF.maxLineLength());
91+
92+
String rewritten = rewrite(markdownRun(url), Style.OJF);
93+
94+
assertThat(rewritten.lines()).hasSize(2);
95+
assertThat(rewritten).contains("/// " + url);
96+
}
97+
98+
@Test
99+
void theMissingSpaceRuleAppliesToEveryLineOfTheRun() throws Exception {
100+
// Otherwise the same source formats differently depending on whether the running JDK hands the run
101+
// over as one tok (23 and later) or as one tok per line.
102+
String rewritten = rewrite("///Summary line.\n" + " ".repeat(COLUMN) + "///More text.", Style.OJF);
103+
104+
assertThat(rewritten).isEqualTo("/// Summary line.\n" + " ".repeat(COLUMN) + "/// More text.");
105+
}
106+
107+
@Test
108+
void anOrdinaryLineCommentStillWraps() throws Exception {
109+
String rewritten = rewrite("// " + wordsOfLength(140), Style.OJF);
110+
111+
assertThat(rewritten.lines()).hasSize(2);
112+
assertThat(rewritten.lines()).allSatisfy(line -> assertThat(line.trim()).startsWith("// "));
113+
}
114+
}

0 commit comments

Comments
 (0)