eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
156 stars 123 forks source link

organize imports adding pointless imports #1589

Open hohwille opened 10 months ago

hohwille commented 10 months ago

I have a Junit5 test that extends some base test class and therefore indirectly extends org.assertj.core.api.Assertions. As a result, I can use assertThat methods directly without adding static imports. However, when I edit the test in Eclipse my save actions trigger formatter and organize imports that adds this line:

import static org.assertj.core.api.Assertions.assertThat;

At the same time Eclipse renders a compiler warning saying The import org.assertj.core.api.Assertions.assertThat is never used.

Behavior like this renders the very cool feature of save-actions into something rather annoying.

I get the error when editing this file: https://github.com/m-m-m/marshall/blob/d141e301031969f668153929b33b65823d071bd2/impl/protobuf/src/test/java/io/github/mmm/marshall/protobuf/ProtoBufFormatTest.java

I also tried to isolate the bug but was not lucky with a naive approach:

import org.assertj.core.api.Assertions;
public class ClassExtendingAssertions extends Assertions {
}

And now editing this file does NOT reproduce the bug as I would have expected:

import org.junit.jupiter.api.Test;
public class EclipseOrganizeImportsBug extends ClassExtendingAssertions {
  @Test
  public void test() {
    assertThat(Boolean.TRUE).isTrue();
  }
}

So its seems I am hitting an edge-case.

Tested with

Version: 2023-09 (4.29.0)
Build id: 20230907-1323

p.s.: IntelliJ actions on save feature is even worse in such case as you have no workaround. Eclipse is smart enough to save twice so invoking undo with [ctrl][z] will revert the modifications from save-actions. Also it seems that IntelliJ abandoned actions on save feature entirely in new versions. I love Eclipse save actions and in combination with devonfw-ide it is the excellent solution to ensure a consistent code-style even in large teams and to avoid diff-wars. I am still a big fan of Eclipse and using it since IBM Visual Age for Java times (>20 years). Thanks for making Eclipse and consider this bug report. I have always been pushing support for Eclipse as first class citizen in devonfw-ide and its successor IDEasy.

jukzi commented 9 months ago

@jjohnstn is "organize imports" core or ui?

jjohnstn commented 9 months ago

@jukzi OrganizeImportsOperation is in jdt.core.manipulation inside eclipse.jdt.ui. ImportRewrite and ImportRemover which is used under the covers is in jdt.core.

jukzi commented 9 months ago

@hohwille can you contribute a junit test for it?

hohwille commented 9 months ago

@hohwille can you contribute a junit test for it?

Are you talking of a JUnit source code file that can be used to reproduce the error manually in eclipse? Or do you mean a JUnit for Eclipse jdt core somehow triggering the formatter and revealing a bug by being red? In the latter case I would at least need a link to a suitable reference as I have no clue about the Eclipse code-base.

In both cases I still have the struggle that it is hard to isolate the bug to a single Java source file (my naive tries did not work) though I can somehow reproduce it in my OSS project.

jukzi commented 9 months ago

Or do you mean a JUnit for Eclipse jdt core somehow triggering the formatter and revealing a bug by being red?

yes, please

In the latter case I would at least need a link to a suitable reference as I have no clue about the Eclipse code-base.

based on comment https://github.com/eclipse-jdt/eclipse.jdt.core/issues/1589#issuecomment-1819472137 its most likely a bug in https://github.com/eclipse-jdt/eclipse.jdt.core/blob/master/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/rewrite/ImportRewrite.java

see also https://github.com/eclipse-jdt/.github/blob/main/CONTRIBUTING.md to get a start

hohwille commented 8 months ago

Or do you mean a JUnit for Eclipse jdt core somehow triggering the formatter and revealing a bug by being red?

yes, please

In the latter case I would at least need a link to a suitable reference as I have no clue about the Eclipse code-base.

based on comment #1589 (comment) its most likely a bug in https://github.com/eclipse-jdt/eclipse.jdt.core/blob/master/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/rewrite/ImportRewrite.java

see also https://github.com/eclipse-jdt/.github/blob/main/CONTRIBUTING.md to get a start

Thanks for your response and the links. I am used to maven or gradle projects that use JUnit for testing. In your code-base, I can not find a single JUnit and only proprietary test stuff, that I can not easily understand. To me it seems, I would need to spend weeks to setup a first automated test of that logic. Unless there is no existing example test, that I can easily run out of the box like in other state-of-the-art Java projects, I do not see that I can help here. Sorry.