Enforcing a space in one-line comment breaks //$NON-NLS-x$ notation

[romp13]

Hi,
Since version 1.4 a new rule formats "//comment" into "// comment". Unfortunately Eclipse uses "//$NON-NLS-x$" comments to avoid internationalization warnings. If you add the space it does not work anymore. We have these all over the place in our codebase which prevents us of using versions of google-java-format later than 1.3.
Would it be possible to handle this case? (maybe a solution is to check if the first character is alpha-numeric)
Alternatively would there be a workaround for us with a setting.
Thanks!

[briandealwis]

By way of background, the Eclipse Compiler for Java (ecj) can be configured to warn about non-externalized strings for internationalization. The warnings can be suppressed by annotating strings with comments like //$NON-NLS-x$ to indicate that the xth string on the line is not meant to be externalized (such as an identifier).

An alternative approach to #222 would be to special case $NON-NLS-x$ similar to the noinspection that's already supported:

private static final Pattern LINE_COMMENT_MISSING_SPACE_PREFIX =
      Pattern.compile("^(//+)(?!noinspection|\\$NON-NLS-\\d+\\$)[^\\s/]");

However there's one additional difference in that the //$NON-NLS-x$ should be kept with the string, and it shouldn't wrap even if it extends beyond the line-length. But perhaps that should just be punted to the developers to manage (e.g., split long strings).

[romp13]

The line wrapping is actually a complicated issue to handle in google-java-format. The Eclipse formatter is actually doing this properly. It's been a while this I reported this issue without answers, so I went ahead and implemented the only way I could think of:

• preprocess the text by recording all litterals and whether they have the //$NON-NLS-x$ associated
• remove all //$NON-NLS-x$ annotations
• format as usual
• re-inject //$NON-NLS-x$ annotations to the litteral at their new positions and with the correct number x

This works like a charm but this is quite a big change, so I did not make a PR for it. I think @briandealwis or my initial solution is a fine, simple solution which covers the non-wrapping cases.
For reference you can have a look at this branch: https://github.com/romp13/google-java-format/tree/eclipse-non-nls-support (1st commit is the feature, 2nd is improving the Eclipse plugin to use it and also automatically reorder imports, 3rd commit can be ignored). If the maintainer of this repo is interested, let me know and I can make a PR.

[cushon]

I understand that reformatting e.g. //$NON-NLS-1$ to // $NON-NLS-1$ (with a space after the //) is an issue for eclipse, and I think special-casing these comments similar to //noinspection is probably OK.

I'm not sure I understand the line wrapping issue, though: google-java-format doesn't currently reflow long string literals, or move line comments off very long lines.

e.g. this is not currently wrapped:

class T {
  private String s =
      "long looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong string"; // $NON-NLS-1$
}

Do you have an example of a //$NON-NLS-x$ comment where line wrapping is an issue?

[briandealwis]

We'll see it where the string is an argument followed by another argument. The following snippet

    } catch (CoreException ex) {
      logger.log(Level.WARNING, "Unable to obtain jst.web facet version from selected project", ex); //$NON-NLS-1$
    }

is line wrapped to

    } catch (CoreException ex) {
      logger.log(
          Level.WARNING,
          "Unable to obtain jst.web facet version from selected project",
          ex); // $NON-NLS-1$
    }

The Eclipse formatter will move the //$NON-NLS-1$ to stay the string.

[cushon]

Thanks, is it an option to write that code like this instead?

    } catch (CoreException ex) {
      logger.log(
          Level.WARNING,
          "Unable to obtain jst.web facet version from selected project", //$NON-NLS-1$
          ex);
    }

[briandealwis]

Sorry, I should have said that: it would be extraordinarily helpful for the formatter to do it automatically, but manually moving tags is perfectly acceptable.

[cushon]

Thanks for clarifying. I agree it would be nice if the formatter could do that automatically, but I'd prefer to avoid adding a lot of logic to the formatter to handle this specific case.

It sounds like not adding the space to //$NON-NLS-1$ comments (like we already do for //noinspection) is a step in the right direction, even if it doesn't completely solve the problem and occasionally you have to manually move a tag?

[briandealwis]

Yup, that would be great. Eclipse can be configured to flag strings missing a //$NON-NLS-x$ so the misattributed tag will be immediately apparent to the developer.

[briandealwis]

Unfortunately this fix is insufficient. Consider the following sample class:

public class ExampleClass {
  public Object resolveClasspathContainer() {
    System.out.printf(
        "This is an expression %s\n", //$NON-NLS-1$
        "value"); //$NON-NLS-2$
    System.out.println("statement"); //$NON-NLS-3$
    Job resolveJob = new Job("Resolving libraries for " + webFacetVersion) { //$NON-NLS-4$
          @Override
          protected IStatus run(IProgressMonitor monitor) {}
        };
    return null;
  }
}

The formatting of //$NON-NLS-3$ and -4 are not preserved:

@@ -3,8 +3,8 @@
     System.out.printf(
         "This is an expression %s\n", //$NON-NLS-1$
         "value"); //$NON-NLS-2$
-    System.out.println("statement"); //$NON-NLS-3$
-    Job resolveJob = new Job("Resolving libraries for " + webFacetVersion) { //$NON-NLS-4$
+    System.out.println("statement"); // $NON-NLS-3$
+    Job resolveJob = new Job("Resolving libraries for " + webFacetVersion) { // $NON-NLS-4$
           @Override
           protected IStatus run(IProgressMonitor monitor) {}
         };

[romp13]

I am a bit puzzled by your example because the number in the non-nls comment should correspond to the literal's number on the line. If you copy this code in Eclipse you will get warnings for invalid non-nls comments.
I believe the code get split into multiple lines first and if you read this thread entirely you will understand that this is not covered by the fix. But there is maybe something else. Could you please post the exact code before formatting and after?

[briandealwis]

Sorry I should have made that clear: the numbers are purely for referencing purposes.