From 0e05ef7dfdc196da564d8abf51b68517c38c32b0 Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Sun, 25 Oct 2026 15:02:33 +0300 Subject: [PATCH 1/9] Keep //$NON-NLS-n$ and //noinspection markers intact after a statement that fits on one line A trailing line comment lands in the level of the statement's last break, while its forced line break lands in the enclosing level. When that level fits on one line it is written from its flat text, and Comment.computeFlat added the space after the slashes itself, bypassing the exemption that JavaCommentsHelper.rewrite has for the IDE markers: `String a = "x"; //$NON-NLS-1$` came out as `// $NON-NLS-1$`, which Eclipse no longer recognises. The missing-space rule is now one method, JavaCommentsHelper.addMissingSpace, applied by both paths, and the comment's width follows its flat text instead of a hand-maintained + 1. A trailing `///` comment that already had its space is no longer turned into `// /` either: 4 of the 15,747 JDK 21 sources change, all that way. The tests were written first: trailingLineCommentGetsItsMissingSpaceExceptIdeMarkers and the ojf-issue-23-non-nls-trailing-marker fixture failed before the change. The first of the three cases of #23. Same defect upstream: google/google-java-format#221, google/google-java-format#361. --- .../com/palantir/javaformat/doc/Comment.java | 17 +++++----- .../javaformat/java/JavaCommentsHelper.java | 22 ++++++++---- .../javaformat/java/FormatterTest.java | 34 +++++++++++++++++++ ...ojf-issue-23-non-nls-trailing-marker.input | 20 +++++++++++ ...jf-issue-23-non-nls-trailing-marker.output | 20 +++++++++++ 5 files changed, 99 insertions(+), 14 deletions(-) create mode 100644 open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-non-nls-trailing-marker.input create mode 100644 open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-non-nls-trailing-marker.output diff --git a/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java b/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java index 3750f74cf..6386d595c 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java @@ -25,6 +25,7 @@ import com.palantir.javaformat.Newlines; import com.palantir.javaformat.Op; import com.palantir.javaformat.Output; +import com.palantir.javaformat.java.JavaCommentsHelper; /** A leaf node in a {@link Doc} for a non-token. */ @Immutable @@ -57,9 +58,9 @@ protected float computeWidth() { if (tok.isComment()) { if (idx > 0) { return idx; - } else if (tok.isSlashSlashComment() && !tok.getOriginalText().startsWith("// ")) { - // Account for line comments with missing spaces, see computeFlat. - return tok.length() + 1; + } else if (tok.isSlashSlashComment()) { + // The flat text may have gained the space after the slashes, see computeFlat. + return getFlat().length(); } else { return tok.length(); } @@ -69,11 +70,11 @@ protected float computeWidth() { @Override protected String computeFlat() { - // TODO(cushon): commentsHelper.rewrite doesn't get called for spans that fit in a single - // line. That's fine for multi-line comment reflowing, but problematic for adding missing - // spaces in line comments. - if (tok.isSlashSlashComment() && !tok.getOriginalText().startsWith("// ")) { - return "// " + tok.getOriginalText().substring("//".length()); + // A level that fits on one line is written from its flat text, and commentsHelper.rewrite is not called for + // the comments in it. So the space a line comment gets after its slashes is added here as well, by the rule + // that rewrite applies: a trailing `//foo` and a trailing `//$NON-NLS-1$` then come out the same either way. + if (tok.isSlashSlashComment()) { + return JavaCommentsHelper.addMissingSpace(tok.getOriginalText()); } return tok.getOriginalText(); } 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 83996c93d..cd0d70fdc 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 @@ -174,6 +174,21 @@ private static boolean isJBangDirective(String text) { private static final Pattern LINE_COMMENT_NO_SPACE_PREFIX = Pattern.compile("^//+(noinspection|\\$NON-NLS-\\d+\\$)"); + /** + * Adds the missing space after the slashes of a line comment: {@code //foo} becomes {@code // foo}. The IDE markers + * {@code //noinspection} and {@code //$NON-NLS-n$} stay as written, since their tools recognise them only without + * the space. {@link com.palantir.javaformat.doc.Comment#computeFlat()} applies the same rule to a comment that is + * written flat, for which {@link #rewrite} is not called. + */ + public static String addMissingSpace(String line) { + Matcher matcher = LINE_COMMENT_MISSING_SPACE_PREFIX.matcher(line); + if (matcher.find() && !LINE_COMMENT_NO_SPACE_PREFIX.matcher(line).find()) { + int length = matcher.group(1).length(); + return "/".repeat(length) + " " + line.substring(length); + } + return line; + } + private static String lineCommentPrefix(String line) { int prefixLength = 0; while (prefixLength < line.length() && line.charAt(prefixLength) == '/') { @@ -191,12 +206,7 @@ private List wrapLineComments(List lines, int column0) { // 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() && !LINE_COMMENT_NO_SPACE_PREFIX.matcher(line).find()) { - int length = matcher.group(1).length(); - line = "/".repeat(length) + " " + line.substring(length); - } + line = addMissingSpace(line); if (line.startsWith("// MOE:")) { // don't wrap comments for https://github.com/google/MOE result.add(line); 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 9e4d24b3f..15539cc56 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 @@ -399,6 +399,40 @@ void lineCommentGetsItsMissingSpaceExceptIdeMarkers() throws Exception { assertThat(Formatter.create().formatSource(input)).isEqualTo(expected); } + @Test + void trailingLineCommentGetsItsMissingSpaceExceptIdeMarkers() throws Exception { + // The same rule for a comment that trails a statement which fits on one line. Such a statement is written + // from its flat text, which skips the comment rewriting, so the IDE markers used to get the space too (#23). + String input = """ + class T { + void m() { + String a = "x"; //$NON-NLS-1$ + String b = "y"; //noinspection unchecked + String c = "z"; //foo + String d = "w"; ///foo + String e = "v"; // bar + String f = "u"; /// baz + } + } + """; + String expected = """ + class T { + void m() { + String a = "x"; //$NON-NLS-1$ + String b = "y"; //noinspection unchecked + String c = "z"; // foo + String d = "w"; /// foo + String e = "v"; // bar + String f = "u"; /// baz + } + } + """; + + String once = Formatter.create().formatSource(input); + assertThat(once).isEqualTo(expected); + assertThat(Formatter.create().formatSource(once)).isEqualTo(once); + } + @Test void throwsFormatterException() throws Exception { assertThatThrownBy(() -> Formatter.create().formatSourceAndFixImports("package foo; public class {")) diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-non-nls-trailing-marker.input b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-non-nls-trailing-marker.input new file mode 100644 index 000000000..19ef37cda --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-non-nls-trailing-marker.input @@ -0,0 +1,20 @@ +class NonNlsTrailingMarker { + private static final String FIELD = "field"; //$NON-NLS-1$ + + String markers() { + String a = "x"; //$NON-NLS-1$ + String two = "x" + "y"; //$NON-NLS-1$ //$NON-NLS-2$ + log( "message" ); //$NON-NLS-1$ + return a; //noinspection ReturnOfNull + } + + void ownLine() { + //$NON-NLS-1$ + String b = "x"; + } + + void otherComments() { + String c = "x"; //foo + String d = "x"; // bar + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-non-nls-trailing-marker.output b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-non-nls-trailing-marker.output new file mode 100644 index 000000000..a566f2bed --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-non-nls-trailing-marker.output @@ -0,0 +1,20 @@ +class NonNlsTrailingMarker { + private static final String FIELD = "field"; //$NON-NLS-1$ + + String markers() { + String a = "x"; //$NON-NLS-1$ + String two = "x" + "y"; //$NON-NLS-1$ //$NON-NLS-2$ + log("message"); //$NON-NLS-1$ + return a; //noinspection ReturnOfNull + } + + void ownLine() { + //$NON-NLS-1$ + String b = "x"; + } + + void otherComments() { + String c = "x"; // foo + String d = "x"; // bar + } +} From 8c527b265e3891deac7f988a2b8a62538562815e Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Sun, 25 Oct 2026 15:03:13 +0300 Subject: [PATCH 2/9] Keep //$NON-NLS-n$ markers on the line of their string literal when a statement is wrapped or reflowed Eclipse numbers a marker by the position of its literal on the line and reads it only there. The formatter left every marker of a wrapped call on the call's last line, and gave a reflowed literal one marker, on its last piece. OpsBuilder.build now pairs each marker of a markers-only trailing comment with the n-th string literal of the comment's input line, within the same partial-format region, and puts a zero-text ReservedWidth after that literal, so the layout counts the marker on the literal's line and a second run changes nothing. JavaOutput.appendNonNlsMarkers writes each paired marker at the end of its literal's output line, numbered by the literal's position among the string literals written there; a marker whose number has no literal stays where it was. JavaCommentsHelper never wraps a comment of markers. StringWrapper recomputes the markers of a reflowed chain's line from the new line structure, one marker after every piece and the markers of the literals before and after the chain renumbered for their lines, and reserves their room per line; a mixed comment, a second changing chain on the line, or a chain that ends in a text block leaves the markers alone. The tests were written first: the fixtures ojf-issue-23-wrapped-arguments, ojf-issue-23-reflowed-literal and ojf-issue-23-marked-literals, with nonNlsMarkersStayOnTheLineOfTheirLiteral and aRunOfNonNlsMarkersIsNeverWrapped, failed before the change. None of the 15,747 JDK 21 sources changes: the JDK carries no Eclipse markers. Fixes #23. Same defect upstream: google/google-java-format#221, google/google-java-format#361. --- .../com/palantir/javaformat/OpsBuilder.java | 69 ++++- .../java/com/palantir/javaformat/Output.java | 9 + .../com/palantir/javaformat/doc/Comment.java | 38 ++- .../javaformat/doc/ReservedWidth.java | 76 ++++++ .../javaformat/java/InputMetadataBuilder.java | 7 + .../javaformat/java/JavaCommentsHelper.java | 5 +- .../palantir/javaformat/java/JavaOutput.java | 61 +++++ .../javaformat/java/NonNlsMarkers.java | 58 ++++ .../javaformat/java/StringWrapper.java | 256 ++++++++++++++++-- .../javaformat/java/FormatterTest.java | 39 +++ .../java/JavaCommentsHelperTest.java | 10 + .../ojf-issue-23-marked-literals.input | 7 + .../ojf-issue-23-marked-literals.output | 20 ++ .../ojf-issue-23-reflowed-literal.input | 9 + .../ojf-issue-23-reflowed-literal.output | 24 ++ .../ojf-issue-23-wrapped-arguments.input | 11 + .../ojf-issue-23-wrapped-arguments.output | 30 ++ 17 files changed, 704 insertions(+), 25 deletions(-) create mode 100644 open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java create mode 100644 open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java create mode 100644 open-java-format/src/test/resources/com/palantir/javaformat/java/StringWrapperIntegrationTest/ojf-issue-23-marked-literals.input create mode 100644 open-java-format/src/test/resources/com/palantir/javaformat/java/StringWrapperIntegrationTest/ojf-issue-23-marked-literals.output create mode 100644 open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-reflowed-literal.input create mode 100644 open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-reflowed-literal.output create mode 100644 open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-wrapped-arguments.input create mode 100644 open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-wrapped-arguments.output diff --git a/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java b/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java index d8d04b160..f36e436c1 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Multimap; +import com.google.common.collect.Range; import com.google.errorprone.annotations.Immutable; import com.palantir.javaformat.Indent.Const; import com.palantir.javaformat.Input.Tok; @@ -30,14 +31,18 @@ import com.palantir.javaformat.doc.DocBuilder; import com.palantir.javaformat.doc.FillMode; import com.palantir.javaformat.doc.NonBreakingSpace; +import com.palantir.javaformat.doc.ReservedWidth; import com.palantir.javaformat.doc.State; import com.palantir.javaformat.doc.Token; import com.palantir.javaformat.doc.Token.RealOrImaginary; import com.palantir.javaformat.java.FormatterDiagnostic; import com.palantir.javaformat.java.InputMetadata; import com.palantir.javaformat.java.InputMetadataBuilder; +import com.palantir.javaformat.java.NonNlsMarkers; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Optional; /** An {@code OpsBuilder} creates a list of {@link Op}s, which is turned into a {@link Doc} by {@link DocBuilder}. */ @@ -505,6 +510,10 @@ public OpsOutput build() { markForPartialFormat(); // Rewrite the ops to insert comments. Multimap tokOps = ArrayListMultimap.create(); + // The room after a string literal for the NON-NLS markers written on its line, see Comment.makeNonNlsMarkers. + Multimap reservations = ArrayListMultimap.create(); + Map tokenOps = new HashMap<>(); + Map tokenByIndex = null; int opsN = ops.size(); for (int i = 0; i < opsN; i++) { Op op = ops.get(i); @@ -528,6 +537,7 @@ public OpsOutput build() { * Regular input token. Copy out toksBefore before token, and toksAfter after it. Insert * this token's toksBefore at position j. */ + tokenOps.put(token, i); int newlines = 0; // Count of newlines in a row. boolean space = false; // Do we need an extra space after a previous "/*" comment? boolean lastWasComment = false; // Was the last thing we output a comment? @@ -591,7 +601,30 @@ public OpsOutput build() { } else { tokOps.put(tokAfterPos, SPACE); } - tokOps.putAll(tokAfterPos, makeComment(tokAfter)); + if (tokAfter.isSlashSlashComment() && NonNlsMarkers.isMarkersOnly(tokAfter.getText())) { + if (tokenByIndex == null) { + tokenByIndex = new HashMap<>(); + for (Input.Token t : input.getTokens()) { + tokenByIndex.put(t.getTok().getIndex(), t); + } + } + ImmutableList literals = + nonNlsLiterals(token, tokAfter, tokenByIndex, tokenOps); + ImmutableList numbers = NonNlsMarkers.numbers(tokAfter.getText()); + for (int m = 0; m < literals.size(); m++) { + Integer literalOp = literals.get(m) < 0 + ? null + : tokenOps.get(tokenByIndex.get(literals.get(m))); + if (literalOp != null) { + String marker = NonNlsMarkers.marker(numbers.get(m)); + reservations.put(literalOp + 1, ReservedWidth.make(marker.length() + 1)); + } + } + tokOps.put(tokAfterPos, Comment.makeNonNlsMarkers(tokAfter, literals)); + tokOps.put(tokAfterPos, Break.makeForced()); + } else { + tokOps.putAll(tokAfterPos, makeComment(tokAfter)); + } if (breakAfter) { tokOps.put(tokAfterPos, Break.make(FillMode.FORCED, "", ZERO)); } @@ -630,6 +663,7 @@ public OpsOutput build() { ImmutableList.Builder newOps = ImmutableList.builder(); boolean afterForcedBreak = false; // Was the last Op a forced break? If so, suppress spaces. for (int i = 0; i < opsN; i++) { + newOps.addAll(reservations.get(i)); for (Op op : tokOps.get(i)) { if (!(afterForcedBreak && op instanceof NonBreakingSpace)) { newOps.add(op); @@ -662,6 +696,39 @@ private static boolean isNonNlsComment(Input.Tok tokAfter) { return tokAfter.isSlashSlashComment() && tokAfter.getText().contains("$NON-NLS"); } + /** + * For each marker of a {@code //$NON-NLS-1$ //$NON-NLS-2$} comment that trails {@code token}, the index of the + * string literal it marks, the n-th one on the comment's input line, or -1 when there is none. A literal in + * another partial-format region counts as none too: a partial format of its region would otherwise lose the + * marker, which belongs to the comment's region. So does a literal without an op of its own. + */ + private ImmutableList nonNlsLiterals( + Input.Token token, + Input.Tok comment, + Map tokenByIndex, + Map tokenOps) { + int line = input.getLineNumber(token.getTok().getPosition()) - 1; + Range toks = input.getRanges(line); + List literals = new ArrayList<>(); + for (int k = toks.lowerEndpoint(); k < Math.min(toks.upperEndpoint(), comment.getIndex()); k++) { + Input.Token candidate = tokenByIndex.get(k); + if (candidate != null && candidate.getTok().getText().startsWith("\"")) { + literals.add(k); + } + } + Range region = inputMetadataBuilder.regionContaining(comment.getIndex()); + ImmutableList.Builder result = ImmutableList.builder(); + for (int n : NonNlsMarkers.numbers(comment.getText())) { + int literal = 1 <= n && n <= literals.size() ? literals.get(n - 1) : -1; + boolean usable = literal >= 0 + && region != null + && region.contains(literal) + && tokenOps.containsKey(tokenByIndex.get(literal)); + result.add(usable ? literal : -1); + } + return result.build(); + } + private static boolean isForcedBreak(Op op) { return op instanceof Break b && b.isForced(); } diff --git a/open-java-format/src/main/java/com/palantir/javaformat/Output.java b/open-java-format/src/main/java/com/palantir/javaformat/Output.java index f4a7a12d1..d2d8e08e4 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/Output.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/Output.java @@ -15,6 +15,7 @@ package com.palantir.javaformat; import com.google.common.base.MoreObjects; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Range; import com.palantir.javaformat.doc.State; @@ -36,6 +37,14 @@ public abstract class Output extends InputOutput { */ public abstract void append(State state, String text, Range range); + /** + * Output a comment of {@code //$NON-NLS-n$} markers, see {@link com.palantir.javaformat.java.NonNlsMarkers}. + * + * @param literals for each marker, the index of the string literal it marks, or -1 + */ + public abstract void appendNonNlsMarkers( + State state, String comment, Range range, ImmutableList literals); + @Override public String toString() { return MoreObjects.toStringHelper(this).add("super", super.toString()).toString(); diff --git a/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java b/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java index 6386d595c..d8af05df6 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java @@ -17,6 +17,7 @@ package com.palantir.javaformat.doc; import com.google.common.base.MoreObjects; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterators; import com.google.common.collect.Range; import com.google.errorprone.annotations.Immutable; @@ -26,14 +27,19 @@ import com.palantir.javaformat.Op; import com.palantir.javaformat.Output; import com.palantir.javaformat.java.JavaCommentsHelper; +import com.palantir.javaformat.java.NonNlsMarkers; /** A leaf node in a {@link Doc} for a non-token. */ @Immutable public final class Comment extends Doc implements Op { private final Input.Tok tok; - private Comment(Input.Tok tok) { + /** For a comment of NON-NLS markers, the index of the string literal each marker marks, or -1; else empty. */ + private final ImmutableList nonNlsLiterals; + + private Comment(Input.Tok tok, ImmutableList nonNlsLiterals) { this.tok = tok; + this.nonNlsLiterals = nonNlsLiterals; } /** @@ -43,7 +49,18 @@ private Comment(Input.Tok tok) { * @return the new {@code Tok} */ public static Comment make(Input.Tok tok) { - return new Comment(tok); + return new Comment(tok, ImmutableList.of()); + } + + /** + * A comment of {@code //$NON-NLS-n$} markers, see {@link NonNlsMarkers}. A marker with a literal is written at + * the end of the literal's output line and takes its room there, as a {@link ReservedWidth} after the literal; + * only the markers without one take room here. + * + * @param literals for each marker, the index of the string literal it marks, or -1 + */ + public static Comment makeNonNlsMarkers(Input.Tok tok, ImmutableList literals) { + return new Comment(tok, literals); } @Override @@ -53,6 +70,17 @@ public void add(DocBuilder builder) { @Override protected float computeWidth() { + if (!nonNlsLiterals.isEmpty()) { + int width = 0; + ImmutableList numbers = NonNlsMarkers.numbers(tok.getOriginalText()); + for (int i = 0; i < numbers.size(); i++) { + if (nonNlsLiterals.get(i) < 0) { + width += (width > 0 ? 1 : 0) + + NonNlsMarkers.marker(numbers.get(i)).length(); + } + } + return width; + } int idx = Newlines.firstBreak(tok.getOriginalText()); // only count the first line of multi-line block comments if (tok.isComment()) { @@ -102,7 +130,11 @@ public State computeBreaks( @Override public void write(State state, Output output) { - output.append(state, state.getTokText(this), range()); + if (nonNlsLiterals.isEmpty()) { + output.append(state, state.getTokText(this), range()); + } else { + output.appendNonNlsMarkers(state, state.getTokText(this), range(), nonNlsLiterals); + } } @Override diff --git a/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java b/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java new file mode 100644 index 000000000..c96303a2d --- /dev/null +++ b/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java @@ -0,0 +1,76 @@ +/* + * (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.doc; + +import com.google.common.base.MoreObjects; +import com.google.common.collect.Range; +import com.google.errorprone.annotations.Immutable; +import com.palantir.javaformat.CommentsHelper; +import com.palantir.javaformat.Op; +import com.palantir.javaformat.Output; + +/** + * A leaf node in a {@link Doc} that takes room on its line without writing anything: the room for a + * {@code //$NON-NLS-n$} marker after the string literal it marks, which {@link Comment} writes at the end of the + * literal's line once the line breaks are known. So the layout already counts the marker on that line. + */ +@Immutable +public final class ReservedWidth extends Doc implements Op { + private final int width; + + private ReservedWidth(int width) { + this.width = width; + } + + public static ReservedWidth make(int width) { + return new ReservedWidth(width); + } + + @Override + public void add(DocBuilder builder) { + builder.add(this); + } + + @Override + protected float computeWidth() { + return width; + } + + @Override + protected String computeFlat() { + return ""; + } + + @Override + protected Range computeRange() { + return Doc.EMPTY_RANGE; + } + + @Override + public State computeBreaks( + CommentsHelper commentsHelper, int maxWidth, State state, Obs.ExplorationNode observationNode) { + return state.withColumn(state.column() + width); + } + + @Override + public void write(State state, Output output) {} + + @Override + public String toString() { + return MoreObjects.toStringHelper(this).add("width", width).toString(); + } +} diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/InputMetadataBuilder.java b/open-java-format/src/main/java/com/palantir/javaformat/java/InputMetadataBuilder.java index ef0669f3c..7bc9846d4 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/InputMetadataBuilder.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/InputMetadataBuilder.java @@ -24,6 +24,7 @@ import com.palantir.javaformat.OpsBuilder.BlankLineWanted; import java.util.HashMap; import java.util.Map; +import javax.annotation.Nullable; public final class InputMetadataBuilder { private final Map blankLines = new HashMap<>(); // Info on blank lines. @@ -50,6 +51,12 @@ public void markForPartialFormat(Input.Token start, Input.Token end) { partialFormatRanges.add(Range.closed(lo, hi)); } + /** The region marked for partial formatting that holds tok {@code k}, or null. */ + @Nullable + public Range regionContaining(int k) { + return partialFormatRanges.rangeContaining(k); + } + public InputMetadata build() { return new InputMetadata(ImmutableMap.copyOf(blankLines), ImmutableRangeSet.copyOf(partialFormatRanges)); } 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 cd0d70fdc..313635eee 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 @@ -207,8 +207,9 @@ private List wrapLineComments(List lines, int column0) { // remainder is emitted as bare code. line = CharMatcher.whitespace().trimLeadingFrom(line); line = addMissingSpace(line); - if (line.startsWith("// MOE:")) { - // don't wrap comments for https://github.com/google/MOE + if (line.startsWith("// MOE:") || NonNlsMarkers.isMarkersOnly(line)) { + // don't wrap comments for https://github.com/google/MOE, nor the markers Eclipse reads only on the + // line of their literal result.add(line); continue; } diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java b/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java index 36b015bc2..17a4c4f38 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java @@ -155,6 +155,67 @@ public void append(State state, String text, Range range) { } } + /** + * Writes each marker of a {@code //$NON-NLS-1$ //$NON-NLS-2$} comment, see {@link NonNlsMarkers}, to the output + * line of the literal it marks, numbered by the position of the literal among the string literals written on + * that line, so that every marker stays with its literal when the statement is wrapped (#23). A marker without a + * literal stays here. + */ + @Override + public void appendNonNlsMarkers( + State state, String comment, Range range, ImmutableList literals) { + ImmutableList numbers = NonNlsMarkers.numbers(comment); + int current = mutableLines.size(); + if (numbers.size() != literals.size() || range.isEmpty() || newlinesPending > 0 || ranges.size() <= current) { + // Not a comment as paired with its literals, or not trailing the code of the current line. + append(state, comment, range); + return; + } + List remaining = new ArrayList<>(); + for (int i = 0; i < numbers.size(); i++) { + int literal = literals.get(i); + int line = literal < 0 ? -1 : outputLineOf(literal, current); + if (line < 0) { + remaining.add(NonNlsMarkers.marker(numbers.get(i))); + } else if (line == current) { + remaining.add(NonNlsMarkers.marker(stringLiteralsOnOutputLine(line, literal))); + } else { + String marker = NonNlsMarkers.marker(stringLiteralsOnOutputLine(line, literal)); + mutableLines.set(line, mutableLines.get(line) + " " + marker); + } + } + if (remaining.isEmpty()) { + // Every marker went to an earlier line. The comment still counts to this line, where it was written, so + // that a partial format of the statement covers it. + ranges.set(current, union(ranges.get(current), range)); + lastK = range.upperEndpoint(); + } else { + append(state, String.join(" ", remaining), range); + } + } + + /** The output line, searched back from {@code current}, that holds tok {@code k}, or -1 if it is not written. */ + private int outputLineOf(int k, int current) { + for (int line = current; line >= 0; line--) { + if (ranges.get(line).contains(k)) { + return line; + } + } + return -1; + } + + /** The position of the string literal {@code k} among the string literals written on an output line. */ + private int stringLiteralsOnOutputLine(int line, int k) { + int count = 0; + for (int i = ranges.get(line).lowerEndpoint(); i <= k; i++) { + Input.Tok tok = javaInput.getToken(i).getTok(); + if (tok.getIndex() == i && tok.getText().startsWith("\"")) { + count++; + } + } + return count; + } + @Override public void indent(int indent) { spacesPending.append(" ".repeat(indent)); diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java b/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java new file mode 100644 index 000000000..9cd5fbe21 --- /dev/null +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java @@ -0,0 +1,58 @@ +/* + * (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 com.google.common.collect.ImmutableList; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * The markers with which Eclipse exempts a string literal from its warning about non-externalized strings: a trailing + * line comment {@code //$NON-NLS-n$}, where n is the position of the literal among the string literals of its line, + * one marker per exempted literal, as in {@code //$NON-NLS-1$ //$NON-NLS-2$}. Eclipse reads a marker only with this + * exact spelling and only on the line of its literal, so the formatter leaves the spelling alone, never wraps a comment + * of markers, and when it moves a literal to another line it moves the marker along and renumbers it (#23): see + * {@link com.palantir.javaformat.OpsBuilder#build()}, which pairs each marker with its literal, and + * {@link JavaOutput#appendNonNlsMarkers}, which writes it on the literal's line. + */ +public final class NonNlsMarkers { + private static final Pattern MARKER = Pattern.compile("//\\$NON-NLS-(\\d{1,9})\\$"); + + /** A line comment that holds markers and nothing else. */ + private static final Pattern MARKERS_ONLY = + Pattern.compile("//\\$NON-NLS-\\d{1,9}\\$([ \\t]+//\\$NON-NLS-\\d{1,9}\\$)*"); + + public static boolean isMarkersOnly(String comment) { + return comment.startsWith("//$NON-NLS-") && MARKERS_ONLY.matcher(comment).matches(); + } + + /** The numbers of the markers in a comment, in order. */ + public static ImmutableList numbers(String comment) { + ImmutableList.Builder numbers = ImmutableList.builder(); + Matcher matcher = MARKER.matcher(comment); + while (matcher.find()) { + numbers.add(Integer.parseInt(matcher.group(1))); + } + return numbers.build(); + } + + public static String marker(int n) { + return "//$NON-NLS-" + n + "$"; + } + + private NonNlsMarkers() {} +} diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java b/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java index 28c954f19..67f730d4c 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java @@ -43,6 +43,7 @@ import com.sun.tools.javac.util.Position.LineMap; import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Comparator; import java.util.Deque; import java.util.HashMap; import java.util.Iterator; @@ -50,6 +51,7 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Collectors; +import javax.annotation.Nullable; /** Wraps string literals that exceed the column limit. */ public final class StringWrapper { @@ -165,10 +167,13 @@ protected TreeRangeMap getReflowReplacements() { List longStringLiterals = new ArrayList<>(); // Paths to text blocks to be re-indented. List textBlocks = new ArrayList<>(); - new LongStringsAndTextBlockScanner(longStringLiterals, textBlocks).scan(new TreePath(unit), null); + // Every string literal, for the numbering of the NON-NLS markers of a line. + List stringLiterals = new ArrayList<>(); + new LongStringsAndTextBlockScanner(longStringLiterals, textBlocks, stringLiterals) + .scan(new TreePath(unit), null); TreeRangeMap replacements = TreeRangeMap.create(); indentTextBlocks(replacements, textBlocks); - wrapLongStrings(replacements, longStringLiterals); + wrapLongStrings(replacements, longStringLiterals, stringLiterals); return replacements; } @@ -176,10 +181,13 @@ private class LongStringsAndTextBlockScanner extends TreePathScanner private final List longStringLiterals; private final List textBlocks; + private final List stringLiterals; - LongStringsAndTextBlockScanner(List longStringLiterals, List textBlocks) { + LongStringsAndTextBlockScanner( + List longStringLiterals, List textBlocks, List stringLiterals) { this.longStringLiterals = longStringLiterals; this.textBlocks = textBlocks; + this.stringLiterals = stringLiterals; } @Override @@ -187,6 +195,7 @@ public Void visitLiteral(LiteralTree literalTree, Void aVoid) { if (literalTree.getKind() != Kind.STRING_LITERAL) { return null; } + stringLiterals.add(literalTree); int pos = getStartPosition(literalTree); if (input.substring(pos, Math.min(input.length(), pos + TEXT_BLOCK_DELIMITER.length())) .equals(TEXT_BLOCK_DELIMITER)) { @@ -351,8 +360,26 @@ private String computePrefixIndentation( return " ".repeat(extraIndent + startParentColumn); } + /** + * A long literal with its neighbours in a chain of {@code +}, reflowed as one text; see {@link #flatten}. + * {@code plain} is its text reflowed without regard to markers, in pieces, one per line. + */ + private record Chain( + List flat, + boolean firstInChain, + int start, + int end, + int lineEnd, + int startColumn, + int firstLineColumn, + List plain) {} + @SuppressWarnings("for-rollout:NullAway") - private void wrapLongStrings(TreeRangeMap replacements, List longStringLiterals) { + private void wrapLongStrings( + TreeRangeMap replacements, + List longStringLiterals, + List stringLiterals) { + List chains = new ArrayList<>(); for (TreePath path : longStringLiterals) { // Find the outermost contiguous enclosing concatenation expression TreePath enclosing = path; @@ -391,10 +418,181 @@ private void wrapLongStrings(TreeRangeMap replacements, List components = stringComponents(input, unit, flat); + List plain = + reflow(columnLimit, trailing, components, first.get(), startColumn, fistLineCol, "", ""); + chains.add(new Chain( + flat, + first.get(), + getStartPosition(flat.get(0)), + end, + lineEnd, + startColumn, + fistLineCol, + plain)); + } + for (Chain chain : chains) { + String joined = join(chain, chain.plain(), "", ""); + if (joined.equals(input.substring(chain.start(), chain.end()))) { + // Every literal on a line past the limit is a candidate; this one does not change. + continue; + } + LineMarkers markers = lineMarkers(chains, chain, stringLiterals); + if (markers == null) { + replacements.put(Range.closedOpen(chain.start(), chain.end()), joined); + continue; + } + ImmutableList components = stringComponents(input, unit, chain.flat()); + List pieces = reflow( + columnLimit, + markers.tail(2).length(), + components, + chain.firstInChain(), + chain.startColumn(), + chain.firstLineColumn(), + markers.firstLine(), + markers.otherLines()); replacements.put( - Range.closedOpen(getStartPosition(flat.get(0)), getEndPosition(unit, getLast(flat))), - reflow(separator, columnLimit, trailing, components, first.get(), startColumn, fistLineCol)); + Range.closedOpen(chain.start(), chain.lineEnd()), + join(chain, pieces, markers.firstLine(), markers.otherLines()) + markers.tail(pieces.size())); + } + } + + private String join(Chain chain, List pieces, String firstLineMarkers, String otherLineMarkers) { + return render( + separator, + pieces, + chain.firstInChain() ? chain.firstLineColumn() + 4 : chain.startColumn() - 2, + firstLineMarkers, + otherLineMarkers); + } + + /** + * The NON-NLS markers of the line a chain ends on, see {@link NonNlsMarkers}, sorted by where they go once the + * chain is reflowed: a marker of a literal before the chain keeps its number on the first line; the chain, if + * marked, gets a marker after every piece, numbered by the piece's line; a marker of a literal after the chain + * is counted from the last piece on the last line; and a marker without a literal stays at the end. Null when + * the markers are left alone: when the comment holds anything else, or when another chain of the line + * changes too, since the chain rewrites the rest of its line. + */ + @Nullable + private LineMarkers lineMarkers(List chains, Chain chain, List stringLiterals) { + String tail = input.substring(chain.end(), chain.lineEnd()); + int comment = lineCommentStart(tail); + if (comment < 0 + || !NonNlsMarkers.isMarkersOnly(tail.substring(comment).stripTrailing())) { + return null; + } + long lastLine = lineMap.getLineNumber(chain.end() - 1); + for (Chain other : chains) { + // The same chain is found once per long literal in it. + if (other.start() != chain.start() + && lineMap.getLineNumber(other.end() - 1) == lastLine + && !join(other, other.plain(), "", "").equals(input.substring(other.start(), other.end()))) { + return null; + } + } + List literals = literalsOnLine(stringLiterals, lastLine); + int lastOfChain = literals.indexOf(getLast(chain.flat())); + if (lastOfChain < 0) { + // A text block, which starts on another line. + return null; + } + int firstOfChain = lastOfChain; + while (firstOfChain > 0 && chain.flat().contains(literals.get(firstOfChain - 1))) { + firstOfChain--; + } + List before = new ArrayList<>(); + List after = new ArrayList<>(); + List unmatched = new ArrayList<>(); + boolean chainMarked = false; + for (int n : NonNlsMarkers.numbers(tail.substring(comment))) { + int i = n - 1; + if (i < 0 || i >= literals.size()) { + unmatched.add(NonNlsMarkers.marker(n)); + } else if (i < firstOfChain) { + before.add(NonNlsMarkers.marker(n)); + } else if (i <= lastOfChain) { + chainMarked = true; + } else { + after.add(i - lastOfChain); + } } + long beforeOnFirstLine = literalsOnLine(stringLiterals, lineMap.getLineNumber(chain.start())).stream() + .filter(literal -> getStartPosition(literal) < chain.start()) + .count(); + return new LineMarkers( + ImmutableList.copyOf(before), + (int) beforeOnFirstLine, + chainMarked, + ImmutableList.copyOf(after), + ImmutableList.copyOf(unmatched), + tail.substring(0, comment)); + } + + /** See {@link #lineMarkers}; {@code code} is the text between the chain and its line's comment. */ + private record LineMarkers( + ImmutableList before, + int beforeOnFirstLine, + boolean chainMarked, + ImmutableList after, + ImmutableList unmatched, + String code) { + + /** The markers after the first piece, when it ends its line. */ + String firstLine() { + List markers = new ArrayList<>(before); + if (chainMarked) { + markers.add(NonNlsMarkers.marker(beforeOnFirstLine + 1)); + } + return markers.isEmpty() ? "" : " " + String.join(" ", markers); + } + + /** The markers after a piece in the middle. */ + String otherLines() { + return chainMarked ? " " + NonNlsMarkers.marker(1) : ""; + } + + /** The text after the last of {@code pieces}: the code, then the markers of that line. */ + String tail(int pieces) { + List markers = new ArrayList<>(); + int chainPosition = pieces > 1 ? 1 : beforeOnFirstLine + 1; + if (pieces == 1) { + markers.addAll(before); + } + if (chainMarked) { + markers.add(NonNlsMarkers.marker(chainPosition)); + } + for (int position : after) { + markers.add(NonNlsMarkers.marker(chainPosition + position)); + } + markers.addAll(unmatched); + return markers.isEmpty() ? code.stripTrailing() : code + String.join(" ", markers); + } + } + + /** The string literals that start on a line, in order. */ + private List literalsOnLine(List stringLiterals, long line) { + return stringLiterals.stream() + .filter(literal -> lineMap.getLineNumber(getStartPosition(literal)) == line) + .sorted(Comparator.comparingInt(StringWrapper::getStartPosition)) + .toList(); + } + + /** The index of the line comment in a line of code, the first {@code //} outside a literal, or -1. */ + private static int lineCommentStart(String line) { + for (int i = 0; i < line.length(); i++) { + char c = line.charAt(i); + if (c == '"' || c == '\'') { + for (i++; i < line.length() && line.charAt(i) != c; i++) { + if (line.charAt(i) == '\\') { + i++; + } + } + } else if (c == '/' && line.startsWith("//", i)) { + return i; + } + } + return -1; } private static boolean onSameLineAsParent(LineMap lineMap, TreePath path) { @@ -479,9 +677,9 @@ private static boolean isEscaped(String input, int idx) { } /** - * Reflows the given source text, trying to split on word boundaries. + * Reflows the given source text, trying to split on word boundaries. Returns the pieces, one per line; see + * {@link #render}. * - * @param separator the line separator * @param columnLimit the number of columns to wrap at * @param trailing extra space to leave after the last line * @param components the text to reflow @@ -489,18 +687,22 @@ private static boolean isEscaped(String input, int idx) { * @param textStartColumn the column position of the beginning of the original text * @param firstLineStartColumn the column where the very first line starts (can be less than textStartColumn if text * follows variable declaration) + * @param firstLineMarkers the NON-NLS markers that trail the first line, with their leading blank, or empty + * @param otherLineMarkers the NON-NLS markers that trail every further line but the last, whose markers are part + * of {@code trailing} */ - private static String reflow( - String separator, + private static List reflow( int columnLimit, int trailing, ImmutableList components, boolean first0, int textStartColumn, - int firstLineStartColumn) { + int firstLineStartColumn, + String firstLineMarkers, + String otherLineMarkers) { // We have space between the start column and the limit to output the first line. - // Reserve two spaces for the quotes. - int width = columnLimit - textStartColumn - 2; + // Reserve two spaces for the quotes, and room for the markers of the line. + int width = columnLimit - textStartColumn - 2 - firstLineMarkers.length(); Deque input = new ArrayDeque<>(components); List lines = new ArrayList<>(); boolean first = first0; @@ -508,7 +710,8 @@ private static String reflow( int length = 0; List line = new ArrayList<>(); if (input.stream().mapToInt(String::length).sum() <= width) { - width -= trailing; + // The last line, probably: its markers are in the trailing text, not reserved on their own. + width -= trailing - (lines.isEmpty() ? 0 : otherLineMarkers.length()); } while (!input.isEmpty() && (length <= 4 || (length + input.peekFirst().length()) < width)) { @@ -535,13 +738,28 @@ private static String reflow( width += textStartColumn - firstLineStartColumn; first = false; } + if (lines.size() == 1) { + width += firstLineMarkers.length() - otherLineMarkers.length(); + } } + return lines; + } - return lines.stream() - .collect(joining( - "\"" + separator + " ".repeat(first0 ? firstLineStartColumn + 4 : textStartColumn - 2) + "+ \"", - "\"", - "\"")); + /** Renders reflowed pieces as literals in a chain of {@code +}, one per line, indented by {@code column}. */ + private static String render( + String separator, List pieces, int column, String firstLineMarkers, String otherLineMarkers) { + String continuation = separator + " ".repeat(column) + "+ "; + StringBuilder result = new StringBuilder(); + for (int i = 0; i < pieces.size(); i++) { + if (i > 0) { + result.append(continuation); + } + result.append('"').append(pieces.get(i)).append('"'); + if (i < pieces.size() - 1) { + result.append(i == 0 ? firstLineMarkers : otherLineMarkers); + } + } + return result.toString(); } /** 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 15539cc56..8cd168626 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 @@ -433,6 +433,45 @@ void m() { assertThat(Formatter.create().formatSource(once)).isEqualTo(once); } + @Test + void nonNlsMarkersStayOnTheLineOfTheirLiteral() throws Exception { + // Eclipse numbers a marker by the position of its literal on the line. When a statement is wrapped, each + // marker moves to the line of its literal and is renumbered for that line; when a literal is reflowed, every + // piece gets one (#23). A marker whose number has no literal stays where it is. + String input = """ + class T { + void m() { + String s = Messages.format("first argument that is long enough to matter", "second argument that is also rather long", "third one"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + String reflowed = "a single string literal that is far too long to fit into one hundred and twenty columns, so the formatter reflows it"; //$NON-NLS-1$ + String n = Messages.format("first argument that is long enough to matter", "second argument that is also rather long", "third one"); //$NON-NLS-5$ + } + } + """; + String expected = """ + class T { + void m() { + String s = Messages.format( + "first argument that is long enough to matter", //$NON-NLS-1$ + "second argument that is also rather long", //$NON-NLS-1$ + "third one"); //$NON-NLS-1$ + String reflowed = + "a single string literal that is far too long to fit into one hundred and twenty" //$NON-NLS-1$ + + " columns, so the formatter reflows it"; //$NON-NLS-1$ + String n = Messages.format( + "first argument that is long enough to matter", + "second argument that is also rather long", + "third one"); //$NON-NLS-5$ + } + } + """; + + Formatter formatter = Formatter.createFormatter( + JavaFormatterOptions.builder().style(Style.OJF).build()); + String once = formatter.formatSourceAndFixImports(input); + assertThat(once).isEqualTo(expected); + assertThat(formatter.formatSourceAndFixImports(once)).isEqualTo(once); + } + @Test void throwsFormatterException() throws Exception { assertThatThrownBy(() -> Formatter.create().formatSourceAndFixImports("package foo; public class {")) 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 index f3d15f6cf..c8e11b697 100644 --- 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 @@ -111,4 +111,14 @@ void anOrdinaryLineCommentStillWraps() throws Exception { assertThat(rewritten.lines()).hasSize(2); assertThat(rewritten.lines()).allSatisfy(line -> assertThat(line.trim()).startsWith("// ")); } + + @Test + void aRunOfNonNlsMarkersIsNeverWrapped() throws Exception { + // Eclipse reads a marker only on the line of its literal, so a run that reaches past the limit stays as it is + // instead of being wrapped at a blank into a `// //$NON-NLS-3$` line of its own. + String run = "//$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$" + + " //$NON-NLS-8$ //$NON-NLS-9$"; + + assertThat(rewrite(run, Style.OJF)).isEqualTo(run); + } } diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/StringWrapperIntegrationTest/ojf-issue-23-marked-literals.input b/open-java-format/src/test/resources/com/palantir/javaformat/java/StringWrapperIntegrationTest/ojf-issue-23-marked-literals.input new file mode 100644 index 000000000..a8d077bea --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/StringWrapperIntegrationTest/ojf-issue-23-marked-literals.input @@ -0,0 +1,7 @@ +class T { + void f() { + String s = "one long incredibly unbroken sentence moving from topic to topic"; //$NON-NLS-1$ + foo("x", "a long unbroken sentence", "y"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + String t = "short" + " and one long sentence"; //$NON-NLS-1$ //$NON-NLS-2$ + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/StringWrapperIntegrationTest/ojf-issue-23-marked-literals.output b/open-java-format/src/test/resources/com/palantir/javaformat/java/StringWrapperIntegrationTest/ojf-issue-23-marked-literals.output new file mode 100644 index 000000000..8a1a7a10e --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/StringWrapperIntegrationTest/ojf-issue-23-marked-literals.output @@ -0,0 +1,20 @@ +class T { + void f() { + String s = "one long" //$NON-NLS-1$ + + " incredibly" //$NON-NLS-1$ + + " unbroken" //$NON-NLS-1$ + + " sentence" //$NON-NLS-1$ + + " moving from" //$NON-NLS-1$ + + " topic to" //$NON-NLS-1$ + + " topic"; //$NON-NLS-1$ + foo( + "x", //$NON-NLS-1$ + "a long" //$NON-NLS-1$ + + " unbroken" //$NON-NLS-1$ + + " sentence", //$NON-NLS-1$ + "y"); //$NON-NLS-1$ + String t = "short" //$NON-NLS-1$ + + " and one long" //$NON-NLS-1$ + + " sentence"; //$NON-NLS-1$ + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-reflowed-literal.input b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-reflowed-literal.input new file mode 100644 index 000000000..15c3eb9ce --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-reflowed-literal.input @@ -0,0 +1,9 @@ +class ReflowedLiteral { + void markers() { + String reflowed = "a single string literal that is far too long to fit into one hundred and twenty columns, so the formatter reflows it"; //$NON-NLS-1$ + String unmarked = "a single string literal that is far too long to fit into one hundred and twenty columns, so the formatter reflows it"; + log( "short", "a string literal as the second argument that is far too long to fit into one hundred and twenty columns", "after" ); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + log(KEY, "a string literal as the second argument that is far too long to fit into one hundred and twenty columns, so it reflows", VALUE); //$NON-NLS-1$ + String chain = "a string literal that is far too long to fit into one hundred and twenty columns, so it reflows" + "with another literal"; //$NON-NLS-1$ //$NON-NLS-2$ + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-reflowed-literal.output b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-reflowed-literal.output new file mode 100644 index 000000000..fdc8aaefa --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-reflowed-literal.output @@ -0,0 +1,24 @@ +class ReflowedLiteral { + void markers() { + String reflowed = + "a single string literal that is far too long to fit into one hundred and twenty" //$NON-NLS-1$ + + " columns, so the formatter reflows it"; //$NON-NLS-1$ + String unmarked = + "a single string literal that is far too long to fit into one hundred and twenty columns, so the" + + " formatter reflows it"; + log( + "short", //$NON-NLS-1$ + "a string literal as the second argument that is far too long to fit into one hundred" //$NON-NLS-1$ + + " and twenty columns", //$NON-NLS-1$ + "after"); //$NON-NLS-1$ + log( + KEY, + "a string literal as the second argument that is far too long to fit into one hundred" //$NON-NLS-1$ + + " and twenty columns, so it reflows", //$NON-NLS-1$ + VALUE); + String chain = + "a string literal that is far too long to fit into one hundred and twenty columns, so it" //$NON-NLS-1$ + + " reflows" //$NON-NLS-1$ + + "with another literal"; //$NON-NLS-1$ + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-wrapped-arguments.input b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-wrapped-arguments.input new file mode 100644 index 000000000..e13bb3e71 --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-wrapped-arguments.input @@ -0,0 +1,11 @@ +class WrappedArguments { + void markers() { + String message = Messages.format("first argument that is long enough to matter", "second argument that is also rather long", "third one"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + String second = Messages.format( "first argument that is long enough to matter", "second argument that is also rather long", "third one" ); //$NON-NLS-2$ + String twoOnALine = Messages.format("first argument that is long enough to matter", "a" + "b", "third argument that is also rather long"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ + String keyAndValue = Messages.format(KEY, "an argument that is long enough to matter", "second argument that is also rather long", VALUE); //$NON-NLS-1$ //$NON-NLS-2$ + String chained = builder.append("first argument that is long enough to matter").append("second argument that is also rather long").toString(); //$NON-NLS-1$ //$NON-NLS-2$ + String noSuchLiteral = Messages.format("first argument that is long enough to matter", "second argument that is also rather long", "third one"); //$NON-NLS-1$ //$NON-NLS-5$ + String fitsOnTheNextLine = Messages.format("first argument", "second one", "third"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-wrapped-arguments.output b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-wrapped-arguments.output new file mode 100644 index 000000000..b9d2836f9 --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ojf-issue-23-wrapped-arguments.output @@ -0,0 +1,30 @@ +class WrappedArguments { + void markers() { + String message = Messages.format( + "first argument that is long enough to matter", //$NON-NLS-1$ + "second argument that is also rather long", //$NON-NLS-1$ + "third one"); //$NON-NLS-1$ + String second = Messages.format( + "first argument that is long enough to matter", + "second argument that is also rather long", //$NON-NLS-1$ + "third one"); + String twoOnALine = Messages.format( + "first argument that is long enough to matter", //$NON-NLS-1$ + "a" + "b", //$NON-NLS-1$ //$NON-NLS-2$ + "third argument that is also rather long"); //$NON-NLS-1$ + String keyAndValue = Messages.format( + KEY, + "an argument that is long enough to matter", //$NON-NLS-1$ + "second argument that is also rather long", //$NON-NLS-1$ + VALUE); + String chained = builder.append("first argument that is long enough to matter") //$NON-NLS-1$ + .append("second argument that is also rather long") //$NON-NLS-1$ + .toString(); + String noSuchLiteral = Messages.format( + "first argument that is long enough to matter", //$NON-NLS-1$ + "second argument that is also rather long", + "third one"); //$NON-NLS-5$ + String fitsOnTheNextLine = + Messages.format("first argument", "second one", "third"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + } +} From eed63c3ca847c843849b44db40cb4b9ae9fd8f10 Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Sun, 25 Oct 2026 17:55:21 +0300 Subject: [PATCH 3/9] Hand the NON-NLS marker lists around as plain List The marker code used Guava's ImmutableList for its lists of marker numbers and literal indices. List.of, List.copyOf and Stream.toList give unmodifiable lists without it, so the signatures take and return List. The field in Comment carries the Immutable suppression the other doc nodes use for their effectively immutable state. --- .../com/palantir/javaformat/OpsBuilder.java | 11 +++++------ .../java/com/palantir/javaformat/Output.java | 5 ++--- .../com/palantir/javaformat/doc/Comment.java | 15 ++++++++------- .../palantir/javaformat/java/JavaOutput.java | 5 ++--- .../palantir/javaformat/java/NonNlsMarkers.java | 17 +++++++---------- .../palantir/javaformat/java/StringWrapper.java | 12 ++++++------ 6 files changed, 30 insertions(+), 35 deletions(-) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java b/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java index f36e436c1..14df20693 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java @@ -608,9 +608,8 @@ public OpsOutput build() { tokenByIndex.put(t.getTok().getIndex(), t); } } - ImmutableList literals = - nonNlsLiterals(token, tokAfter, tokenByIndex, tokenOps); - ImmutableList numbers = NonNlsMarkers.numbers(tokAfter.getText()); + List literals = nonNlsLiterals(token, tokAfter, tokenByIndex, tokenOps); + List numbers = NonNlsMarkers.numbers(tokAfter.getText()); for (int m = 0; m < literals.size(); m++) { Integer literalOp = literals.get(m) < 0 ? null @@ -702,7 +701,7 @@ private static boolean isNonNlsComment(Input.Tok tokAfter) { * another partial-format region counts as none too: a partial format of its region would otherwise lose the * marker, which belongs to the comment's region. So does a literal without an op of its own. */ - private ImmutableList nonNlsLiterals( + private List nonNlsLiterals( Input.Token token, Input.Tok comment, Map tokenByIndex, @@ -717,7 +716,7 @@ private ImmutableList nonNlsLiterals( } } Range region = inputMetadataBuilder.regionContaining(comment.getIndex()); - ImmutableList.Builder result = ImmutableList.builder(); + List result = new ArrayList<>(); for (int n : NonNlsMarkers.numbers(comment.getText())) { int literal = 1 <= n && n <= literals.size() ? literals.get(n - 1) : -1; boolean usable = literal >= 0 @@ -726,7 +725,7 @@ private ImmutableList nonNlsLiterals( && tokenOps.containsKey(tokenByIndex.get(literal)); result.add(usable ? literal : -1); } - return result.build(); + return List.copyOf(result); } private static boolean isForcedBreak(Op op) { diff --git a/open-java-format/src/main/java/com/palantir/javaformat/Output.java b/open-java-format/src/main/java/com/palantir/javaformat/Output.java index d2d8e08e4..f6689a4b2 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/Output.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/Output.java @@ -15,9 +15,9 @@ package com.palantir.javaformat; import com.google.common.base.MoreObjects; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Range; import com.palantir.javaformat.doc.State; +import java.util.List; /** An output from the formatter. */ public abstract class Output extends InputOutput { @@ -42,8 +42,7 @@ public abstract class Output extends InputOutput { * * @param literals for each marker, the index of the string literal it marks, or -1 */ - public abstract void appendNonNlsMarkers( - State state, String comment, Range range, ImmutableList literals); + public abstract void appendNonNlsMarkers(State state, String comment, Range range, List literals); @Override public String toString() { diff --git a/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java b/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java index d8af05df6..54b2a2d0d 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java @@ -17,7 +17,6 @@ package com.palantir.javaformat.doc; import com.google.common.base.MoreObjects; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterators; import com.google.common.collect.Range; import com.google.errorprone.annotations.Immutable; @@ -28,6 +27,7 @@ import com.palantir.javaformat.Output; import com.palantir.javaformat.java.JavaCommentsHelper; import com.palantir.javaformat.java.NonNlsMarkers; +import java.util.List; /** A leaf node in a {@link Doc} for a non-token. */ @Immutable @@ -35,9 +35,10 @@ public final class Comment extends Doc implements Op { private final Input.Tok tok; /** For a comment of NON-NLS markers, the index of the string literal each marker marks, or -1; else empty. */ - private final ImmutableList nonNlsLiterals; + @SuppressWarnings("Immutable") // List.of and List.copyOf: unmodifiable + private final List nonNlsLiterals; - private Comment(Input.Tok tok, ImmutableList nonNlsLiterals) { + private Comment(Input.Tok tok, List nonNlsLiterals) { this.tok = tok; this.nonNlsLiterals = nonNlsLiterals; } @@ -49,7 +50,7 @@ private Comment(Input.Tok tok, ImmutableList nonNlsLiterals) { * @return the new {@code Tok} */ public static Comment make(Input.Tok tok) { - return new Comment(tok, ImmutableList.of()); + return new Comment(tok, List.of()); } /** @@ -59,8 +60,8 @@ public static Comment make(Input.Tok tok) { * * @param literals for each marker, the index of the string literal it marks, or -1 */ - public static Comment makeNonNlsMarkers(Input.Tok tok, ImmutableList literals) { - return new Comment(tok, literals); + public static Comment makeNonNlsMarkers(Input.Tok tok, List literals) { + return new Comment(tok, List.copyOf(literals)); } @Override @@ -72,7 +73,7 @@ public void add(DocBuilder builder) { protected float computeWidth() { if (!nonNlsLiterals.isEmpty()) { int width = 0; - ImmutableList numbers = NonNlsMarkers.numbers(tok.getOriginalText()); + List numbers = NonNlsMarkers.numbers(tok.getOriginalText()); for (int i = 0; i < numbers.size(); i++) { if (nonNlsLiterals.get(i) < 0) { width += (width > 0 ? 1 : 0) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java b/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java index 17a4c4f38..2a5b74ab4 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java @@ -162,9 +162,8 @@ public void append(State state, String text, Range range) { * literal stays here. */ @Override - public void appendNonNlsMarkers( - State state, String comment, Range range, ImmutableList literals) { - ImmutableList numbers = NonNlsMarkers.numbers(comment); + public void appendNonNlsMarkers(State state, String comment, Range range, List literals) { + List numbers = NonNlsMarkers.numbers(comment); int current = mutableLines.size(); if (numbers.size() != literals.size() || range.isEmpty() || newlinesPending > 0 || ranges.size() <= current) { // Not a comment as paired with its literals, or not trailing the code of the current line. diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java b/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java index 9cd5fbe21..c6cd1a115 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java @@ -16,8 +16,7 @@ package com.palantir.javaformat.java; -import com.google.common.collect.ImmutableList; -import java.util.regex.Matcher; +import java.util.List; import java.util.regex.Pattern; /** @@ -40,14 +39,12 @@ public static boolean isMarkersOnly(String comment) { return comment.startsWith("//$NON-NLS-") && MARKERS_ONLY.matcher(comment).matches(); } - /** The numbers of the markers in a comment, in order. */ - public static ImmutableList numbers(String comment) { - ImmutableList.Builder numbers = ImmutableList.builder(); - Matcher matcher = MARKER.matcher(comment); - while (matcher.find()) { - numbers.add(Integer.parseInt(matcher.group(1))); - } - return numbers.build(); + /** The numbers of the markers in a comment, in order; an unmodifiable list. */ + public static List numbers(String comment) { + return MARKER.matcher(comment) + .results() + .map(result -> Integer.parseInt(result.group(1))) + .toList(); } public static String marker(int n) { diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java b/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java index 67f730d4c..32831222e 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java @@ -521,21 +521,21 @@ private LineMarkers lineMarkers(List chains, Chain chain, List stri .filter(literal -> getStartPosition(literal) < chain.start()) .count(); return new LineMarkers( - ImmutableList.copyOf(before), + List.copyOf(before), (int) beforeOnFirstLine, chainMarked, - ImmutableList.copyOf(after), - ImmutableList.copyOf(unmatched), + List.copyOf(after), + List.copyOf(unmatched), tail.substring(0, comment)); } /** See {@link #lineMarkers}; {@code code} is the text between the chain and its line's comment. */ private record LineMarkers( - ImmutableList before, + List before, int beforeOnFirstLine, boolean chainMarked, - ImmutableList after, - ImmutableList unmatched, + List after, + List unmatched, String code) { /** The markers after the first piece, when it ends its line. */ From 7bfdbdb00eb739ca907f4fdd4375df865d1a3521 Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Sun, 25 Oct 2026 17:55:22 +0300 Subject: [PATCH 4/9] Cover NonNlsMarkers, ReservedWidth and addMissingSpace with unit tests The fixtures exercise the marker code end to end only. NonNlsMarkersTest pins which comments count as markers only, the order and the places in which the numbers are read, and the spelling of a marker; ReservedWidthTest pins that a reservation takes its width on the line and nothing else; the two JavaCommentsHelperTest cases pin the missing-space rule and its exemptions. --- .../javaformat/doc/ReservedWidthTest.java | 47 +++++++++++++++ .../java/JavaCommentsHelperTest.java | 25 ++++++++ .../javaformat/java/NonNlsMarkersTest.java | 60 +++++++++++++++++++ 3 files changed, 132 insertions(+) create mode 100644 open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java create mode 100644 open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java diff --git a/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java b/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java new file mode 100644 index 000000000..cfddbaa48 --- /dev/null +++ b/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java @@ -0,0 +1,47 @@ +/* + * (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.doc; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.palantir.javaformat.CommentsHelper; +import org.junit.jupiter.api.Test; + +/** A {@link ReservedWidth} takes the room of a NON-NLS marker that is written to its line later, and nothing else. */ +final class ReservedWidthTest { + + private static final int WIDTH = " //$NON-NLS-1$".length(); + + @Test + void takesItsWidthOnTheLineButHasNoText() { + ReservedWidth reserved = ReservedWidth.make(WIDTH); + + assertThat(reserved.getWidth()).isEqualTo((float) WIDTH); + assertThat(reserved.getFlat()).isEmpty(); + assertThat(reserved.range().isEmpty()).isTrue(); + } + + @Test + void movesTheColumnByItsWidthWhenTheBreaksAreComputed() { + State before = State.startingState().withColumn(10); + CommentsHelper noRewrite = (tok, maxWidth, column0) -> tok.getOriginalText(); + + State after = ReservedWidth.make(WIDTH).computeBreaks(noRewrite, 120, before, Obs.createRoot(new NoopSink())); + + assertThat(after.column()).isEqualTo(10 + WIDTH); + } +} 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 index c8e11b697..9c14ec878 100644 --- 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 @@ -19,6 +19,7 @@ import static org.assertj.core.api.Assertions.assertThat; import com.palantir.javaformat.java.JavaFormatterOptions.Style; +import java.util.List; import org.junit.jupiter.api.Test; /** @@ -121,4 +122,28 @@ void aRunOfNonNlsMarkersIsNeverWrapped() throws Exception { assertThat(rewrite(run, Style.OJF)).isEqualTo(run); } + + @Test + void addMissingSpacePutsABlankAfterTheSlashesWhateverTheirNumber() { + assertThat(JavaCommentsHelper.addMissingSpace("//foo")).isEqualTo("// foo"); + assertThat(JavaCommentsHelper.addMissingSpace("///foo")).isEqualTo("/// foo"); + assertThat(JavaCommentsHelper.addMissingSpace("//-- foo")).isEqualTo("// -- foo"); + } + + @Test + void addMissingSpaceLeavesSpacedCommentsAndIdeMarkersAlone() { + // A blank or a tab after the slashes is a space already; the IDE markers are recognised only without one. + List comments = List.of( + "// foo", + "// two spaces", + "//\tfoo", + "//", + "///", + "//$NON-NLS-1$ //$NON-NLS-2$", + "//noinspection unchecked"); + + for (String comment : comments) { + assertThat(JavaCommentsHelper.addMissingSpace(comment)).as(comment).isEqualTo(comment); + } + } } diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java new file mode 100644 index 000000000..a22195ee9 --- /dev/null +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java @@ -0,0 +1,60 @@ +/* + * (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 java.util.List; +import org.junit.jupiter.api.Test; + +final class NonNlsMarkersTest { + + @Test + void aCommentOfMarkersAloneIsMarkersOnly() { + assertThat(NonNlsMarkers.isMarkersOnly("//$NON-NLS-1$")).isTrue(); + assertThat(NonNlsMarkers.isMarkersOnly("//$NON-NLS-1$ //$NON-NLS-2$")).isTrue(); + assertThat(NonNlsMarkers.isMarkersOnly("//$NON-NLS-1$\t//$NON-NLS-12$")).isTrue(); + } + + @Test + void anyOtherTextMakesACommentNotMarkersOnly() { + // A blank after the slashes, text next to a marker, or a marker without its number or its closing dollar. + List comments = List.of( + "// $NON-NLS-1$", "//$NON-NLS-1$ note", "// note //$NON-NLS-1$", "//$NON-NLS-$", "//$NON-NLS-1", "// foo"); + + for (String comment : comments) { + assertThat(NonNlsMarkers.isMarkersOnly(comment)).as(comment).isFalse(); + } + } + + @Test + void numbersListsTheMarkersInTheirOrder() { + assertThat(NonNlsMarkers.numbers("//$NON-NLS-3$ //$NON-NLS-1$ //$NON-NLS-12$")).containsExactly(3, 1, 12); + } + + @Test + void numbersFindsMarkersAnywhereInAComment() { + assertThat(NonNlsMarkers.numbers("// note //$NON-NLS-2$")).containsExactly(2); + assertThat(NonNlsMarkers.numbers("// no markers here")).isEmpty(); + } + + @Test + void markerSpellsWhatEclipseReads() { + assertThat(NonNlsMarkers.marker(7)).isEqualTo("//$NON-NLS-7$"); + assertThat(NonNlsMarkers.numbers(NonNlsMarkers.marker(42))).containsExactly(42); + } +} From e17d4045aaeee4f3faf3a4821191d61d9697102b Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Sun, 25 Oct 2026 17:55:22 +0300 Subject: [PATCH 5/9] Wrap `// MOE:` line comments like any other google/MOE, the tool whose markers the line-comment wrapper kept on one line, has been dead for years. Its special case goes, with the test that pinned it; only the NON-NLS marker runs stay unwrapped. --- .../javaformat/java/JavaCommentsHelper.java | 5 ++--- .../palantir/javaformat/java/FormatterTest.java | 15 --------------- 2 files changed, 2 insertions(+), 18 deletions(-) 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 313635eee..67a07b58d 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 @@ -207,9 +207,8 @@ private List wrapLineComments(List lines, int column0) { // remainder is emitted as bare code. line = CharMatcher.whitespace().trimLeadingFrom(line); line = addMissingSpace(line); - if (line.startsWith("// MOE:") || NonNlsMarkers.isMarkersOnly(line)) { - // don't wrap comments for https://github.com/google/MOE, nor the markers Eclipse reads only on the - // line of their literal + if (NonNlsMarkers.isMarkersOnly(line)) { + // Eclipse reads the markers only on the line of their literal, so a run of them is never wrapped. result.add(line); continue; } 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 8cd168626..cddef9ed4 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 @@ -545,21 +545,6 @@ void wrapMarkdownDocstringRunKeepsTheSlashPrefix() throws Exception { assertThat(Formatter.create().formatSource(input)).isEqualTo(expected); } - @Test - void dontWrapMoeLineComments() throws Exception { - assertThat(Formatter.create() - .formatSource("class T {\n" - + " // MOE: one long incredibly" - + " unbroken sentence moving from topic to topic so that no-one had a" - + " chance to interrupt;\n" - + "}\n")) - .isEqualTo("class T {\n" - + " // MOE: one long incredibly" - + " unbroken sentence moving from topic to topic so that no-one had a" - + " chance to interrupt;\n" - + "}\n"); - } - @Test void canParse_java9_private_interface_methods() { assertThatCode(() -> Formatter.create() From 42945bd8a2cf2b0349afe7ff4cf07682b09bf5fa Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Sun, 25 Oct 2026 18:30:02 +0300 Subject: [PATCH 6/9] Start the files new in this change with the Apache header alone The fork's own files carry the Apache 2.0 header without a copyright line. The four files this change adds were created with a Palantir one; CLAUDE.md records the rule, and #154 covers the rest of the tree. --- .../main/java/com/palantir/javaformat/doc/ReservedWidth.java | 2 -- .../main/java/com/palantir/javaformat/java/NonNlsMarkers.java | 2 -- .../java/com/palantir/javaformat/doc/ReservedWidthTest.java | 2 -- .../java/com/palantir/javaformat/java/NonNlsMarkersTest.java | 2 -- 4 files changed, 8 deletions(-) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java b/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java index c96303a2d..a36083d11 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java @@ -1,6 +1,4 @@ /* - * (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 diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java b/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java index c6cd1a115..70e4942d3 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java @@ -1,6 +1,4 @@ /* - * (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 diff --git a/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java b/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java index cfddbaa48..ce7a4e149 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java @@ -1,6 +1,4 @@ /* - * (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 diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java index a22195ee9..b1bb261bd 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java @@ -1,6 +1,4 @@ /* - * (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 From 3d97ebe03d24500391e437896e4aceef17f1fe89 Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Sun, 25 Oct 2026 18:30:28 +0300 Subject: [PATCH 7/9] Drop Error Prone's @Immutable from the marker nodes Comment carried the annotation and needed a suppression for its List field, and ReservedWidth had taken the annotation from its sibling nodes. Doc itself is not annotated, so the check adds nothing here: the nodes are immutable by construction. --- .../src/main/java/com/palantir/javaformat/doc/Comment.java | 3 --- .../main/java/com/palantir/javaformat/doc/ReservedWidth.java | 2 -- 2 files changed, 5 deletions(-) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java b/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java index 54b2a2d0d..68821b9d2 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java @@ -19,7 +19,6 @@ import com.google.common.base.MoreObjects; import com.google.common.collect.Iterators; import com.google.common.collect.Range; -import com.google.errorprone.annotations.Immutable; import com.palantir.javaformat.CommentsHelper; import com.palantir.javaformat.Input; import com.palantir.javaformat.Newlines; @@ -30,12 +29,10 @@ import java.util.List; /** A leaf node in a {@link Doc} for a non-token. */ -@Immutable public final class Comment extends Doc implements Op { private final Input.Tok tok; /** For a comment of NON-NLS markers, the index of the string literal each marker marks, or -1; else empty. */ - @SuppressWarnings("Immutable") // List.of and List.copyOf: unmodifiable private final List nonNlsLiterals; private Comment(Input.Tok tok, List nonNlsLiterals) { diff --git a/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java b/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java index a36083d11..bb2c80927 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/doc/ReservedWidth.java @@ -16,7 +16,6 @@ import com.google.common.base.MoreObjects; import com.google.common.collect.Range; -import com.google.errorprone.annotations.Immutable; import com.palantir.javaformat.CommentsHelper; import com.palantir.javaformat.Op; import com.palantir.javaformat.Output; @@ -26,7 +25,6 @@ * {@code //$NON-NLS-n$} marker after the string literal it marks, which {@link Comment} writes at the end of the * literal's line once the line breaks are known. So the layout already counts the marker on that line. */ -@Immutable public final class ReservedWidth extends Doc implements Op { private final int width; From a67fa95bf2b87275fb761f7cd5a9265f7a100bd2 Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Sun, 25 Oct 2026 18:30:28 +0300 Subject: [PATCH 8/9] Use var for the locals of the marker code The locals this change added declare their type with var where the initializer gives it unchanged. The loop variables over a List keep int, so that they are not boxed, and the lambda keeps its target type. --- .../com/palantir/javaformat/OpsBuilder.java | 31 +++++----- .../com/palantir/javaformat/doc/Comment.java | 6 +- .../javaformat/java/JavaCommentsHelper.java | 4 +- .../palantir/javaformat/java/JavaOutput.java | 18 +++--- .../javaformat/java/NonNlsMarkers.java | 3 +- .../javaformat/java/StringWrapper.java | 59 +++++++++---------- .../javaformat/doc/ReservedWidthTest.java | 6 +- .../javaformat/java/FormatterTest.java | 14 ++--- .../java/JavaCommentsHelperTest.java | 6 +- .../javaformat/java/NonNlsMarkersTest.java | 13 ++-- 10 files changed, 82 insertions(+), 78 deletions(-) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java b/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java index 14df20693..ae3e0852d 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Multimap; -import com.google.common.collect.Range; import com.google.errorprone.annotations.Immutable; import com.palantir.javaformat.Indent.Const; import com.palantir.javaformat.Input.Tok; @@ -511,8 +510,8 @@ public OpsOutput build() { // Rewrite the ops to insert comments. Multimap tokOps = ArrayListMultimap.create(); // The room after a string literal for the NON-NLS markers written on its line, see Comment.makeNonNlsMarkers. - Multimap reservations = ArrayListMultimap.create(); - Map tokenOps = new HashMap<>(); + var reservations = ArrayListMultimap.create(); + var tokenOps = new HashMap(); Map tokenByIndex = null; int opsN = ops.size(); for (int i = 0; i < opsN; i++) { @@ -608,14 +607,14 @@ public OpsOutput build() { tokenByIndex.put(t.getTok().getIndex(), t); } } - List literals = nonNlsLiterals(token, tokAfter, tokenByIndex, tokenOps); - List numbers = NonNlsMarkers.numbers(tokAfter.getText()); - for (int m = 0; m < literals.size(); m++) { - Integer literalOp = literals.get(m) < 0 + var literals = nonNlsLiterals(token, tokAfter, tokenByIndex, tokenOps); + var numbers = NonNlsMarkers.numbers(tokAfter.getText()); + for (var m = 0; m < literals.size(); m++) { + var literalOp = literals.get(m) < 0 ? null : tokenOps.get(tokenByIndex.get(literals.get(m))); if (literalOp != null) { - String marker = NonNlsMarkers.marker(numbers.get(m)); + var marker = NonNlsMarkers.marker(numbers.get(m)); reservations.put(literalOp + 1, ReservedWidth.make(marker.length() + 1)); } } @@ -706,20 +705,20 @@ private List nonNlsLiterals( Input.Tok comment, Map tokenByIndex, Map tokenOps) { - int line = input.getLineNumber(token.getTok().getPosition()) - 1; - Range toks = input.getRanges(line); - List literals = new ArrayList<>(); + var line = input.getLineNumber(token.getTok().getPosition()) - 1; + var toks = input.getRanges(line); + var literals = new ArrayList(); for (int k = toks.lowerEndpoint(); k < Math.min(toks.upperEndpoint(), comment.getIndex()); k++) { - Input.Token candidate = tokenByIndex.get(k); + var candidate = tokenByIndex.get(k); if (candidate != null && candidate.getTok().getText().startsWith("\"")) { literals.add(k); } } - Range region = inputMetadataBuilder.regionContaining(comment.getIndex()); - List result = new ArrayList<>(); + var region = inputMetadataBuilder.regionContaining(comment.getIndex()); + var result = new ArrayList(); for (int n : NonNlsMarkers.numbers(comment.getText())) { - int literal = 1 <= n && n <= literals.size() ? literals.get(n - 1) : -1; - boolean usable = literal >= 0 + var literal = 1 <= n && n <= literals.size() ? literals.get(n - 1) : -1; + var usable = literal >= 0 && region != null && region.contains(literal) && tokenOps.containsKey(tokenByIndex.get(literal)); diff --git a/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java b/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java index 68821b9d2..49297093e 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/doc/Comment.java @@ -69,9 +69,9 @@ public void add(DocBuilder builder) { @Override protected float computeWidth() { if (!nonNlsLiterals.isEmpty()) { - int width = 0; - List numbers = NonNlsMarkers.numbers(tok.getOriginalText()); - for (int i = 0; i < numbers.size(); i++) { + var width = 0; + var numbers = NonNlsMarkers.numbers(tok.getOriginalText()); + for (var i = 0; i < numbers.size(); i++) { if (nonNlsLiterals.get(i) < 0) { width += (width > 0 ? 1 : 0) + NonNlsMarkers.marker(numbers.get(i)).length(); 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 67a07b58d..6c0f89f43 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 @@ -181,9 +181,9 @@ private static boolean isJBangDirective(String text) { * written flat, for which {@link #rewrite} is not called. */ public static String addMissingSpace(String line) { - Matcher matcher = LINE_COMMENT_MISSING_SPACE_PREFIX.matcher(line); + var matcher = LINE_COMMENT_MISSING_SPACE_PREFIX.matcher(line); if (matcher.find() && !LINE_COMMENT_NO_SPACE_PREFIX.matcher(line).find()) { - int length = matcher.group(1).length(); + var length = matcher.group(1).length(); return "/".repeat(length) + " " + line.substring(length); } return line; diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java b/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java index 2a5b74ab4..1b254f680 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/JavaOutput.java @@ -163,23 +163,23 @@ public void append(State state, String text, Range range) { */ @Override public void appendNonNlsMarkers(State state, String comment, Range range, List literals) { - List numbers = NonNlsMarkers.numbers(comment); - int current = mutableLines.size(); + var numbers = NonNlsMarkers.numbers(comment); + var current = mutableLines.size(); if (numbers.size() != literals.size() || range.isEmpty() || newlinesPending > 0 || ranges.size() <= current) { // Not a comment as paired with its literals, or not trailing the code of the current line. append(state, comment, range); return; } - List remaining = new ArrayList<>(); - for (int i = 0; i < numbers.size(); i++) { + var remaining = new ArrayList(); + for (var i = 0; i < numbers.size(); i++) { int literal = literals.get(i); - int line = literal < 0 ? -1 : outputLineOf(literal, current); + var line = literal < 0 ? -1 : outputLineOf(literal, current); if (line < 0) { remaining.add(NonNlsMarkers.marker(numbers.get(i))); } else if (line == current) { remaining.add(NonNlsMarkers.marker(stringLiteralsOnOutputLine(line, literal))); } else { - String marker = NonNlsMarkers.marker(stringLiteralsOnOutputLine(line, literal)); + var marker = NonNlsMarkers.marker(stringLiteralsOnOutputLine(line, literal)); mutableLines.set(line, mutableLines.get(line) + " " + marker); } } @@ -195,7 +195,7 @@ public void appendNonNlsMarkers(State state, String comment, Range rang /** The output line, searched back from {@code current}, that holds tok {@code k}, or -1 if it is not written. */ private int outputLineOf(int k, int current) { - for (int line = current; line >= 0; line--) { + for (var line = current; line >= 0; line--) { if (ranges.get(line).contains(k)) { return line; } @@ -205,9 +205,9 @@ private int outputLineOf(int k, int current) { /** The position of the string literal {@code k} among the string literals written on an output line. */ private int stringLiteralsOnOutputLine(int line, int k) { - int count = 0; + var count = 0; for (int i = ranges.get(line).lowerEndpoint(); i <= k; i++) { - Input.Tok tok = javaInput.getToken(i).getTok(); + var tok = javaInput.getToken(i).getTok(); if (tok.getIndex() == i && tok.getText().startsWith("\"")) { count++; } diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java b/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java index 70e4942d3..312760db3 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/NonNlsMarkers.java @@ -34,7 +34,8 @@ public final class NonNlsMarkers { Pattern.compile("//\\$NON-NLS-\\d{1,9}\\$([ \\t]+//\\$NON-NLS-\\d{1,9}\\$)*"); public static boolean isMarkersOnly(String comment) { - return comment.startsWith("//$NON-NLS-") && MARKERS_ONLY.matcher(comment).matches(); + return comment.startsWith("//$NON-NLS-") + && MARKERS_ONLY.matcher(comment).matches(); } /** The numbers of the markers in a comment, in order; an unmodifiable list. */ diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java b/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java index 32831222e..d62ea7e1d 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java @@ -379,7 +379,7 @@ private void wrapLongStrings( TreeRangeMap replacements, List longStringLiterals, List stringLiterals) { - List chains = new ArrayList<>(); + var chains = new ArrayList(); for (TreePath path : longStringLiterals) { // Find the outermost contiguous enclosing concatenation expression TreePath enclosing = path; @@ -418,8 +418,7 @@ private void wrapLongStrings( // Get the original source text of the string literals, excluding `"` and `+`. ImmutableList components = stringComponents(input, unit, flat); - List plain = - reflow(columnLimit, trailing, components, first.get(), startColumn, fistLineCol, "", ""); + var plain = reflow(columnLimit, trailing, components, first.get(), startColumn, fistLineCol, "", ""); chains.add(new Chain( flat, first.get(), @@ -430,19 +429,19 @@ private void wrapLongStrings( fistLineCol, plain)); } - for (Chain chain : chains) { - String joined = join(chain, chain.plain(), "", ""); + for (var chain : chains) { + var joined = join(chain, chain.plain(), "", ""); if (joined.equals(input.substring(chain.start(), chain.end()))) { // Every literal on a line past the limit is a candidate; this one does not change. continue; } - LineMarkers markers = lineMarkers(chains, chain, stringLiterals); + var markers = lineMarkers(chains, chain, stringLiterals); if (markers == null) { replacements.put(Range.closedOpen(chain.start(), chain.end()), joined); continue; } - ImmutableList components = stringComponents(input, unit, chain.flat()); - List pieces = reflow( + var components = stringComponents(input, unit, chain.flat()); + var pieces = reflow( columnLimit, markers.tail(2).length(), components, @@ -476,14 +475,14 @@ private String join(Chain chain, List pieces, String firstLineMarkers, S */ @Nullable private LineMarkers lineMarkers(List chains, Chain chain, List stringLiterals) { - String tail = input.substring(chain.end(), chain.lineEnd()); - int comment = lineCommentStart(tail); + var tail = input.substring(chain.end(), chain.lineEnd()); + var comment = lineCommentStart(tail); if (comment < 0 || !NonNlsMarkers.isMarkersOnly(tail.substring(comment).stripTrailing())) { return null; } - long lastLine = lineMap.getLineNumber(chain.end() - 1); - for (Chain other : chains) { + var lastLine = lineMap.getLineNumber(chain.end() - 1); + for (var other : chains) { // The same chain is found once per long literal in it. if (other.start() != chain.start() && lineMap.getLineNumber(other.end() - 1) == lastLine @@ -491,22 +490,22 @@ private LineMarkers lineMarkers(List chains, Chain chain, List stri return null; } } - List literals = literalsOnLine(stringLiterals, lastLine); - int lastOfChain = literals.indexOf(getLast(chain.flat())); + var literals = literalsOnLine(stringLiterals, lastLine); + var lastOfChain = literals.indexOf(getLast(chain.flat())); if (lastOfChain < 0) { // A text block, which starts on another line. return null; } - int firstOfChain = lastOfChain; + var firstOfChain = lastOfChain; while (firstOfChain > 0 && chain.flat().contains(literals.get(firstOfChain - 1))) { firstOfChain--; } - List before = new ArrayList<>(); - List after = new ArrayList<>(); - List unmatched = new ArrayList<>(); - boolean chainMarked = false; + var before = new ArrayList(); + var after = new ArrayList(); + var unmatched = new ArrayList(); + var chainMarked = false; for (int n : NonNlsMarkers.numbers(tail.substring(comment))) { - int i = n - 1; + var i = n - 1; if (i < 0 || i >= literals.size()) { unmatched.add(NonNlsMarkers.marker(n)); } else if (i < firstOfChain) { @@ -517,7 +516,7 @@ private LineMarkers lineMarkers(List chains, Chain chain, List stri after.add(i - lastOfChain); } } - long beforeOnFirstLine = literalsOnLine(stringLiterals, lineMap.getLineNumber(chain.start())).stream() + var beforeOnFirstLine = literalsOnLine(stringLiterals, lineMap.getLineNumber(chain.start())).stream() .filter(literal -> getStartPosition(literal) < chain.start()) .count(); return new LineMarkers( @@ -540,7 +539,7 @@ private record LineMarkers( /** The markers after the first piece, when it ends its line. */ String firstLine() { - List markers = new ArrayList<>(before); + var markers = new ArrayList<>(before); if (chainMarked) { markers.add(NonNlsMarkers.marker(beforeOnFirstLine + 1)); } @@ -554,8 +553,8 @@ String otherLines() { /** The text after the last of {@code pieces}: the code, then the markers of that line. */ String tail(int pieces) { - List markers = new ArrayList<>(); - int chainPosition = pieces > 1 ? 1 : beforeOnFirstLine + 1; + var markers = new ArrayList(); + var chainPosition = pieces > 1 ? 1 : beforeOnFirstLine + 1; if (pieces == 1) { markers.addAll(before); } @@ -580,8 +579,8 @@ private List literalsOnLine(List stringLiterals, long line) { /** The index of the line comment in a line of code, the first {@code //} outside a literal, or -1. */ private static int lineCommentStart(String line) { - for (int i = 0; i < line.length(); i++) { - char c = line.charAt(i); + for (var i = 0; i < line.length(); i++) { + var c = line.charAt(i); if (c == '"' || c == '\'') { for (i++; i < line.length() && line.charAt(i) != c; i++) { if (line.charAt(i) == '\\') { @@ -702,7 +701,7 @@ private static List reflow( String otherLineMarkers) { // We have space between the start column and the limit to output the first line. // Reserve two spaces for the quotes, and room for the markers of the line. - int width = columnLimit - textStartColumn - 2 - firstLineMarkers.length(); + var width = columnLimit - textStartColumn - 2 - firstLineMarkers.length(); Deque input = new ArrayDeque<>(components); List lines = new ArrayList<>(); boolean first = first0; @@ -748,9 +747,9 @@ private static List reflow( /** Renders reflowed pieces as literals in a chain of {@code +}, one per line, indented by {@code column}. */ private static String render( String separator, List pieces, int column, String firstLineMarkers, String otherLineMarkers) { - String continuation = separator + " ".repeat(column) + "+ "; - StringBuilder result = new StringBuilder(); - for (int i = 0; i < pieces.size(); i++) { + var continuation = separator + " ".repeat(column) + "+ "; + var result = new StringBuilder(); + for (var i = 0; i < pieces.size(); i++) { if (i > 0) { result.append(continuation); } diff --git a/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java b/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java index ce7a4e149..866c9d53a 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/doc/ReservedWidthTest.java @@ -26,7 +26,7 @@ final class ReservedWidthTest { @Test void takesItsWidthOnTheLineButHasNoText() { - ReservedWidth reserved = ReservedWidth.make(WIDTH); + var reserved = ReservedWidth.make(WIDTH); assertThat(reserved.getWidth()).isEqualTo((float) WIDTH); assertThat(reserved.getFlat()).isEmpty(); @@ -35,10 +35,10 @@ void takesItsWidthOnTheLineButHasNoText() { @Test void movesTheColumnByItsWidthWhenTheBreaksAreComputed() { - State before = State.startingState().withColumn(10); + var before = State.startingState().withColumn(10); CommentsHelper noRewrite = (tok, maxWidth, column0) -> tok.getOriginalText(); - State after = ReservedWidth.make(WIDTH).computeBreaks(noRewrite, 120, before, Obs.createRoot(new NoopSink())); + var after = ReservedWidth.make(WIDTH).computeBreaks(noRewrite, 120, before, Obs.createRoot(new NoopSink())); assertThat(after.column()).isEqualTo(10 + WIDTH); } 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 cddef9ed4..3f75b8ea8 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 @@ -403,7 +403,7 @@ void lineCommentGetsItsMissingSpaceExceptIdeMarkers() throws Exception { void trailingLineCommentGetsItsMissingSpaceExceptIdeMarkers() throws Exception { // The same rule for a comment that trails a statement which fits on one line. Such a statement is written // from its flat text, which skips the comment rewriting, so the IDE markers used to get the space too (#23). - String input = """ + var input = """ class T { void m() { String a = "x"; //$NON-NLS-1$ @@ -415,7 +415,7 @@ void m() { } } """; - String expected = """ + var expected = """ class T { void m() { String a = "x"; //$NON-NLS-1$ @@ -428,7 +428,7 @@ void m() { } """; - String once = Formatter.create().formatSource(input); + var once = Formatter.create().formatSource(input); assertThat(once).isEqualTo(expected); assertThat(Formatter.create().formatSource(once)).isEqualTo(once); } @@ -438,7 +438,7 @@ void nonNlsMarkersStayOnTheLineOfTheirLiteral() throws Exception { // Eclipse numbers a marker by the position of its literal on the line. When a statement is wrapped, each // marker moves to the line of its literal and is renumbered for that line; when a literal is reflowed, every // piece gets one (#23). A marker whose number has no literal stays where it is. - String input = """ + var input = """ class T { void m() { String s = Messages.format("first argument that is long enough to matter", "second argument that is also rather long", "third one"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ @@ -447,7 +447,7 @@ void m() { } } """; - String expected = """ + var expected = """ class T { void m() { String s = Messages.format( @@ -465,9 +465,9 @@ void m() { } """; - Formatter formatter = Formatter.createFormatter( + var formatter = Formatter.createFormatter( JavaFormatterOptions.builder().style(Style.OJF).build()); - String once = formatter.formatSourceAndFixImports(input); + var once = formatter.formatSourceAndFixImports(input); assertThat(once).isEqualTo(expected); assertThat(formatter.formatSourceAndFixImports(once)).isEqualTo(once); } 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 index 9c14ec878..59e564baf 100644 --- 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 @@ -117,7 +117,7 @@ void anOrdinaryLineCommentStillWraps() throws Exception { void aRunOfNonNlsMarkersIsNeverWrapped() throws Exception { // Eclipse reads a marker only on the line of its literal, so a run that reaches past the limit stays as it is // instead of being wrapped at a blank into a `// //$NON-NLS-3$` line of its own. - String run = "//$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$" + var run = "//$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$" + " //$NON-NLS-8$ //$NON-NLS-9$"; assertThat(rewrite(run, Style.OJF)).isEqualTo(run); @@ -133,7 +133,7 @@ void addMissingSpacePutsABlankAfterTheSlashesWhateverTheirNumber() { @Test void addMissingSpaceLeavesSpacedCommentsAndIdeMarkersAlone() { // A blank or a tab after the slashes is a space already; the IDE markers are recognised only without one. - List comments = List.of( + var comments = List.of( "// foo", "// two spaces", "//\tfoo", @@ -142,7 +142,7 @@ void addMissingSpaceLeavesSpacedCommentsAndIdeMarkersAlone() { "//$NON-NLS-1$ //$NON-NLS-2$", "//noinspection unchecked"); - for (String comment : comments) { + for (var comment : comments) { assertThat(JavaCommentsHelper.addMissingSpace(comment)).as(comment).isEqualTo(comment); } } diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java index b1bb261bd..e53c363ec 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/NonNlsMarkersTest.java @@ -31,10 +31,15 @@ void aCommentOfMarkersAloneIsMarkersOnly() { @Test void anyOtherTextMakesACommentNotMarkersOnly() { // A blank after the slashes, text next to a marker, or a marker without its number or its closing dollar. - List comments = List.of( - "// $NON-NLS-1$", "//$NON-NLS-1$ note", "// note //$NON-NLS-1$", "//$NON-NLS-$", "//$NON-NLS-1", "// foo"); - - for (String comment : comments) { + var comments = List.of( + "// $NON-NLS-1$", + "//$NON-NLS-1$ note", + "// note //$NON-NLS-1$", + "//$NON-NLS-$", + "//$NON-NLS-1", + "// foo"); + + for (var comment : comments) { assertThat(NonNlsMarkers.isMarkersOnly(comment)).as(comment).isFalse(); } } From 308c0ad45d6f71343f2863fe65a15b259872eedc Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Sun, 25 Oct 2026 18:30:29 +0300 Subject: [PATCH 9/9] Write down the conventions for new code CLAUDE.md: plain java.util collections instead of Guava's immutable ones, javax.annotation.Nullable as the one nullability annotation, var for locals, no Error Prone @Immutable, and the Apache header without a copyright line on a new file. --- CLAUDE.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..695521c1e --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,35 @@ +# Working in this repository + +Building and testing: the README's "Building and testing locally" section. Gradle runs on the Temurin 21 that +`mise install` provides, `./gradlew test` is what CI runs, and `./gradlew formatDiff` formats the files you changed. + +## Conventions for new code + +- **Collections.** Plain `java.util` types. `List.of`, `List.copyOf`, `Map.of` and `Stream.toList()` give + unmodifiable collections, so new code does not need Guava's `ImmutableList` or `ImmutableMap`; use them only + where an existing API already hands one over. +- **Nullability.** One annotation, `javax.annotation.Nullable` (jsr305), the one the code uses; do not add another + `Nullable`. +- **Local variables.** `var` wherever the initializer gives the type unchanged, in `for` loops too. Not where it + would box a primitive (`for (var n : List)`), and not without an initializer that has a type of its own + (a lambda, `null`). +- **Immutability annotations.** Do not add Error Prone's `@Immutable` or `@SuppressWarnings("Immutable")`. +- **License header.** A new file starts with the Apache 2.0 header alone, without a copyright line: + + ```java + /* + * 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. + */ + ``` + + Files inherited from google-java-format and palantir-java-format keep their original headers.