Fix: NON-NLS comments move before + in string concatenations (#45)

NON-NLS comments are moved behind `+` tokens in string concatenations. This is a heuristic that doesn't look at what the thing before the `+` is, so it might produce confusing results.

Author: dansanduleac

changelog/@unreleased/pr-45.v2.yml

@@ -0,0 +1,7 @@
+type: fix
+fix:
+  description: NON-NLS comments are moved behind `+` tokens in string concatenations.
+    This is a heuristic that doesn't look at what the thing before the `+` is, so
+    it might produce confusing results.
+  links:
+  - https://github.com/palantir/palantir-java-format/pull/45

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

@@ -541,21 +541,32 @@ public final ImmutableList<Op> build() {
                         tokOps.put(j, SPACE);
                     }
                     // Now we've seen the Token; output the toksAfter.
+
+                    // Reordering of NON-NLS comments that might follow a `+` in a chain of string concatenations, in
+                    // order to move the comments before the Break that precedes the `+` token.
+                    boolean nonNlsCommentsAfterPlus = token.getToksAfter().stream()
+                                    .anyMatch(OpsBuilder::isNonNlsComment)
+                            && token.getTok().getText().equals("+")
+                            && k > 0
+                            && ops.get(k - 1) instanceof Break;
+
+                    int tokAfterPos = nonNlsCommentsAfterPlus ? k - 1 : k + 1;
+
                     for (Input.Tok tokAfter : token.getToksAfter()) {
                         if (tokAfter.isComment()) {
                             boolean breakAfter = tokAfter.isJavadocComment()
                                     || (tokAfter.isSlashStarComment()
                                             && tokenOp.breakAndIndentTrailingComment().isPresent());
                             if (breakAfter) {
-                                tokOps.put(k + 1, Break.make(
+                                tokOps.put(tokAfterPos, Break.make(
                                         FillMode.FORCED, "", tokenOp.breakAndIndentTrailingComment().orElse(
                                                         Const.ZERO)));
                             } else {
-                                tokOps.put(k + 1, SPACE);
+                                tokOps.put(tokAfterPos, SPACE);
                             }
-                            tokOps.putAll(k + 1, makeComment(tokAfter));
+                            tokOps.putAll(tokAfterPos, makeComment(tokAfter));
                             if (breakAfter) {
-                                tokOps.put(k + 1, Break.make(FillMode.FORCED, "", ZERO));
+                                tokOps.put(tokAfterPos, Break.make(FillMode.FORCED, "", ZERO));
                             }
                         }
                     }
@@ -620,6 +631,10 @@ public final ImmutableList<Op> build() {
         return newOps.build();
     }
 
+    private static boolean isNonNlsComment(Input.Tok tokAfter) {
+        return tokAfter.isSlashSlashComment() && tokAfter.getText().contains("$NON-NLS");
+    }
+
     private static boolean isForcedBreak(Op op) {
         return op instanceof Break && ((Break) op).isForced();
     }

palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/NON-NLS.output

@@ -3,15 +3,11 @@ package test;
 class MultilineStringConstant {
 
     // NON-NLS comments are required for i18n, it's important they are kept with their strings.
-    private static final String MULTIPLE_LINE_NON_NLS = "field_0,"
-            + //$NON-NLS-1$
-            "field_1,"
-            + //$NON-NLS-1$
-            "field_2,"
-            + //$NON-NLS-1$
-            "field_3,"
-            + //$NON-NLS-1$
-            "field_4"; //$NON-NLS-1$
+    private static final String MULTIPLE_LINE_NON_NLS = "field_0," //$NON-NLS-1$
+            + "field_1," //$NON-NLS-1$
+            + "field_2," //$NON-NLS-1$
+            + "field_3," //$NON-NLS-1$
+            + "field_4"; //$NON-NLS-1$
 
     private static final String MULTIPLE_LINE_NO_COMMENT =
             "field_0," + "field_1," + "field_2," + "field_3," + "field_4";