Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 351444 - [1.7][content assist] Need to know whether I can use diamond
Summary: [1.7][content assist] Need to know whether I can use diamond
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: All All
: P2 normal (vote)
Target Milestone: 3.7.1   Edit
Assignee: Ayushman Jain CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 350559 (view as bug list)
Depends on:
Blocks: 349274
  Show dependency tree
 
Reported: 2011-07-07 09:28 EDT by Dani Megert CLA
Modified: 2011-08-05 02:52 EDT (History)
4 users (show)

See Also:
Olivier_Thomann: review+


Attachments
proposed fix v1.0 (7.88 KB, patch)
2011-07-11 14:24 EDT, Ayushman Jain CLA
no flags Details | Diff
proposed fix v2.0 + regression tests (24.75 KB, patch)
2011-07-12 14:23 EDT, Ayushman Jain CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2011-07-07 09:28:25 EDT
BETA_JAVA7.

For constructor invocation proposals I need to have a way to find out whether I can use the diamond or not, e.g. CompletionProposal.canUseDiamond().

Example where diamond cannot be used:

class X<T> {
	X(T x) {
	}

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


For 3.7.1 we could use the following simple heuristic: if the proposed constructor uses a type variable in the signature which is also used by the declaring type, then we say the diamond cannot be used. Later we can improve that if needed. Note though that we will never be able to give a 100% correct answer as it also depends which actual arguments the user passes after inserting the constructor proposal.
Comment 1 Dani Megert CLA 2011-07-07 10:03:01 EDT
*** Bug 350559 has been marked as a duplicate of this bug. ***
Comment 2 Ayushman Jain CLA 2011-07-08 11:25:41 EDT
(In reply to comment #0)
> Note though that we will never be able to give a 100% correct
> answer as it also depends which actual arguments the user passes after
> inserting the constructor proposal.

You mean this case? :

class X<T> {
    X(T x) {
    }

    X<Integer> x = new X<>(1);
}

i.e. even though the const. uses a type variable of the declaring class, the diamond usage is legal. yet the heuristic would say, cannot be used.
Comment 3 Dani Megert CLA 2011-07-11 03:22:28 EDT
(In reply to comment #2)
> (In reply to comment #0)
> > Note though that we will never be able to give a 100% correct
> > answer as it also depends which actual arguments the user passes after
> > inserting the constructor proposal.
> 
> You mean this case? :
> 
> class X<T> {
>     X(T x) {
>     }
> 
>     X<Integer> x = new X<>(1);
> }
> 
> i.e. even though the const. uses a type variable of the declaring class, the
> diamond usage is legal. yet the heuristic would say, cannot be used.

Yes, for a start. As said before, we can improve the heuristic later (or even now if it is obvious/inexpensive to set the right flag value.
Comment 4 Ayushman Jain CLA 2011-07-11 14:24:42 EDT
Created attachment 199438 [details]
proposed fix v1.0

This patch declares a new API in CompletionProposal
public boolean canUseDiamond(String[] paramTypeNames, CompletionContext coreContext)

Clients can just pass the parameter types in the constructor invocation and the completion context associated with a completion proposal to use this API. The actual computation is done in InternalExtendedCompletionContext since here bindings can be constructed and compared to see if the chosen constructor proposal uses any of the class' type variables (i.e. I've used the heuristic discussed above).

An example of using this API from jdt.text.ParameterGuessingProposal is 

             ........
	public void apply(IDocument document, char trigger, int offset) {
		boolean isDiamond = fProposal.canUseDiamond(this.getParameterTypes(), this.fCoreContext);
		try {
             ........

I tried a few other ways but none of them really worked (just to document):
1) Tried setting a flag with every constructor proposal at the time the proposal is built in jdt/core. However, this failed for those types that had been found from the search engine (i.e. those not in the same CU as the one in which CTRL+SPACE was done), since there were no bindings for those types.
2) Tried to create an Allocation expression from the CompletionProposal, but we do not have enough information at the time the user choses a particular proposal to be inserted.

So the patch is the best I could do. Dani, can you please tell me if this works for you? Thanks!
Comment 5 Dani Megert CLA 2011-07-12 06:48:47 EDT
> So the patch is the best I could do. Dani, can you please tell me if this works
> for you? Thanks!

It worked for the cases I tested. Of course we need to make the heuristics smarter during 3.8, e.g. for the following example the method should also return 'true':

public class X<T extends Number> {
        public X(T param) {
            new X
        }
}


Of course passing the parameter types which JDT Text needs to compute doesn't make sense to me. You have all that information in the proposal already, see CompletionProposal.getSignature().
Comment 6 Ayushman Jain CLA 2011-07-12 14:23:30 EDT
Created attachment 199522 [details]
proposed fix v2.0 + regression tests

This patch takes care of the above case as well i.e. wherever we don't have an LHS/return type (i.e. no expected type), we always return true (<> can always be inferred as Object). 
It also checks whether the proposal is a constructor, source level is < 1.7, etc.
Comment 7 Ayushman Jain CLA 2011-07-12 14:24:40 EDT
Olivier, please review. Thanks!
Comment 8 Olivier Thomann CLA 2011-07-13 14:04:50 EDT
Patch looks good.
Comment 9 Ayushman Jain CLA 2011-07-14 02:20:16 EDT
Released in BETA_JAVA7 branch
Comment 10 Raksha Vasisht CLA 2011-07-20 03:12:40 EDT
Verified.