"Organize imports" should never delete comments

[fbricon]

context: when using JBang, comments are used to declare several directives, like classpath dependencies. Those comments are critical to the proper functioning of JBang.

In certain conditions, calling "organize imports" will delete all comments above removed imports.

eg. with the following code:

///usr/bin/env jbang "$0" "$@" ; exit $?
package foo;

//DESCRIPTION all those comments are JBang Directives. organizing imports should never ever remove them!!!
//DEPS com.github.jamesnetherton:lolcat4j:0.4.0
//DEPS com.github.lalyos:jfiglet:0.0.9
import static java.lang.System.*;

public class OrganizeImportsBug {
    public static void main(String[] args) {
        System.out.println("Nooope");
    }
}

invoking "Organize imports" will delete all comments below the first non-comment line (the package statement), and the now removed import static java.lang.System.*;, producing:

///usr/bin/env jbang "$0" "$@" ; exit $?
package foo;

public class OrganizeImportsBug {
    public static void main(String[] args) {
        System.out.println("Nooope");
    }
}

With JBang Eclipse installed, this would mean the dependencies would be automatically removed from the classpath, causing compilation failures on a file actually using classes from those dependencies. Bad JDT!

Same bug can be observed when running in VS Code with jbang-vscode.

cc @maxandersen @rgrunber @jjohnstn

[rgrunber]

There's a good chance this is a bug in the AST rewriting mechanism. I remember for a lot of rewrites, if the "extended" source range wasn't used, comments might be ignored/removed.

[jjohnstn]

This is a common problem. JDT treats comments that precede an element and line comments on the same line as being owned by the element. This is reasonable considering many comments apply to the next line. When removed, the comments belonging to an element are removed as well. Obviously, there are times when this approach is wrong and the comment is a global comment or describing logic flow for more than just the next line. Looking at the Organize imports code, it is purposely treating the comments preceding the import statements as belonging to the first import statement just as it would consider comments between imports as belonging to the next import statement.

For example:

// import a list
import java.util.List; // added on Mar 3/1999
// Import an ArrayList
import java.util.ArrayList; // added on Mar 4/1999

public class A {
   public void foo(List<String> x) {
  }
}

Invoking Organize Imports on the sample above will remove the ArrayList import plus all its comments and leave the comments for List. If List were also not used, then its comments would be removed as well. Interestingly enough, if the threshold for using a . import is reached, it leaves all comments in place for the items that are merged and adds the new . (e.g. import java.util.*;).

Is there any reason the directive comments aren't all grouped together above the package statement? (i.e. are they supposed to be below the package statement)?

One idea might be to add a specific preference to clarify what is desired: e.g. an option in the Organize Imports preferences to ignore leading comments before the first import statement. If you expect such comments sprinkled throughout the code to be preserved a much more complex option would be to needed to add filters for comments to ignore and perhaps a list for shebang could be contributed for the user to select by default.

[rgrunber]

Is there any reason the directive comments aren't all grouped together above the package statement? (i.e. are they supposed to be below the package statement)?

Because of https://github.com/eclipse-jdt/eclipse.jdt.core/issues/649 :stuck_out_tongue:

One idea might be to add a specific preference to clarify what is desired: e.g. an option in the Organize Imports preferences to ignore leading comments before the first import statement. If you expect such comments sprinkled throughout the code to be preserved a much more complex option would be to needed to add filters for comments to ignore and perhaps a list for shebang could be contributed for the user to select by default.

So maybe a formatting option of some sort, which is what I think Fred implied there.

[fbricon]

This is a common problem. JDT treats comments that precede an element and line comments on the same line as being owned by the element. This is reasonable considering many comments apply to the next line. When removed, the comments belonging to an element are removed as well. Obviously, there are times when this approach is wrong and the comment is a global comment or describing logic flow for more than just the next line. Looking at the Organize imports code, it is purposely treating the comments preceding the import statements as belonging to the first import statement just as it would consider comments between imports as belonging to the next import statement.

For example:

// import a list
import java.util.List; // added on Mar 3/1999
// Import an ArrayList
import java.util.ArrayList; // added on Mar 4/1999

public class A {
   public void foo(List<String> x) {
  }
}

Invoking Organize Imports on the sample above will remove the ArrayList import plus all its comments and leave the comments for List. If List were also not used, then its comments would be removed as well.

No, even that case is inconsistent. If there are no statement above, the comments are not removed. If you try with

// import a list
import java.util.List; // added on Mar 3/1999
// Import an ArrayList
import java.util.ArrayList; // added on Mar 4/1999

public class A {
   public void foo() {
  }
}

you end up with

// import a list

public class A {
   public void foo() {
  }
}

[jjohnstn]

No, even that case is inconsistent. If there are no statement above, the comments are not removed. If you try with

// import a list
import java.util.List; // added on Mar 3/1999
// Import an ArrayList
import java.util.ArrayList; // added on Mar 4/1999

public class A {
   public void foo() {
  }
}

you end up with

// import a list

public class A {
   public void foo() {
  }
}

This is intentional. The first comment is being considered the file header and there was a bug opened because it was removed if the imports were removed: https://bugs.eclipse.org/121428

With Roland's patch, adding back the imports will properly treat the first comments as being the file header.

With a package statement in the file, the comments are treated as belonging to the first import statement. If it is deleted, they are deleted.

I am currently working on adding the package statement and not affecting the header comments.