Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 346959

Summary: [1.7][compiler] Avoid type variables bound re-initialization in Scope.getStaticFactory(..)
Product: [Eclipse Project] JDT Reporter: Ayushman Jain <amj87.iitr>
Component: CoreAssignee: Ayushman Jain <amj87.iitr>
Status: VERIFIED WONTFIX QA Contact:
Severity: normal    
Priority: P3 CC: srikanth_sankaran
Version: 3.7   
Target Milestone: ---   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
proposed fix none

Description Ayushman Jain CLA 2011-05-24 06:25:27 EDT 
BETA_JAVA7

The code in Scope.getStaticFactory(..), lines 4091 onwards initializes the bounds and performs the necessary substitution on the dummy type arguments created for the static factory. i.e.
if static X<T'> <factory>(T' t) is the method created corresponding to a constrcutor X(T t) in a type X<T extends String>, it makes sure that superclass of T' is also String and similarly for any superinterfaces. Now when the method binding corresponding to a static factory method is created successfully, we attempt to create a ParameterizedMethodBinding from it at line 4132. Again in the Ctor of PMBinding we repeat the process that we just did above i.e. making sure the bounds of each type arg are correct and the bounds are also substituted. 

This can be bypassed.
Comment 1 Ayushman Jain CLA 2011-05-24 07:05:21 EDT 
Created attachment 196424 [details]
proposed fix

The above patch bypasses the re-substitution in the ParameterizedMethodBinding constructor.
However, this itself is not sufficient. Since we initialize the type arguments of the new binding to those coming from the original method binding, we need to readjust their declaring element to the ParameterizedMethodBinding being created. This is also handled by the patch.
Comment 2 Ayushman Jain CLA 2011-05-24 07:06:09 EDT 
Srikanth, please review. Thanks!
Comment 3 Srikanth Sankaran CLA 2011-05-26 07:36:35 EDT 
I reviewed this change and despite this change being valid & functional,
I am suggesting we close the issue at hand as WONT_FIX.

First of all, there is the question of motivation - what are doing this for:
Is it to 

(a) Address performance issues ? it is not as though we know this redundant
renaming is causing any performance issues. It is extremely unlikely to.

(b) Minimize code clutter ? If so, there is (marginally) more clutter now
than without the fix - (it is not as though either one of the two chunks
of codes has been somehow gotten rid of and there is all the further parameter
passing, delegation, short circuiting etc) - It is not easy for the code
that synthesizes the static factories to somehow push the substitution down
into the constructor of the ParameterizedMethodBinding, nor can the latter
push it up to the former. Each has its own needs to have this code
in place.  

(c) Improve code readability ? It would be a valid, worthy and noble goal
to do something, anything at all that would avoid a future reader of this
code from scratching his/her head over this piece of code. But I think
this is better served by a comment in the code (possibly with a reference
to this bug)

In summary, I don't see the need for this change - it smells like a hack,
admittedly not particularly so.

Architecturally, the current code looks clean even with this redundancy.
getStaticFactory is a straight forward implementation of the relevant
section of the coins project specification and PMB's constructor is what
it has always been.

Now if the duplicate code could be refactored into method that could be
called from both places - that, coupled with some documentation explaining
what is happening there, would be the best solution IMO - because it would
address both (b) and (c) - but it is not worth the trouble though.
Comment 4 Ayushman Jain CLA 2011-05-26 11:04:36 EDT 
This was mostly because there was a redundancy and I thought we could probably do away with it. Wasn't so much a concern for performance as I don't think it really has too much of an effect.
Btw, are we sure that it won't have any side effects as such. I couldn't imagine a case where we would, but still would be good to be sure.
I'm not too inclined one way or the other, so we can close as WONTFIX.
Comment 5 Srikanth Sankaran CLA 2011-05-26 11:40:08 EDT 
(In reply to comment #4)

> Btw, are we sure that it won't have any side effects as such. I couldn't
> imagine a case where we would, but still would be good to be sure.

Side effect with the patch or without it ?
Comment 6 Ayushman Jain CLA 2011-05-26 11:44:24 EDT 
(In reply to comment #5)
> (In reply to comment #4)
> 
> > Btw, are we sure that it won't have any side effects as such. I couldn't
> > imagine a case where we would, but still would be good to be sure.
> 
> Side effect with the patch or without it ?

Without, if we close as WONTFIX.
Comment 7 Srikanth Sankaran CLA 2011-05-26 11:52:54 EDT 
(In reply to comment #6)

> Without, if we close as WONTFIX.

No, I can't think of any - From PMB's constructor's stand point, nothing
has changed. It is business as usual. The question then is whether the
code that synthesizes the factory leaves things in a state identical to
how things would have been if the factory were to have been declared by
the programmer. I believe this is the case. If evidence emerges to the
contrary, it calls for a fix at that point.
Comment 8 Ayushman Jain CLA 2011-05-26 11:55:55 EDT 
Ok. WONTFIX then!