Bug 202470 - [assist] provide all elements that are visible
Summary: [assist] provide all elements that are visible
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P1 normal (vote)
Target Milestone: 3.4 M6   Edit
Assignee: David Audel CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 177655 70005 201718 204551
  Show dependency tree
 
Reported: 2007-09-06 10:52 EDT by Martin Aeschlimann CLA
Modified: 2008-04-09 13:24 EDT (History)
4 users (show)

See Also:


Attachments
Implementation of the proposed API (47.39 KB, patch)
2007-11-16 05:14 EST, David Audel CLA
no flags Details | Diff
Proposed patch (296.62 KB, patch)
2008-03-06 10:13 EST, David Audel CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Aeschlimann CLA 2007-09-06 10:52:46 EDT
3.3

To proceed on bug 110181, we decided to split it in smaller, specific requests.

To be able to improve the performance of our own proposals 
we need to know what elements are visible at the current location.

> On method completions, we want to fill in good arguments (and offer alternative proposals)
> On template completion, we want to guess what to fill in the template variables and offer alternative proposals

Minimal requirement:

On each code assist, give list of:
- local variables, parameters and fields visible in the scope
- the type of these variables

If possible:
- Bindings of the variable types, so we can see if type is assignment compatible to the template variable we want to fill in
- Methods

Example

class A {

private int x;

public void foo(boolean z, List l, String name) {

foo| // visible elements: int x, boolean z, List l, String name;
}
}
Comment 1 Jerome Lanneluc CLA 2007-09-11 08:15:54 EDT
To be investigated for 3.4
Comment 2 David Audel CLA 2007-11-16 05:12:52 EST
It's possible to give the list of local variables, parameters, fields and methods.
But i won't use bindings to return this list because during completion process internal bindings are build from an incomplete ast and the binding resolution is not complete.
We can use instead an API similar to CompletionProposal which use signatures (or binding keys).

public final class CompletionContext {
	...
	public static abstract class Element {
		public static final int K_LOCAL = 1;
		public static final int K_FIELD = 2;
		public static final int K_METHOD = 3;
		
		public int getKind();
		
		public char[] getName();
		
		public char[] getSignature();
		
		public char[] getDeclarationSignature();
		
		public int getFlags();
		
		public char[] getKey();
	}
	
	/**
	 * Returns elements visible from the completion location.
	 */
	public Element[] getVisibleElements();
	...
}


Name, signature and declaration signature allow to identify the element. I believe that modifier flags are required for arguments guessing.
Binding key give redundant information but could be compared to other keys given by code assist API like CompletionContext#getExpectedTypesKeys()

Compute these visible elements can have a noticeable impact on performance so it must be an optional behavior (disable by default).
In your case the performance loss on visible elements computing will be certainly smaller than the performance gain on argument guessing.


public class CompletionRequestor {
	...
	/**
	 * Visible elements are computed only if askVisibleElements() return true. Returns false by default.
	 */
	public boolean askVisibleElements() { return false; }
	...
}

What do you think about these API ?

I will attach to this bug an implementation of these API.
Comment 3 David Audel CLA 2007-11-16 05:14:43 EST
Created attachment 83053 [details]
Implementation of the proposed API
Comment 4 Martin Aeschlimann CLA 2007-11-16 05:57:56 EST
Thanks for the proposal! I have two comments:

- About bindings: The advantage of bindings would have been that we could have avoided to create type hierarchies of all field/variable types and method return types to find out if we can fill in a variable for a given parameter (bug 201718). 
We have made some performance improvements there, but if code assist has all that information, it would be good if we could reuse it.
Maybe we can spec that there are some restrictions to the returned bindings? For example it would be OK if you it is missing some information, like incomplete members on types. (see IBinding.isRecovered())

- About the new 'Element' type. I guess you agree that the all-in-one API is not the most wonderful. Would it be possible to use Java elements instead? Maybe something like 'resolved' Java elements, where we can get the key or even the types in a resolved way (like for class files). 
  IJavaElement[] getVisibleElements(); // ILocalVariable, IField, IMethod

Comment 5 Dani Megert CLA 2007-11-19 07:06:32 EST
I agree with Martin here: assuming you already have the bindings it would be good to give us access to that information (but don't compute it just for us). A Java element based solution also looks more attractive to me.

>askVisibleElements
I would rename this to areVisibleElementsRequested() and offer setRequireVisibleElements(boolean). This would be more aligned to the existing setters.
Comment 6 Jerome Lanneluc CLA 2007-11-22 09:07:08 EST
The solution with IJavaElement is attractive indeed. However, the given IJavaElements won't be in the Java model cache. As a result, asking questions (like getFlags()) to such IJavaElement will force another parsing. This is too bad since code asssist already had this information. We are afraid that this would have a performance impact.

So the main question is what information do you exactly need? Could you please list the methods you need here? From there, we can decide what the best approach is. We could for example provide a special IJavaElement that would cache the information (e.g. the flags) on the handle itself.
Comment 7 Martin Aeschlimann CLA 2007-11-26 04:27:07 EST
It would be good if the special Java element could contain:
- the resolved type of all variables (fields, local variables)
- flags

Also good to have:
- method return type and signature



The resulting API would be something like this:
IJavaElement[] getVisibleElements(); // ILocalVariable, IField, IMethod

If you have some type hierarchy information then would could think of an additional API that would help us with the variable suggestions:

IJavaElement[] getAssignableElements(String typeSignature); // ILocalVariable, IField, IMethod

returning all visible variables/methods that can be assigned to the type.

Again, as Dani said in comment 5, such an API would only make sense if JDT core has all the information already. If it first needs to be computed, it's probably better if we do it ourselves with ITypeHierarchies when applying the proposal. 
Comment 8 Dani Megert CLA 2008-02-01 03:31:06 EST
*** Bug 217267 has been marked as a duplicate of this bug. ***
Comment 9 Dani Megert CLA 2008-02-01 03:46:57 EST
*** Bug 177655 has been marked as a duplicate of this bug. ***
Comment 10 Dani Megert CLA 2008-02-01 03:48:06 EST
Another use case for this new API would be bug 177655.
Comment 11 David Audel CLA 2008-02-04 04:54:34 EST
Return an array of IJavaElement seems to be the better way to give the list of visible elements. And there is no need to add special IJavaElement that cache information on handle because the returned elements will be almost always opened java elements.

Comment 12 David Audel CLA 2008-02-04 04:55:04 EST
Daniel, Martin - If i can add the API 'getAssignableElements(...)' suggested in comment 7 do you still need of 'getVisibleElements()' or do you will use only 'getAssignableElements()' ?
Comment 13 Martin Aeschlimann CLA 2008-02-04 05:08:14 EST
We still need getVisibleElements for the getter/setter and for the 'constructor with parameters' proposal (bug 177655).

getAssignableElements(..) would be used for parameter guessing. 
Comment 14 David Audel CLA 2008-02-06 05:40:39 EST
Why do you need getVisibleElements for the getter/setter and for the 'constructor
with parameters' ?

If i correctly understand, you only need of the list of fields and methods defined in the enclosing type and you don't need of the list of all visible elements.
IType#getMethods() or IType#getFields() can give you these information and that's you currently do in CompletionProposalCollector#acceptPotentialMethodDeclaration().
Comment 15 Martin Aeschlimann CLA 2008-02-06 06:30:30 EST
That's correct, in this case we only need the fields of the enclosing type. However, at the moment getField only works when the compilation unit has been reconciled.
So the correct code would be

cu.reconcile()
IType#getFields()

but we had to remove the reconcile call for performance reasons. 

For this reason you sometimes see no getter/setter proposals (because the reconciler has not yet ran) and have to try code assist a second time.
Comment 16 David Audel CLA 2008-03-06 10:13:17 EST
Created attachment 91767 [details]
Proposed patch
Comment 17 David Audel CLA 2008-03-06 10:34:30 EST
Released for 3.4M6.

Tests added
  CompletionContextTests#test0143() -> test0161()
  CompletionContextTests_1_5#test0028() -> test0036()

The arguments guessing and the getter/setter completion doesn't need the same information.
The arguments guessing needs the list of visible elements.
getter/setter needs the enclosing element and its children.

So i added two different API:
* CompletionContext#getEnclosingElement() -  This method returns the innermost enclosing Java element which contains the completion location.

public class X {
	void foo() {
		zzz| // ctrl+space at | location
	}
}

getEnclosingElement() returns the IMethod named foo.

* CompletionContext#getVisibleElements(String) - This method returns the elements visible from the completion location.

public class X {
	p.Y f1;
	p.Z f2;
	void foo() {
		p.Y l1;
		p.Z l2;
		zzz| // ctrl+space at | location
	}
}

getVisibleElements("Lp/Z;") returns the IField named f2 and the ILocalVariable name l2.



Added API are:
public class CompletionRequestor {
	...
	
	/**
	 * Returns whether this requestor requires an extended context.
	 * 
	 * By default this method return <code>false</code>.
	 * 
	 * @return <code>true</code> if this requestor requires an extended context.
	 * 
	 * @see CompletionContext#isExtended()
	 * 
	 * @since 3.4
	 */
	public boolean isExtendedContextRequired() {...}
	
	/**
	 * Sets whether this requestor requires an extended context.
	 * 
	 * @param require <code>true</code> if this requestor requires an extended context.
	 * 
	 * @see CompletionContext#isExtended()
	 * 
	 * @since 3.4
	 */
	public void setRequireExtendedContext(boolean require) {...}
	
	...
}

public class CompletionContext {
	...
	
	/**
	 * Returns whether this completion context is an extended context.
	 * Some methods of this context can be used only if this context is an extended context but an extended context consumes more memory.
	 * 
	 * @return <code>true</code> if this completion context is an extended context.
	 * 
	 * @since 3.4
	 */
	public boolean isExtended() {...}
	
	/**
	 * Returns the innermost enclosing Java element which contains the completion location or <code>null</code> if this element cannot be computed.
	 * The returned Java element and all Java elements in the same compilation unit which can be navigated to from the returned Java element are special Java elements:
	 * <ul>
	 * <li>they are based on the current content of the compilation unit's buffer, they are not the result of a reconcile operation</li>
	 * <li>they are not updated if the buffer changes.</li>
	 * <li>they do not contain local types which are not visible from the completion location.</li>
	 * <li>they do not give information about categories. {@link IMember#getCategories()} will return an empty array</li>
	 * </ul>
	 * 
	 * Reasons for returning <code>null</code> include:
	 * <ul>
	 * <li>the compilation unit no longer exists</li>
	 * <li>the completion occurred in a binary type. However this restriction might be relaxed in the future.</li>
	 * </ul>
	 * 
	 * @return the innermost enclosing Java element which contains the completion location or <code>null</code> if this element cannot be computed.
	 * 
	 * @exception UnsupportedOperationException if the context is not an extended context
	 * 
	 * @since 3.4
	 */
	public IJavaElement getEnclosingElement() {...}
	
	/**
	 * Return the elements which are visible from the completion location and which can be assigned to the given type.
	 * An element is assignable if its type can be assigned to a variable
	 * of the given type, as specified in section 5.2 of <em>The Java Language
	 * Specification, Third Edition</em> (JLS3).
	 * A visible element is either:
	 * <ul>
	 * <li>a {@link ILocalVariable} - the element type is {@link ILocalVariable#getTypeSignature()}</li>
	 * <li>a {@link IField} - the element type is {@link IField#getTypeSignature()}</li>
	 * <li>a {@link IMethod} - the element type is {@link IMethod#getReturnType()}</li>
	 * </ul>
	 * 
	 * Returned elements defined in the completed compilation unit are special Java elements:
	 * <ul>
	 * <li>they are based on the current content of the compilation unit's buffer, they are not the result of a reconcile operation</li>
	 * <li>they are not updated if the buffer changes.</li>
	 * <li>they do not contain local types which are not visible from the completion location.</li>
	 * <li>they do not give information about categories. {@link IMember#getCategories()} will return an empty array</li>
	 * </ul>
	 *
	 * Note the array can be empty if:
	 * <ul>
	 * <li>the compilation unit no longer exists</li>
	 * <li>the completion occurred in a binary type. However this restriction might be relaxed in the future.</li>
	 * </ul>
	 * 
	 * @param typeSignature elements which can be assigned to this type are returned.
	 * 		If <code>null</code> there is no constraint on the type of the returned elements.
	 * 
	 * @return elements which are visible from the completion location and which can be assigned to the given type.
	 * 
	 * @exception UnsupportedOperationException if the context is not an extended context
	 * 
	 * @see #isExtended()
	 * 
	 * @since 3.4
	 */
	public IJavaElement[] getVisibleElements(String typeSignature) {...}
	
	...
}
Comment 18 Jerome Lanneluc CLA 2008-03-25 10:51:39 EDT
Verified for 3.4M6 using I20080325-0100