Skip to content

Commit 8f3a269

Browse files
authored
Merge pull request #150 from openjavaformat/fix/non-nls-marker-space
Keep //$NON-NLS-n$ markers valid: no inserted space, each marker on the line of its string
2 parents 8932ddc + 308c0ad commit 8f3a269

22 files changed

Lines changed: 957 additions & 56 deletions

‎CLAUDE.md‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
# Working in this repository
2+
3+
Building and testing: the README's "Building and testing locally" section. Gradle runs on the Temurin 21 that
4+
`mise install` provides, `./gradlew test` is what CI runs, and `./gradlew formatDiff` formats the files you changed.
5+
6+
## Conventions for new code
7+
8+
- **Collections.** Plain `java.util` types. `List.of`, `List.copyOf`, `Map.of` and `Stream.toList()` give
9+
unmodifiable collections, so new code does not need Guava's `ImmutableList` or `ImmutableMap`; use them only
10+
where an existing API already hands one over.
11+
- **Nullability.** One annotation, `javax.annotation.Nullable` (jsr305), the one the code uses; do not add another
12+
`Nullable`.
13+
- **Local variables.** `var` wherever the initializer gives the type unchanged, in `for` loops too. Not where it
14+
would box a primitive (`for (var n : List<Integer>)`), and not without an initializer that has a type of its own
15+
(a lambda, `null`).
16+
- **Immutability annotations.** Do not add Error Prone's `@Immutable` or `@SuppressWarnings("Immutable")`.
17+
- **License header.** A new file starts with the Apache 2.0 header alone, without a copyright line:
18+
19+
```java
20+
/*
21+
* Licensed under the Apache License, Version 2.0 (the "License");
22+
* you may not use this file except in compliance with the License.
23+
* You may obtain a copy of the License at
24+
*
25+
* http://www.apache.org/licenses/LICENSE-2.0
26+
*
27+
* Unless required by applicable law or agreed to in writing, software
28+
* distributed under the License is distributed on an "AS IS" BASIS,
29+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
30+
* See the License for the specific language governing permissions and
31+
* limitations under the License.
32+
*/
33+
```
34+
35+
Files inherited from google-java-format and palantir-java-format keep their original headers.

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

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,18 @@
3030
import com.palantir.javaformat.doc.DocBuilder;
3131
import com.palantir.javaformat.doc.FillMode;
3232
import com.palantir.javaformat.doc.NonBreakingSpace;
33+
import com.palantir.javaformat.doc.ReservedWidth;
3334
import com.palantir.javaformat.doc.State;
3435
import com.palantir.javaformat.doc.Token;
3536
import com.palantir.javaformat.doc.Token.RealOrImaginary;
3637
import com.palantir.javaformat.java.FormatterDiagnostic;
3738
import com.palantir.javaformat.java.InputMetadata;
3839
import com.palantir.javaformat.java.InputMetadataBuilder;
40+
import com.palantir.javaformat.java.NonNlsMarkers;
3941
import java.util.ArrayList;
42+
import java.util.HashMap;
4043
import java.util.List;
44+
import java.util.Map;
4145
import java.util.Optional;
4246

4347
/** An {@code OpsBuilder} creates a list of {@link Op}s, which is turned into a {@link Doc} by {@link DocBuilder}. */
@@ -505,6 +509,10 @@ public OpsOutput build() {
505509
markForPartialFormat();
506510
// Rewrite the ops to insert comments.
507511
Multimap<Integer, Op> tokOps = ArrayListMultimap.create();
512+
// The room after a string literal for the NON-NLS markers written on its line, see Comment.makeNonNlsMarkers.
513+
var reservations = ArrayListMultimap.<Integer, Op>create();
514+
var tokenOps = new HashMap<Input.Token, Integer>();
515+
Map<Integer, Input.Token> tokenByIndex = null;
508516
int opsN = ops.size();
509517
for (int i = 0; i < opsN; i++) {
510518
Op op = ops.get(i);
@@ -528,6 +536,7 @@ public OpsOutput build() {
528536
* Regular input token. Copy out toksBefore before token, and toksAfter after it. Insert
529537
* this token's toksBefore at position j.
530538
*/
539+
tokenOps.put(token, i);
531540
int newlines = 0; // Count of newlines in a row.
532541
boolean space = false; // Do we need an extra space after a previous "/*" comment?
533542
boolean lastWasComment = false; // Was the last thing we output a comment?
@@ -591,7 +600,29 @@ public OpsOutput build() {
591600
} else {
592601
tokOps.put(tokAfterPos, SPACE);
593602
}
594-
tokOps.putAll(tokAfterPos, makeComment(tokAfter));
603+
if (tokAfter.isSlashSlashComment() && NonNlsMarkers.isMarkersOnly(tokAfter.getText())) {
604+
if (tokenByIndex == null) {
605+
tokenByIndex = new HashMap<>();
606+
for (Input.Token t : input.getTokens()) {
607+
tokenByIndex.put(t.getTok().getIndex(), t);
608+
}
609+
}
610+
var literals = nonNlsLiterals(token, tokAfter, tokenByIndex, tokenOps);
611+
var numbers = NonNlsMarkers.numbers(tokAfter.getText());
612+
for (var m = 0; m < literals.size(); m++) {
613+
var literalOp = literals.get(m) < 0
614+
? null
615+
: tokenOps.get(tokenByIndex.get(literals.get(m)));
616+
if (literalOp != null) {
617+
var marker = NonNlsMarkers.marker(numbers.get(m));
618+
reservations.put(literalOp + 1, ReservedWidth.make(marker.length() + 1));
619+
}
620+
}
621+
tokOps.put(tokAfterPos, Comment.makeNonNlsMarkers(tokAfter, literals));
622+
tokOps.put(tokAfterPos, Break.makeForced());
623+
} else {
624+
tokOps.putAll(tokAfterPos, makeComment(tokAfter));
625+
}
595626
if (breakAfter) {
596627
tokOps.put(tokAfterPos, Break.make(FillMode.FORCED, "", ZERO));
597628
}
@@ -630,6 +661,7 @@ public OpsOutput build() {
630661
ImmutableList.Builder<Op> newOps = ImmutableList.builder();
631662
boolean afterForcedBreak = false; // Was the last Op a forced break? If so, suppress spaces.
632663
for (int i = 0; i < opsN; i++) {
664+
newOps.addAll(reservations.get(i));
633665
for (Op op : tokOps.get(i)) {
634666
if (!(afterForcedBreak && op instanceof NonBreakingSpace)) {
635667
newOps.add(op);
@@ -662,6 +694,39 @@ private static boolean isNonNlsComment(Input.Tok tokAfter) {
662694
return tokAfter.isSlashSlashComment() && tokAfter.getText().contains("$NON-NLS");
663695
}
664696

697+
/**
698+
* For each marker of a {@code //$NON-NLS-1$ //$NON-NLS-2$} comment that trails {@code token}, the index of the
699+
* string literal it marks, the n-th one on the comment's input line, or -1 when there is none. A literal in
700+
* another partial-format region counts as none too: a partial format of its region would otherwise lose the
701+
* marker, which belongs to the comment's region. So does a literal without an op of its own.
702+
*/
703+
private List<Integer> nonNlsLiterals(
704+
Input.Token token,
705+
Input.Tok comment,
706+
Map<Integer, Input.Token> tokenByIndex,
707+
Map<Input.Token, Integer> tokenOps) {
708+
var line = input.getLineNumber(token.getTok().getPosition()) - 1;
709+
var toks = input.getRanges(line);
710+
var literals = new ArrayList<Integer>();
711+
for (int k = toks.lowerEndpoint(); k < Math.min(toks.upperEndpoint(), comment.getIndex()); k++) {
712+
var candidate = tokenByIndex.get(k);
713+
if (candidate != null && candidate.getTok().getText().startsWith("\"")) {
714+
literals.add(k);
715+
}
716+
}
717+
var region = inputMetadataBuilder.regionContaining(comment.getIndex());
718+
var result = new ArrayList<Integer>();
719+
for (int n : NonNlsMarkers.numbers(comment.getText())) {
720+
var literal = 1 <= n && n <= literals.size() ? literals.get(n - 1) : -1;
721+
var usable = literal >= 0
722+
&& region != null
723+
&& region.contains(literal)
724+
&& tokenOps.containsKey(tokenByIndex.get(literal));
725+
result.add(usable ? literal : -1);
726+
}
727+
return List.copyOf(result);
728+
}
729+
665730
private static boolean isForcedBreak(Op op) {
666731
return op instanceof Break b && b.isForced();
667732
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import com.google.common.base.MoreObjects;
1818
import com.google.common.collect.Range;
1919
import com.palantir.javaformat.doc.State;
20+
import java.util.List;
2021

2122
/** An output from the formatter. */
2223
public abstract class Output extends InputOutput {
@@ -36,6 +37,13 @@ public abstract class Output extends InputOutput {
3637
*/
3738
public abstract void append(State state, String text, Range<Integer> range);
3839

40+
/**
41+
* Output a comment of {@code //$NON-NLS-n$} markers, see {@link com.palantir.javaformat.java.NonNlsMarkers}.
42+
*
43+
* @param literals for each marker, the index of the string literal it marks, or -1
44+
*/
45+
public abstract void appendNonNlsMarkers(State state, String comment, Range<Integer> range, List<Integer> literals);
46+
3947
@Override
4048
public String toString() {
4149
return MoreObjects.toStringHelper(this).add("super", super.toString()).toString();

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

Lines changed: 44 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,20 +19,25 @@
1919
import com.google.common.base.MoreObjects;
2020
import com.google.common.collect.Iterators;
2121
import com.google.common.collect.Range;
22-
import com.google.errorprone.annotations.Immutable;
2322
import com.palantir.javaformat.CommentsHelper;
2423
import com.palantir.javaformat.Input;
2524
import com.palantir.javaformat.Newlines;
2625
import com.palantir.javaformat.Op;
2726
import com.palantir.javaformat.Output;
27+
import com.palantir.javaformat.java.JavaCommentsHelper;
28+
import com.palantir.javaformat.java.NonNlsMarkers;
29+
import java.util.List;
2830

2931
/** A leaf node in a {@link Doc} for a non-token. */
30-
@Immutable
3132
public final class Comment extends Doc implements Op {
3233
private final Input.Tok tok;
3334

34-
private Comment(Input.Tok tok) {
35+
/** For a comment of NON-NLS markers, the index of the string literal each marker marks, or -1; else empty. */
36+
private final List<Integer> nonNlsLiterals;
37+
38+
private Comment(Input.Tok tok, List<Integer> nonNlsLiterals) {
3539
this.tok = tok;
40+
this.nonNlsLiterals = nonNlsLiterals;
3641
}
3742

3843
/**
@@ -42,7 +47,18 @@ private Comment(Input.Tok tok) {
4247
* @return the new {@code Tok}
4348
*/
4449
public static Comment make(Input.Tok tok) {
45-
return new Comment(tok);
50+
return new Comment(tok, List.of());
51+
}
52+
53+
/**
54+
* A comment of {@code //$NON-NLS-n$} markers, see {@link NonNlsMarkers}. A marker with a literal is written at
55+
* the end of the literal's output line and takes its room there, as a {@link ReservedWidth} after the literal;
56+
* only the markers without one take room here.
57+
*
58+
* @param literals for each marker, the index of the string literal it marks, or -1
59+
*/
60+
public static Comment makeNonNlsMarkers(Input.Tok tok, List<Integer> literals) {
61+
return new Comment(tok, List.copyOf(literals));
4662
}
4763

4864
@Override
@@ -52,14 +68,25 @@ public void add(DocBuilder builder) {
5268

5369
@Override
5470
protected float computeWidth() {
71+
if (!nonNlsLiterals.isEmpty()) {
72+
var width = 0;
73+
var numbers = NonNlsMarkers.numbers(tok.getOriginalText());
74+
for (var i = 0; i < numbers.size(); i++) {
75+
if (nonNlsLiterals.get(i) < 0) {
76+
width += (width > 0 ? 1 : 0)
77+
+ NonNlsMarkers.marker(numbers.get(i)).length();
78+
}
79+
}
80+
return width;
81+
}
5582
int idx = Newlines.firstBreak(tok.getOriginalText());
5683
// only count the first line of multi-line block comments
5784
if (tok.isComment()) {
5885
if (idx > 0) {
5986
return idx;
60-
} else if (tok.isSlashSlashComment() && !tok.getOriginalText().startsWith("// ")) {
61-
// Account for line comments with missing spaces, see computeFlat.
62-
return tok.length() + 1;
87+
} else if (tok.isSlashSlashComment()) {
88+
// The flat text may have gained the space after the slashes, see computeFlat.
89+
return getFlat().length();
6390
} else {
6491
return tok.length();
6592
}
@@ -69,11 +96,11 @@ protected float computeWidth() {
6996

7097
@Override
7198
protected String computeFlat() {
72-
// TODO(cushon): commentsHelper.rewrite doesn't get called for spans that fit in a single
73-
// line. That's fine for multi-line comment reflowing, but problematic for adding missing
74-
// spaces in line comments.
75-
if (tok.isSlashSlashComment() && !tok.getOriginalText().startsWith("// ")) {
76-
return "// " + tok.getOriginalText().substring("//".length());
99+
// A level that fits on one line is written from its flat text, and commentsHelper.rewrite is not called for
100+
// the comments in it. So the space a line comment gets after its slashes is added here as well, by the rule
101+
// that rewrite applies: a trailing `//foo` and a trailing `//$NON-NLS-1$` then come out the same either way.
102+
if (tok.isSlashSlashComment()) {
103+
return JavaCommentsHelper.addMissingSpace(tok.getOriginalText());
77104
}
78105
return tok.getOriginalText();
79106
}
@@ -101,7 +128,11 @@ public State computeBreaks(
101128

102129
@Override
103130
public void write(State state, Output output) {
104-
output.append(state, state.getTokText(this), range());
131+
if (nonNlsLiterals.isEmpty()) {
132+
output.append(state, state.getTokText(this), range());
133+
} else {
134+
output.appendNonNlsMarkers(state, state.getTokText(this), range(), nonNlsLiterals);
135+
}
105136
}
106137

107138
@Override
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/*
2+
* Licensed under the Apache License, Version 2.0 (the "License");
3+
* you may not use this file except in compliance with the License.
4+
* You may obtain a copy of the License at
5+
*
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
*
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
*/
14+
15+
package com.palantir.javaformat.doc;
16+
17+
import com.google.common.base.MoreObjects;
18+
import com.google.common.collect.Range;
19+
import com.palantir.javaformat.CommentsHelper;
20+
import com.palantir.javaformat.Op;
21+
import com.palantir.javaformat.Output;
22+
23+
/**
24+
* A leaf node in a {@link Doc} that takes room on its line without writing anything: the room for a
25+
* {@code //$NON-NLS-n$} marker after the string literal it marks, which {@link Comment} writes at the end of the
26+
* literal's line once the line breaks are known. So the layout already counts the marker on that line.
27+
*/
28+
public final class ReservedWidth extends Doc implements Op {
29+
private final int width;
30+
31+
private ReservedWidth(int width) {
32+
this.width = width;
33+
}
34+
35+
public static ReservedWidth make(int width) {
36+
return new ReservedWidth(width);
37+
}
38+
39+
@Override
40+
public void add(DocBuilder builder) {
41+
builder.add(this);
42+
}
43+
44+
@Override
45+
protected float computeWidth() {
46+
return width;
47+
}
48+
49+
@Override
50+
protected String computeFlat() {
51+
return "";
52+
}
53+
54+
@Override
55+
protected Range<Integer> computeRange() {
56+
return Doc.EMPTY_RANGE;
57+
}
58+
59+
@Override
60+
public State computeBreaks(
61+
CommentsHelper commentsHelper, int maxWidth, State state, Obs.ExplorationNode observationNode) {
62+
return state.withColumn(state.column() + width);
63+
}
64+
65+
@Override
66+
public void write(State state, Output output) {}
67+
68+
@Override
69+
public String toString() {
70+
return MoreObjects.toStringHelper(this).add("width", width).toString();
71+
}
72+
}

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.palantir.javaformat.OpsBuilder.BlankLineWanted;
2525
import java.util.HashMap;
2626
import java.util.Map;
27+
import javax.annotation.Nullable;
2728

2829
public final class InputMetadataBuilder {
2930
private final Map<Integer, BlankLineWanted> blankLines = new HashMap<>(); // Info on blank lines.
@@ -50,6 +51,12 @@ public void markForPartialFormat(Input.Token start, Input.Token end) {
5051
partialFormatRanges.add(Range.closed(lo, hi));
5152
}
5253

54+
/** The region marked for partial formatting that holds tok {@code k}, or null. */
55+
@Nullable
56+
public Range<Integer> regionContaining(int k) {
57+
return partialFormatRanges.rangeContaining(k);
58+
}
59+
5360
public InputMetadata build() {
5461
return new InputMetadata(ImmutableMap.copyOf(blankLines), ImmutableRangeSet.copyOf(partialFormatRanges));
5562
}

0 commit comments

Comments
 (0)