Bug 340747

Comment 1 Srikanth Sankaran 2011-03-24 01:17:36 EDT

For the time being you may want to extract
the patch that does this from the early patches
posted to bug 339478.

I am not convinced we want to do this, but having
the patch should help if we decide to.

Comment 2 Ayushman Jain 2011-03-24 01:53:25 EDT

(In reply to comment #1)
> For the time being you may want to extract
> the patch that does this from the early patches
> posted to bug 339478.

I'll post a patch once bug 339478 is cleared. I cannot extract 2 independent patches right now.

Comment 3 Srikanth Sankaran 2011-04-18 08:42:52 EDT

(In reply to comment #0)
> I think once the diamond case is fully supported via bug 339478, it'll be good
> to have a an option to enable the user to be warned when he redundantly
> specifies type arguments when the diamond construct could've been used instead.

There is more to it than what meets the eye here. Compiler can infer different
types for <> than what the programmer intended and explicitly specified.
This can result in code that used to compile alright not compiling anymore, or
worse yet producing different results.

So if we were to implement this warning, we would have to first verify that
the type inferred for <> matches what the programmer has specified in the
first place.


class X<T> {
X(T x) {}
X<Number> x = new X<Number>(1);
}

is an example of a place where the 'Number' cannot be simply elided.

Comment 4 Srikanth Sankaran 2011-05-26 09:18:26 EDT

(In reply to comment #3)
> (In reply to comment #0)

> There is more to it than what meets the eye here. Compiler can infer different
> types for <> than what the programmer intended and explicitly specified.
> This can result in code that used to compile alright not compiling anymore, or
> worse yet producing different results.

On account if this, I suggest we close this issue as WONT_FIX.
If a user indeed wants to elide the types wherever possible,
some regular expression based text search may be handy enough.
(there is a proof in compiler land as to why regular expressions are
not good enough for describing pair-balanced syntactic structures,
but in practice, it should be handy enough)

Comment 5 Ayushman Jain 2011-05-26 10:10:38 EDT

Yes, I agree. I have more clarity on the spec now and I think that eliding type parameters where they have been used already might change the resulting type and we'll have to do an (probably) expensive comparison to determine if the two types are the same or not before we give out the warning.

Closing as WONTFIX.

Comment 6 Markus Keller 2011-07-05 14:36:32 EDT

Can we reopen this request? This would really be helpful to make existing code readable.

In practice, the type arguments are very often redundant, and people even use creator methods like this just to make the code manageable:

ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l= Lists.newArrayList();

The above is already way better than this:
ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
        new ArrayList<Map<String,PriorityBlockingQueue<Integer>>>();

The warning computation can start out very conservative (i.e. only show it if the actual type arguments exactly match the inferred ones). I think we can even relax this and also produce the warning if the actual type arguments are different, as long as
a) none of the constructor's parameter types refers to a type variable of the declaring type (comment 3), and
b) the constructor invocation is directly assigned to a variable.

This example would still produce the warning:
		Set<? extends Number> s= new HashSet<Integer>();

Comment 3, bug 349336 comment 5, and the following variant of the latter example would not produce the warning:

        X.testFunction(new X<>(null).getField()); // prints 2
        X.testFunction(new X<String>(null).getField()); // prints 1

Comment 7 Markus Keller 2011-07-06 06:18:07 EDT

Reopening as per bug 349336 comment 11. This would be nice to have for 3.7.1, but  can also be deferred to 3.8.

Comment 8 Dani Megert 2011-07-06 06:20:16 EDT

(In reply to comment #7)
> Reopening as per bug 349336 comment 11. This would be nice to have for 3.7.1,
> but  can also be deferred to 3.8.

+1.

Comment 9 Ayushman Jain 2011-07-09 13:24:22 EDT

Created attachment 199376 [details]
proposed fix v1.0 + regression tests

This patch tries to find out whether it is safe to remove the type args and use diamond instead, by removing the type args and passing the new elided type to the inference mechanism. If infered type is same as the one with type args, then the warning "Redundant declaration of type arguments <{0}>" is given. Two heuristics are applied to avoid the inference from being done in all case:
1) If the constructor parameters do not use any type variable of the class, and if an expected type is present to guide the inference, we straightaway report the warning, because removing the type args will not change the infered type.
This covers cases such as these
class X<T> {
X(String abc, String def) {}
void foo() {
        X<Integer> x = new X<Integer>("","");
}
X<Integer> foo2() {
        return new X<Integer>("","");
}
void foo3(X<Integer> x) {}
}

2) Else, if the constructor parameters are empty and the number of type parameters on LHS and RHS are same, we check if all type params are same, and if they are, we report the warning. This covers the very common case:
class X<T> {
void foo() {
        X<Integer> x = new X<Integer>();
        foo3(new X<Integer>("","");
}
X<Integer> foo2() {
        return new X<Integer>();
}
void foo3(X<Integer> x) {}
}

If the above 2 fail, we depend on our inference mechanism for the answer

Comment 10 Ayushman Jain 2011-07-09 13:33:20 EDT

Created attachment 199377 [details]
fix v1.0 corrected+ regression tests

The above patch had a few commented out lines left by mistake.

Comment 11 Ayushman Jain 2011-07-09 15:30:47 EDT

Srikanth, can you please review? Thanks!

Comment 12 Srikanth Sankaran 2011-07-11 05:20:37 EDT

(In reply to comment #9)

[...]

> Two
> heuristics are applied to avoid the inference from being done in all case:
> 1) If the constructor parameters do not use any type variable of the class, and
> if an expected type is present to guide the inference, we straightaway report
> the warning, because removing the type args will not change the infered type.
> This covers cases such as these

(1) I would keep this part simple and get rid of all these heuristics driven
special case code. we don't know that we have a performance issue here, so
why optimize prematurely ? This inference mechanism is too complicated and
runs to several pages of dense prose.

The way this is implemented is simplistic and will miss cases such as:

import java.util.ArrayList;
import java.util.List;

public class X<T> {
	X(List<? extends T> p) {
	}
	Object x = new X<CharSequence>((ArrayList<String>) null); 
}


We report the new diagnostic here. However making it a diamond will
infer String instead of CharSequence. This is admittedly a contrived
case, but it shows the problem alright.

(2) checkTypeParameterRequired is better named checkTypeArgumentRedundancy.

(3) RedundantDeclarationOfTypeArguments - should be redundant specification
 not declaration.

(4) We issue the message: Redundant declaration of type arguments <String>
but high light the generic type instead of the redundant arguments.

(5) In checkTypeParameterRequired, we really don't need the locals 
allocTypeWithDiamond and flag ?

Comment 13 Ayushman Jain 2011-07-12 07:10:31 EDT

Created attachment 199483 [details]
proposed fix v1.1 + regression tests

Taken care of above comments. Removed one hueristic but still preserved the one which deals with the case of no constructor parameters.

Also added another test to capture the example given in the above comment.
Srikanth, please take a final look at the patch and I'll release it. Thanks!

Comment 14 Srikanth Sankaran 2011-07-12 07:27:14 EDT

Looks good to me.

Comment 15 Ayushman Jain 2011-07-13 04:22:03 EDT

Created attachment 199547 [details]
proposed fix v1.2 + regression tests

Small update in error reporting

Comment 16 Ayushman Jain 2011-07-13 04:29:31 EDT

Released in BETA_JAVA7 branch

Comment 17 Raksha Vasisht 2011-07-19 06:07:00 EDT

In this case:

ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
        new ArrayList<Map<String,PriorityBlockingQueue<Integer>>>();

after applying the quick fix it results in 

ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
	        new ArrayList<Map<>>();

which seems wrong and shows errors. Shouldn't it result in this instead? :

ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
	        new ArrayList<>();(In reply to comment #12)


> (In reply to comment #9)

> (4) We issue the message: Redundant declaration of type arguments <String>
> but high light the generic type instead of the redundant arguments.

I feel we should highlight the generic type too, but it currently still highlights the arguments.

Comment 18 Deepak Azad 2011-07-19 06:31:25 EDT

(In reply to comment #17)
> In this case:
> 
> ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
>         new ArrayList<Map<String,PriorityBlockingQueue<Integer>>>();
> 
> after applying the quick fix it results in 
> 
> ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
>             new ArrayList<Map<>>();
> 
> which seems wrong and shows errors. Shouldn't it result in this instead? :
> 
> ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
>             new ArrayList<>();(In reply to comment #12)
The problem is with the quick fix, not the warning. The quick fix is bug 349336.
 
> I feel we should highlight the generic type too, but it currently still
> highlights the arguments.
The type arguments are redundant hence they are marked, so this is correct.

Comment 19 Raksha Vasisht 2011-07-20 02:34:40 EDT

(In reply to comment #18)
> (In reply to comment #17)
> > In this case:
> > 
> > ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
> >         new ArrayList<Map<String,PriorityBlockingQueue<Integer>>>();
> > 
> > after applying the quick fix it results in 
> > 
> > ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
> >             new ArrayList<Map<>>();
> > 
> > which seems wrong and shows errors. Shouldn't it result in this instead? :
> > 
> > ArrayList<Map<String, PriorityBlockingQueue<Integer>>> l2=
> >             new ArrayList<>();(In reply to comment #12)
> The problem is with the quick fix, not the warning. The quick fix is bug #
> 349336.
Yep! Used to verifying UI bugs :) Thanks for updating the bug. Marking this verified.