Bug 349336

Comment 1 Deepak Azad 2011-06-21 09:15:53 EDT

This is not trivial, a few examples where the type of the created object changes or a compilation error occurs if the explicit type parameters are removed.

-------------------------------------------------------------------------
package p;
class A {}
class B extends A {}

public class X<T> {

	X() {}
	X(T s) {}

	X<? extends A> f1 = new X<A>(); // ok to remove
	X<? extends A> f2 = new X<B>(); // NOT ok to remove
	X<? extends A> f3 = new X<A>(new B()); // NOT ok to remove
	
	X<? super B> f4 = new X<A>(); // NOT ok to remove
	X<? super B> f5 = new X<B>(); // ok to remove
	X<? super A> f6 = new X<A>(new B()); // NOT ok to remove
	X<? super B> f7 = new X<A>(new B()); // NOT ok to remove
	X<? super B> f8 = new X<A>(new A()); // ok to remove
	
	X<?> f9 = new X<A>(new B()); // NOT ok to remove
}
------------------------------------------------------------------------

Probably a good solution would be to NOT offer the quick assist if the LHS contains wildcards, as in these cases the code would be more readable if explicit type arguments are present on the RHS.

Comment 2 Deepak Azad 2011-06-22 01:50:20 EDT

Satyam pointed out this morning that the type information is used only at compile time, and is missing at run time. Hence if the compiler does not complain, it might be ok to remove the type arguments. Having said that I am not comfortable with the idea of silently changing the type bindings.

(In reply to comment #1)
>     X<? super A> f6 = new X<A>(new B()); // NOT ok to remove
In this case removing the type arguments from RHS results in a compile error, in all other cases from comment 1 only the type bindings change.

Comment 3 Srikanth Sankaran 2011-06-22 02:24:49 EDT

See also bug 340747

Comment 4 Satyam Kandula 2011-06-22 03:02:57 EDT

Srikanth, 
Assuming that replacing the type arguments with <> doesn't result into a compile error, is it safe to assume that there is no semantic change?

Comment 5 Srikanth Sankaran 2011-06-22 03:58:19 EDT

(In reply to comment #4)
> Srikanth, 
> Assuming that replacing the type arguments with <> doesn't result into a
> compile error, is it safe to assume that there is no semantic change?

No, it is not. Here is a test case that shows program output differing
(no compilation error) when type arguments are replaced with <>:
(this is a slightly modified version of org.eclipse.jdt.core.tests.compiler.regression.GenericsRegressionTest_1_7.test007())

public class X<T> {
	T field1;
	public X(T param){
		field1 = param;
	}
	public static void main(String[] args) {
		X.testFunction(new X<Object>("hello").getField()); //prints 2
		X.testFunction(new X<>("hello").getField()); // prints 1
	}
	public static void testFunction(String param){
		System.out.println(1);
	}
	public static void testFunction(Object param){
		System.out.println(2);
	}
	public T getField(){
		return field1;
	}
}

So eliding the <Object> to <> will change the program behavior while still
resulting in a valid program.

Which is why as https://bugs.eclipse.org/bugs/show_bug.cgi?id=340747#c3 points
out such eliding can be undertaken only after first inferring and confirming
that compiler's inferred type would match the explicit type provided by the
programmer.

Comment 6 Srikanth Sankaran 2011-06-22 04:01:01 EDT

(In reply to comment #1)

> Probably a good solution would be to NOT offer the quick assist if the LHS
> contains wildcards, as in these cases the code would be more readable if
> explicit type arguments are present on the RHS.

Per comment#5 of current bug and also bug 340747 comment c3, there are some
tricky issues even when wildcards are not in the picture.

Comment 7 Deepak Azad 2011-06-22 04:26:29 EDT

Thanks Srikanth!

The example from comment 5 provides greater clarity. Essentially in "new X<A>(new B())" if A is supertype of B, the type arguments cannot be removed. (This construct is present in 3 cases from comment 1)

Comment 8 Ayushman Jain 2011-07-04 02:31:26 EDT

A cleanup, IMHO, is not a very good option here. Java7's motivation of providing the diamond construct is to make sure that the user can avoid "writing" repeatedly, the type args, or omitting them where they aren't really needed. The need for diamond ends here, and does not give any particular benefit to the user to clean up the explicit type arg declaration when they're already present in the code. 
Even prior to 1.7, we could elide type args for a generic method call, but we never had either a clean up or a quick fix for it. Then why now?

As we can see in examples above, it is much more obvious to a reader of the code what type is actually used when type args are explicitly specified. Diamond is a rather confusing construct, where a reader will have to summon up some blood and gather some grey cells to sit and manually decipher the inferred type args.

Bug 351048 may be a step towards making that simpler. Once that is done, we can just propose a quick fix for every generic type allocation expression, and the user can inspect the inferred type now when the type args have been deleted. If there's a mismatch, CTRL+Z to the rescue!

Comment 9 Deepak Azad 2011-07-04 02:42:32 EDT

(In reply to comment #8)
> A cleanup, IMHO, is not a very good option here. Java7's motivation of
> providing the diamond construct is to make sure that the user can avoid
> "writing" repeatedly, the type args, or omitting them where they aren't really
> needed. The need for diamond ends here, and does not give any particular
> benefit to the user to clean up the explicit type arg declaration when they're
> already present in the code. 

- Content assist helps you with 'writing' new code, so you do not really have to write the type arguments
- 'Reading' code is an important activity which could become easier in many cases with use of diamond. I agree that there are cases where it is better to have explicit type arguments, but these are rare. For a majority of cases I think it is better to use diamond.

Comment 10 Markus Keller 2011-07-05 14:49:04 EDT

We shouldn't start doing expensive what-if analyses in quick assists / clean ups. Waiting for bug 340747.

Nevertheless, I disagree with comment 8. The only reason for adding the diamond feature to Java 7 was to make the uninteresting code bloat go away. We should support that goal as well as we can.

Yes, there are cases where the compiler behavior may be hard to understand, but in the majority of practical cases, the type arguments just don't matter.

Comment 11 Srikanth Sankaran 2011-07-06 00:34:21 EDT

(In reply to comment #10)
> We shouldn't start doing expensive what-if analyses in quick assists / clean
> ups. Waiting for bug 340747.

If after due consideration, the UI team feels a strong need for this, please
feel free to reopen bug 340747 -- Thanks.

Comment 12 Markus Keller 2011-07-13 06:07:59 EDT

We should start with a quick fix for BETA_JAVA7 and defer the cleanup to 3.8 (too risky at this point, since the new warning hasn't been widely tested).

A quick assist would need to create another AST where the compiler option is enabled. We could do that for 3.8, but only after a few cheap checks to narrow down the cases where the second AST is needed. Also, the second AST should use a focal position then.

Comment 13 Deepak Azad 2011-07-13 07:50:26 EDT

Created attachment 199569 [details]
fix + tests

Comment 14 Deepak Azad 2011-07-13 07:50:47 EDT

Fixed in BETA_JAVA7.

Comment 15 Markus Keller 2011-07-13 08:50:08 EDT

What's up with the fully-qualified org.eclipse.jdt.core.dom.ParameterizedType references?

Comment 16 Deepak Azad 2011-07-13 09:11:19 EDT

(In reply to comment #15)
> What's up with the fully-qualified org.eclipse.jdt.core.dom.ParameterizedType
> references?
Looks like I need some coffee. I was thrown off by *java.lang*.reflect.ParameterizedType. 

Anyway, fixed it now.

Comment 17 Deepak Azad 2011-07-19 06:37:16 EDT

The quick fix does not work correctly for the following
 
 ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
         new ArrayList<Map<String,PriorityBlockingQueue<Integer>>>();

Comment 18 Deepak Azad 2011-07-21 05:30:58 EDT

Bug 352699 takes care of problem mentioned in comment 17. I have added 2 more tests to LocalCorrectionsQuickFixTest17.

Comment 19 Dani Megert 2011-08-03 05:26:28 EDT

Verified in I20110729-1200 and M20110729-1400.

Comment 20 Andrey Loskutov 2012-10-16 05:45:50 EDT

(In reply to comment #12)
> We should start with a quick fix for BETA_JAVA7 and defer the cleanup to 3.8
> (too risky at this point, since the new warning hasn't been widely tested).

I've created bug 392040 for this.

Comment 21 Deepak Azad 2013-09-15 19:23:59 EDT

Pushed branch - http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/log/?h=dazad/bug-351956-Suggest-to-use-diamond-where-applicable

I added the cleanups to Unnecessary Code tab, but that can be changed easily if required.

Comment 22 Deepak Azad 2013-09-15 19:25:50 EDT

(In reply to Deepak Azad from comment #21)
Oops.. wrong bug.