Community
Participate
Working Groups
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. eg. ArrayList<String> a = new ArrayList<String>(); ^^^^^^^^^^^^^^^^^^^ will warn "Redundant declaration of type arguments <String>
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.
(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.
(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.
(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)
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.
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
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.
(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.
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
Created attachment 199377 [details] fix v1.0 corrected+ regression tests The above patch had a few commented out lines left by mistake.
Srikanth, can you please review? Thanks!
(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 ?
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!
Looks good to me.
Created attachment 199547 [details] proposed fix v1.2 + regression tests Small update in error reporting
Released in BETA_JAVA7 branch
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.
(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.
(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.
