Skip to content

Commit d17c6d7

Browse files
authored
Merge pull request #169 from openjavaformat/fix/wrapped-trailing-comment
Keep a wrapped trailing line comment where the first run put it
2 parents c3d9e78 + 08e470a commit d17c6d7

13 files changed

Lines changed: 302 additions & 48 deletions

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -551,7 +551,7 @@ public OpsOutput build() {
551551
tokBefore.isSlashSlashComment() ? FillMode.FORCED : FillMode.UNIFIED,
552552
"",
553553
tokenOp.getPlusIndentCommentsBefore()));
554-
tokOps.putAll(j, makeComment(tokBefore));
554+
tokOps.putAll(j, makeComment(tokBefore, false));
555555
space = tokBefore.isSlashStarComment();
556556
newlines = 0;
557557
lastWasComment = true;
@@ -621,7 +621,7 @@ public OpsOutput build() {
621621
tokOps.put(tokAfterPos, Comment.makeNonNlsMarkers(tokAfter, literals));
622622
tokOps.put(tokAfterPos, Break.makeForced());
623623
} else {
624-
tokOps.putAll(tokAfterPos, makeComment(tokAfter));
624+
tokOps.putAll(tokAfterPos, makeComment(tokAfter, true));
625625
}
626626
if (breakAfter) {
627627
tokOps.put(tokAfterPos, Break.make(FillMode.FORCED, "", ZERO));
@@ -731,10 +731,9 @@ private static boolean isForcedBreak(Op op) {
731731
return op instanceof Break b && b.isForced();
732732
}
733733

734-
private static List<Op> makeComment(Input.Tok comment) {
735-
return comment.isSlashStarComment()
736-
? ImmutableList.of(Comment.make(comment))
737-
: ImmutableList.of(Comment.make(comment), Break.makeForced());
734+
private static List<Op> makeComment(Input.Tok comment, boolean trailing) {
735+
var doc = trailing ? Comment.makeTrailing(comment) : Comment.make(comment);
736+
return comment.isSlashStarComment() ? List.of(doc) : List.of(doc, Break.makeForced());
738737
}
739738

740739
@Override

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

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,17 @@ public final class Comment extends Doc implements Op {
3535
/** For a comment of NON-NLS markers, the index of the string literal each marker marks, or -1; else empty. */
3636
private final List<Integer> nonNlsLiterals;
3737

38-
private Comment(Input.Tok tok, List<Integer> nonNlsLiterals) {
38+
/** Whether the comment trails code on its line. */
39+
private final boolean trailing;
40+
41+
/** Set by {@link #withFirstWordOnly}. */
42+
private final boolean firstWordOnly;
43+
44+
private Comment(Input.Tok tok, List<Integer> nonNlsLiterals, boolean trailing, boolean firstWordOnly) {
3945
this.tok = tok;
4046
this.nonNlsLiterals = nonNlsLiterals;
47+
this.trailing = trailing;
48+
this.firstWordOnly = firstWordOnly;
4149
}
4250

4351
/**
@@ -47,7 +55,12 @@ private Comment(Input.Tok tok, List<Integer> nonNlsLiterals) {
4755
* @return the new {@code Tok}
4856
*/
4957
public static Comment make(Input.Tok tok) {
50-
return new Comment(tok, List.of());
58+
return new Comment(tok, List.of(), false, false);
59+
}
60+
61+
/** A comment that trails code on its line. */
62+
public static Comment makeTrailing(Input.Tok tok) {
63+
return new Comment(tok, List.of(), true, false);
5164
}
5265

5366
/**
@@ -58,7 +71,32 @@ public static Comment make(Input.Tok tok) {
5871
* @param literals for each marker, the index of the string literal it marks, or -1
5972
*/
6073
public static Comment makeNonNlsMarkers(Input.Tok tok, List<Integer> literals) {
61-
return new Comment(tok, List.copyOf(literals));
74+
return new Comment(tok, List.copyOf(literals), false, false);
75+
}
76+
77+
/**
78+
* This comment, laid out by its slashes and its first word only, as a line comment that spans lines is. The next
79+
* run reads a wrapped trailing comment back as one that spans lines, so laying it out that way from the start gives
80+
* both runs the same layout, and the code before it is not broken for room that the comment does not get.
81+
*/
82+
public Comment withFirstWordOnly() {
83+
return new Comment(tok, nonNlsLiterals, trailing, true);
84+
}
85+
86+
/**
87+
* Whether this trailing line comment, counted in full by the layout, came out of {@code state} wrapped: it did not
88+
* fit after the code before it, however that was broken.
89+
*/
90+
public boolean wrappedIn(State state) {
91+
return trailing
92+
&& tok.isSlashSlashComment()
93+
&& !countsFirstWordOnly()
94+
&& Newlines.containsBreaks(state.getTokText(this));
95+
}
96+
97+
/** Whether the layout counts only the slashes and the first word of this line comment. */
98+
boolean countsFirstWordOnly() {
99+
return tok.isSlashSlashComment() && (firstWordOnly || Newlines.containsBreaks(tok.getOriginalText()));
62100
}
63101

64102
@Override
@@ -79,6 +117,11 @@ protected float computeWidth() {
79117
}
80118
return width;
81119
}
120+
if (countsFirstWordOnly()) {
121+
// The rest of the comment goes where the wrap puts it, so the code before it is laid out as if it were not
122+
// there; see withFirstWordOnly.
123+
return JavaCommentsHelper.firstWordLength(getFlat());
124+
}
82125
int idx = Newlines.firstBreak(tok.getOriginalText());
83126
// only count the first line of multi-line block comments
84127
if (tok.isComment()) {

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

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import java.util.stream.Collector;
4949
import java.util.stream.Collectors;
5050
import java.util.stream.Stream;
51+
import javax.annotation.Nullable;
5152

5253
/** A {@code Level} inside a {@link Doc}. */
5354
public final class Level extends Doc {
@@ -67,6 +68,10 @@ public final class Level extends Doc {
6768
@SuppressWarnings("Immutable") // Effectively immutable
6869
private final ImmutableSupplier<Integer> memoizedMaxDepth = Suppliers.memoize(() -> computeMaxDepth(docs))::get;
6970

71+
/** See {@link #hasCommentByFirstWord()}; computed when first asked for, once the docs are all in. */
72+
@Nullable
73+
private Boolean hasCommentByFirstWord;
74+
7075
/**
7176
* Where {@link #tryBreakLastLevel} has already failed. An attempt can lay out all of the last inner level before it
7277
* fails, and each level above repeats it for every layout it tries, so without this deeply nested calls took time
@@ -124,15 +129,38 @@ protected Range<Integer> computeRange() {
124129

125130
@Override
126131
public State computeBreaks(CommentsHelper commentsHelper, int maxWidth, State state, Obs.ExplorationNode observer) {
127-
return tryToFitOnOneLine(maxWidth, state, docs)
128-
.map(newWidth -> state.withColumn(newWidth).withLevelState(this, new State.LevelState(true)))
129-
.orElseGet(() -> {
130-
Obs.LevelNode childLevel = observer.newChildNode(this, state);
131-
State newState =
132-
new BreakImpl(commentsHelper, maxWidth, state, childLevel).apply(getBreakBehaviour());
133-
134-
return childLevel.finishLevel(state.updateAfterLevel(newState));
135-
});
132+
var oneLine = tryToFitOnOneLine(maxWidth, state, docs);
133+
if (oneLine.isPresent() && !hasCommentByFirstWord()) {
134+
return state.withColumn(oneLine.get()).withLevelState(this, new State.LevelState(true));
135+
}
136+
137+
var childLevel = observer.newChildNode(this, state);
138+
State newState;
139+
if (oneLine.isPresent()) {
140+
// A level that fits is written from its flat text, which would skip wrapping a comment that counted
141+
// only its first word. So it is laid out with none of its breaks taken, and the comment is wrapped
142+
// where it needs to.
143+
newState = childLevel
144+
.explore(
145+
"one line, comments laid out",
146+
state,
147+
explorationNode -> tryToLayOutLevelOnOneLine(
148+
commentsHelper, maxWidth, state, memoizedSplitsBreaks.get(), explorationNode))
149+
.markAccepted();
150+
} else {
151+
newState = new BreakImpl(commentsHelper, maxWidth, state, childLevel).apply(getBreakBehaviour());
152+
}
153+
return childLevel.finishLevel(state.updateAfterLevel(newState));
154+
}
155+
156+
/** Whether this level or a level in it holds a comment that the layout counts by its first word only. */
157+
private boolean hasCommentByFirstWord() {
158+
if (hasCommentByFirstWord == null) {
159+
hasCommentByFirstWord = docs.stream()
160+
.anyMatch(doc -> (doc instanceof Comment comment && comment.countsFirstWordOnly())
161+
|| (doc instanceof Level level && level.hasCommentByFirstWord()));
162+
}
163+
return hasCommentByFirstWord;
136164
}
137165

138166
/**

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

Lines changed: 44 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import com.palantir.javaformat.OpsBuilder;
3131
import com.palantir.javaformat.OpsBuilder.OpsOutput;
3232
import com.palantir.javaformat.Utils;
33+
import com.palantir.javaformat.doc.Comment;
3334
import com.palantir.javaformat.doc.Doc;
3435
import com.palantir.javaformat.doc.DocBuilder;
3536
import com.palantir.javaformat.doc.Level;
@@ -48,6 +49,9 @@
4849
import java.io.UncheckedIOException;
4950
import java.net.URI;
5051
import java.util.Collection;
52+
import java.util.HashSet;
53+
import java.util.List;
54+
import java.util.Set;
5155
import javax.tools.Diagnostic;
5256
import javax.tools.DiagnosticCollector;
5357
import javax.tools.DiagnosticListener;
@@ -150,14 +154,27 @@ static JavaOutput format(
150154
opsBuilder.drain();
151155
OpsOutput opsOutput = opsBuilder.build();
152156

153-
Level doc = new DocBuilder().withOps(opsOutput.ops()).build();
154-
155-
// Don't even allocate all those JSON nodes if we're not going to write it out
156-
Sink sink = debugMode ? new JsonSink() : new NoopSink();
157-
158-
Obs.ExplorationNode observationNode = Obs.createRoot(sink);
159-
State finalState =
160-
doc.computeBreaks(commentsHelper, options.maxLineLength(), State.startingState(), observationNode);
157+
// A trailing line comment that came out wrapped did not fit however the code before it was broken. Laid
158+
// out again by its first word only, as the next run lays out the wrapped comment it reads, it leaves that
159+
// code as it would be without the comment, and the next run gives the same layout.
160+
List<Op> ops = opsOutput.ops();
161+
Level doc;
162+
Sink sink;
163+
State finalState;
164+
while (true) {
165+
doc = new DocBuilder().withOps(ops).build();
166+
167+
// Don't even allocate all those JSON nodes if we're not going to write it out
168+
sink = debugMode ? new JsonSink() : new NoopSink();
169+
finalState = doc.computeBreaks(
170+
commentsHelper, options.maxLineLength(), State.startingState(), Obs.createRoot(sink));
171+
172+
var wrapped = wrappedComments(ops, finalState);
173+
if (wrapped.isEmpty()) {
174+
break;
175+
}
176+
ops = withFirstWordOnly(ops, wrapped);
177+
}
161178

162179
JavaOutput javaOutput = new JavaOutput(javaInput, opsOutput.inputMetadata());
163180
doc.write(finalState, javaOutput);
@@ -169,6 +186,25 @@ static JavaOutput format(
169186
return javaOutput;
170187
}
171188

189+
/** The comments of {@code ops} that came out of {@code state} wrapped, see {@link Comment#wrappedIn}. */
190+
private static Set<Comment> wrappedComments(List<Op> ops, State state) {
191+
var wrapped = new HashSet<Comment>();
192+
for (var op : ops) {
193+
if (op instanceof Comment comment && comment.wrappedIn(state)) {
194+
wrapped.add(comment);
195+
}
196+
}
197+
return wrapped;
198+
}
199+
200+
/** {@code ops} with each of the {@code wrapped} comments laid out by its first word only. */
201+
private static List<Op> withFirstWordOnly(List<Op> ops, Set<Comment> wrapped) {
202+
return ops.stream()
203+
.map(op ->
204+
op instanceof Comment comment && wrapped.contains(comment) ? comment.withFirstWordOnly() : op)
205+
.toList();
206+
}
207+
172208
static JCCompilationUnit parseJcCompilationUnit(Context context, String sourceText) throws FormatterException {
173209
DiagnosticCollector<JavaFileObject> diagnostics = new DiagnosticCollector<>();
174210
context.put(DiagnosticListener.class, diagnostics);

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,14 +189,28 @@ public static String addMissingSpace(String line) {
189189
return line;
190190
}
191191

192-
private static String lineCommentPrefix(String line) {
192+
static String lineCommentPrefix(String line) {
193193
int prefixLength = 0;
194194
while (prefixLength < line.length() && line.charAt(prefixLength) == '/') {
195195
prefixLength++;
196196
}
197197
return "/".repeat(prefixLength);
198198
}
199199

200+
/**
201+
* The length of the slashes and the first word of a line comment, which stay on its line however it is wrapped:
202+
* {@link #wrapLineComments} breaks a line only at whitespace after them.
203+
*/
204+
public static int firstWordLength(String line) {
205+
var prefixLength = lineCommentPrefix(line).length();
206+
for (var i = prefixLength + 1; i < line.length(); i++) {
207+
if (CharMatcher.breakingWhitespace().matches(line.charAt(i))) {
208+
return i;
209+
}
210+
}
211+
return line.length();
212+
}
213+
200214
private List<String> wrapLineComments(List<String> lines, int column0) {
201215
List<String> result = new ArrayList<>();
202216
for (String line : lines) {

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

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -445,7 +445,10 @@ public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOExcept
445445
}
446446
}
447447
if (strings.size() == 1) {
448-
toks.add(new Tok(isNumbered ? kN++ : -1, originalTokText, tokText, charI, columnI, isToken, t.kind()));
448+
if (!tokText.startsWith("//") || !continueTrailingComment(toks, originalTokText, text)) {
449+
toks.add(new Tok(
450+
isNumbered ? kN++ : -1, originalTokText, tokText, charI, columnI, isToken, t.kind()));
451+
}
449452
charI += originalTokText.length();
450453
columnI = updateColumn(columnI, originalTokText);
451454

@@ -497,6 +500,56 @@ private static Collection<JCDiagnostic> getDiagnostics(DeferredDiagnosticHandler
497500
}
498501
}
499502

503+
/**
504+
* Adds {@code comment}, a line comment that starts a line of its own, to the line comment that trails code on the
505+
* line above when it starts in that comment's column, the way the formatter writes a trailing comment it wraps. The
506+
* two become one tok, with the line break and the indentation between them, as a run of {@code ///} lines from JDK
507+
* 23 on is. Read as a comment of its own, the continuation went to the indentation of the code on the next run.
508+
*
509+
* @param toks the toks so far, which end with the trailing comment, a newline and the indentation of
510+
* {@code comment} when it continues one
511+
* @return whether {@code comment} continues the trailing comment and is now part of it
512+
*/
513+
private static boolean continueTrailingComment(List<Tok> toks, String comment, String text) {
514+
var n = toks.size();
515+
if (n < 3) {
516+
return false;
517+
}
518+
519+
var trailing = toks.get(n - 3);
520+
var newline = toks.get(n - 2);
521+
var indentation = toks.get(n - 1);
522+
if (!trailing.isSlashSlashComment() || !newline.isNewline() || !isSpaces(indentation)) {
523+
return false;
524+
}
525+
526+
// What comes before the trailing comment on its line is read in the text, since Tok.getColumn() counts an
527+
// operator such as == once for each of its characters. Code there makes the comment a trailing one, and its
528+
// length is the comment's column.
529+
var position = trailing.getPosition();
530+
var lineStart = Math.max(text.lastIndexOf('\n', position), text.lastIndexOf('\r', position)) + 1;
531+
var beforeComment = text.substring(lineStart, position);
532+
if (beforeComment.isBlank()
533+
|| beforeComment.length() != indentation.length()
534+
|| !JavaCommentsHelper.lineCommentPrefix(trailing.getOriginalText())
535+
.equals(JavaCommentsHelper.lineCommentPrefix(comment))
536+
|| NonNlsMarkers.isMarkersOnly(trailing.getOriginalText())
537+
|| NonNlsMarkers.isMarkersOnly(comment)) {
538+
return false;
539+
}
540+
541+
var continued =
542+
trailing.getOriginalText() + newline.getOriginalText() + indentation.getOriginalText() + comment;
543+
toks.subList(n - 3, n).clear();
544+
toks.add(new Tok(
545+
trailing.getIndex(), continued, continued, position, trailing.getColumn(), false, trailing.kind()));
546+
return true;
547+
}
548+
549+
private static boolean isSpaces(Tok tok) {
550+
return !tok.isToken() && !tok.isNewline() && !tok.isComment();
551+
}
552+
500553
@SuppressWarnings("for-rollout:NullAway")
501554
private static int updateColumn(int columnI, String originalTokText) {
502555
Integer last = Iterators.getLast(Newlines.lineOffsetIterator(originalTokText));

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

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -212,26 +212,6 @@ void trailingCommentWithoutTerminalNewline() throws Exception {
212212
assertThat(FORMATTER.formatSource("/*\n * my comment */")).isEqualTo("/*\n * my comment */\n");
213213
}
214214

215-
@Test
216-
void wrapLineComment() throws Exception {
217-
// Not a case in test-cases yet: a second run moves the wrapped part of the comment to the indent of the block,
218-
// and the cases check that a second run changes nothing.
219-
assertThat(FORMATTER.formatSource("class T {\n"
220-
+ " public static void main(String[] args) { // one long incredibly"
221-
+ " unbroken sentence moving from topic to topic so that no-one had a"
222-
+ " chance to interrupt;\n"
223-
+ " }\n"
224-
+ "}\n"))
225-
.isEqualTo("class T {\n"
226-
+ " public static void main(\n"
227-
+ " String[]\n"
228-
+ " args) { // one long incredibly unbroken sentence moving"
229-
+ " from topic to topic so that no-one had a\n"
230-
+ " // chance to interrupt;\n"
231-
+ " }\n"
232-
+ "}\n");
233-
}
234-
235215
@Test
236216
void throwsFormatterException() throws Exception {
237217
assertThatThrownBy(() -> FORMATTER.formatSourceAndFixImports("package foo; public class {"))

0 commit comments

Comments
 (0)