Bug 559146

Comment 1 Duncan Gittins 2020-11-06 13:36:11 EST

Another "Remove unnecessary array creation" save action which introduces runtime-errors:

    String [] args = new String[] {"a","b","c"};
    Class<?> runClass = Class.forName("some.ClassWithMainMethod");
    Class<?>[] mainArgTypes = new Class<?>[] { args.getClass() };
    Method method = runClass.getMethod("main", mainArgTypes);
    method.invoke(null, new Object[] { args });

Remove unnecessary array creation converts the last line to:

    method.invoke(null, args);

The new code does not run, throwing: 

    java.lang.IllegalArgumentException: argument type mismatch
    
Eclipse also reports the above change with a warning:

Type String[] of the last argument to method invoke(Object, Object...) doesn't exactly match the vararg parameter type. 
Cast to Object[] to confirm the non-varargs invocation, or pass individual arguments of type Object for a varargs invocation.

Comment 2 Julien HENRY 2021-03-18 13:40:43 EDT

Another case where this refactoring can produce bugs:

public class Example {

  public void doSomething(String a) {
    // do something with a
  }

  public void doSomething(String... args) {
    // do something else with args
  }

}

Call to 
  doSomething(new String[]{"foo"}) 
is sometimes automatically changed to
  doSomething("foo") 

but then it means that a different method is called.

Note that I tried to create a minimal reproducer but I was not able. Seems Eclipse is sometimes able to "see" there is another method that will conflict.
In my real life case, the calling code was in JUnit tests:
https://github.com/SonarSource/sonarlint-core/commit/25feb87f7928cf47931f3bf55b8d1bb2bca2fc0f#diff-e4793d82783f725b109570facf25aaf9065ddc88e6c713251d188eaa78b33f0fR60

Comment 3 Carsten Hammer 2021-03-18 14:01:29 EDT

(In reply to Julien HENRY from comment #2)
> Another case where this refactoring can produce bugs:
> 
> public class Example {
> 
>   public void doSomething(String a) {
>     // do something with a
>   }
> 
>   public void doSomething(String... args) {
>     // do something else with args
>   }
> 
> }
> 
> Call to 
>   doSomething(new String[]{"foo"}) 
> is sometimes automatically changed to
>   doSomething("foo") 
> 
> but then it means that a different method is called.

This sounds similar to Bug 564983. Which version do you use?

Comment 4 Manoj N Palat 2021-03-19 01:07:41 EDT

Since refactoring starts with jdt.ui, moving to jdt.ui for the initial investigation.

Comment 5 Julien HENRY 2021-03-19 06:37:15 EDT

(In reply to Carsten Hammer from comment #3)
> (In reply to Julien HENRY from comment #2)
> 
> This sounds similar to Bug 564983. Which version do you use?

I can reproduce on Eclipse 2021-03 (4.19)

Comment 6 Jeff Johnston 2021-03-19 14:27:57 EDT

(In reply to Benjamin Heidelmeier from comment #0)
> Applying the save action "Remove unnecessary array creation" to the
> following code has two major flaws:
> 
> 1. Comments are removed
> 2. Code errors are introduced
> 
> import java.util.Arrays;
> import java.util.Collection;
> 
> public class Example {
>   public static Collection<Object[]> data() {
>     return Arrays.asList(new Object[][] { //
>         // comment 1
> 
>         // comment 2
>         { "string1", // comment 3
>             "", 1 }, // comment 4
>         // comment 5
>         { "string2", // comment 6
>             "", 1 } });
>   }
> }

Let me start with this one.  Using latest Eclipse, this does not fail.  A fix has already been made to not handle multi-dimensional arrays.

That said, comments on separate lines for the parameters are not preserved for a legal modification.

Comment 7 Jeff Johnston 2021-03-19 14:40:52 EDT

(In reply to Julien HENRY from comment #5)
> (In reply to Carsten Hammer from comment #3)
> > (In reply to Julien HENRY from comment #2)
> > 
> > This sounds similar to Bug 564983. Which version do you use?
> 
> I can reproduce on Eclipse 2021-03 (4.19)

The test given above is handled, the following taken from your actual code issue does reproduce the problem:

public class ArrayProblem {
    
    void doError(String msg, Object arg) {
    }

    void doError(String msg, Object arg1, Object arg2) {
    }

    void doError(String msg, Object... args) {
    }

    public void foo() {
        doError("a", new Object[] {"b"});
    }
}

Comment 8 Jeff Johnston 2021-03-19 19:13:14 EDT

(In reply to Duncan Gittins from comment #1)
> Another "Remove unnecessary array creation" save action which introduces
> runtime-errors:
> 
>     String [] args = new String[] {"a","b","c"};
>     Class<?> runClass = Class.forName("some.ClassWithMainMethod");
>     Class<?>[] mainArgTypes = new Class<?>[] { args.getClass() };
>     Method method = runClass.getMethod("main", mainArgTypes);
>     method.invoke(null, new Object[] { args });
> 
> Remove unnecessary array creation converts the last line to:
> 
>     method.invoke(null, args);
> 
> The new code does not run, throwing: 
> 
>     java.lang.IllegalArgumentException: argument type mismatch
>     
> Eclipse also reports the above change with a warning:
> 
> Type String[] of the last argument to method invoke(Object, Object...)
> doesn't exactly match the vararg parameter type. 
> Cast to Object[] to confirm the non-varargs invocation, or pass individual
> arguments of type Object for a varargs invocation.

This too has been fixed prior to 2021-03.  The code does not clean-up if the first element is an array.

Comment 9 Jeff Johnston 2021-03-19 19:16:01 EDT

(In reply to Julien HENRY from comment #5)
> (In reply to Carsten Hammer from comment #3)
> > (In reply to Julien HENRY from comment #2)
> > 
> > This sounds similar to Bug 564983. Which version do you use?
> 
> I can reproduce on Eclipse 2021-03 (4.19)

Hi Julien, as mentioned, I have found how to reproduce this problem which has to do with package-private modifiers being ignored (also private in same class).  I have created a new bug:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=572131

as this bug will concentrate on the reporter's issue of comments being removed since the actual given scenario is handled properly, but a valid scenario with line comments for arguments does remove them.

Comment 10 Eclipse Genie 2023-03-27 18:07:31 EDT

This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.

Comment 11 Jeff Johnston 2023-03-27 19:36:56 EDT

Closing this as fixed.