Quickfix/cleanup removes //$NON-NLS-1$ about eclipse.jdt.ui

[jjohnstn]

Hi Thomas, can you take a look at this?

[tsmaeder]

@vik-chand which cleanup and/or quick fix is this? Instructions to reproduce would be much appreciated.

[tsmaeder]

There are a couple of factors to be considered here:

1. The "non-nls" warning is not a Java error, instead it's a convention introduced by the Eclipse source code itself
2. The problem is configurable and might be turned off. In this case I would consider it wrong to introduce "$NON-NLS" tags.
3. There might be tagged strings outside of the initializer being moved both in the origin as well as the target line. Both existing and moved tag numbers will have to be updated. Consider this case:

   String bla= "gork";File f= new File("abc"); String zoz= "zoz"; //$NON-NLS3
   f= new File("foo" + "bar"); //$NON-NLS2
   

4. If tags exist, they should be moved, even if omitting them would not introduce an error (as in case 2.)

[ktatavarthi]

We also would need to consider that the cleanup/quickfix should also not add extra warnings.

[jjohnstn]

The NLS comment should not be removed in the simple example originally given in the issue. You can reconstruct the comment if you create a brand new statement via ASTRewrite.createStringPlaceHolder and cast it to be a line comment. If there is some really complex situation you can think of where the cleanup won't know what to do or the code will be overly complex, simply bail on the cleanup (e.g. don't handle any VarDeclarationStatement with multiple fragments, don't handle multiple statements on a single line, etc..). There should be few of these complex scenarios in the wild but the first scenario is very common.

[tsmaeder]

@jjohnstn a problem I'm running into is that the AST simply does not seem to have any line information: if there already is a line comment on the declaration line, how do I even match that to the var initializer I'm replacing? NLS processing is based on TextEdits and lines, not an AST.

[tsmaeder]

Btw: I think even before I started working on this, the cleanup could have introduced warnings. Consider this:

var foo= new File("abcdef"); //$NON-NLS-1
foo= new File(this.currentFile);

That would have been rewritten to

var foo= new File(this.currentFile); //$NON-NLS-1

, which will generate a "unnessary non-nls tag" warning.

[jjohnstn]

Hi Thomas, there are two methods you can call: ASTNodes.getLeadingComments(node) and ASTNode.getTrailingComments(node).
From there, you can build a statement in a StringBuffer and prepend/append comments as you please. You can then cast that String into any node using ASTRewrite.createStringPlaceHolder(String s, int nodeType). You can see an example of this in: SwitchExpressionsFixCore.getNewStatementForCase(). In that example, I move some leading line comments into a switch case so line comments need to be converted to block comments so they don't obliterate the line.

[tsmaeder]

I've thought about this intensely, and handling this gets fiendishly difficult except for the most trivial case: where both the original and the overriden assignment sit alone on their own line. In any other case it's not even clear, where one would insert a line comment: from a syntactical point of view, comments are empty space, so you'd have to find an insert location that is between two ast nodes that are on the current and the next line. The location may also be the first child of an "upper level" node, etc.
As I said, this gets hairy real fast.

[jjohnstn]

Handle the cases you can. That is the vast majority of cases. Who declares a variable with an initializer, then changes its value on the same line AND has comments intertwined? If you don't add the NLS for the simple cases, this will break builds and create a lot of work for the end-user. If a user has some convoluted situation where the original and overridden are on the same line (easy to recognize) and there are comments, simply bail; don't do the cleanup.

[tsmaeder]

I'm not talking about strange cases, but simple ones like these:

String foo= "bar" + //$NON-NLS-1
           "zoz" + 
           "gorx"; // NON-NLS-1

or

var z= "borx"; var x= "foo";  // some comment, maybe for the first, maybe for the second var //$NON-NLS-1 //$NON-NLS-2
...
x= "bla";

or even worse:

if ("TRUE".equals(str)) { var x= "bar"; //$NON-NLS-1 //NON-NLS-2

Even just detecting that this last case is a problem is not trivial looking at the AST. There is a good reason the NLS fixes work on text edits, not AST manipulation. Mixing the two is hard.

That said, yes, I'll just bail on anything that is non-trivial. But if our aim is to not introduce any warnings/errors, we need to detect all the cases where we would introduce a problem, which itself is non-trivial.

[tsmaeder]

This is driving me nuts! So I've got a solution that would just move the line end comments: the problem is: line comments are not part of the AST, so you can't remove them from the AST either (throws NPE). So the only solution I can see would be to replace the parent node of the variable declarations (probably a Block) as text. But that does not seem like a good approach: we'd have to do the manipulation of the AST as well as text edits (instead of AST manipulation).
At this point, I would close this one as "won't fix", t.b.h.

[tsmaeder]

@jjohnstn I tried your idea with using string placeholders to replace the whole variable declaration statement. The problem I'm running into is in handling the leading comments. For example:

     /**
      * Some comment
      */
      String bla= "foo" //$NON-NLS-1

If I use a ASTRewrite.createStringPlaceholder(), I can either lost the leading comment or the leading indent of the comment gets scrambled in the result since string placeholders do not get formatted. Should I try to fix the indenting of leading comments myself or what's a promising approach here?

[tsmaeder]

Never mind, I found a way to ignore the leading comments using a custom source range computer.

[jjohnstn]

After looking at it, I think it is better to ensure that both the declaration and replacement lines don't share lines with other statements rather than restrict if they have multiple lines. It is fine to move up the case with:

int s = "some line"; //$NON-NLS-1$
s = "abc" + //$NON-NLS-1$
      "def" +
      "ghi"; //$NON-NLS-1$

as this doesn't cause any warnings that weren't already there. I have modified the code and added a new case to the test that verifies that movement doesn't occur when there are multiple statements on a line or multiple declarations on a line. I also modified to move preceding comments as well which is in the test as well.