Bug 193909 - improve content assist after 'instanceof'
Summary: improve content assist after 'instanceof'
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 enhancement with 1 vote (vote)
Target Milestone: 3.4 M3   Edit
Assignee: David Audel CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-06-22 04:33 EDT by Martin Aeschlimann CLA
Modified: 2007-10-29 13:50 EDT (History)
4 users (show)

See Also:


Attachments
Proposed fix (155.20 KB, patch)
2007-10-19 04:16 EDT, 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-06-22 04:33:57 EDT
3.3

The following code is quite common:

Object x= selection.getFirstElement();
if (x instanceof IType) {
   x.get|code assist

Code assist should suggest methods of Object ('getClass') and of IType ('getFullyQualifiedName').
If a method of IType is selected, a cast should be added to x (and IType imported if necessary)

if (x instanceof IType) {
   ((IType) x).getFullyQualifiedName
Comment 1 Philipe Mulet CLA 2007-08-31 13:02:31 EDT
Need some investigation before commitment for 3.4.
The suggestion is nice though.

Comment 2 David Audel CLA 2007-09-21 07:23:07 EDT
You would like that code assist suggest methods of IType for this simple case.

Object x= selection.getFirstElement();
if (x instanceof IType) {
   x.get|code assist

But do you expect the same behavior for more complex test case ?

1) There is some statements between completion location and the condition.

Object x= selection.getFirstElement();
if (x instanceof IType) {
   foo();
   x.get|code assist

2) The condition isn't only an instanceof expression.

Object x= selection.getFirstElement();
if (x != y && x instanceof IType) {
   x.get|code assist

3) The completion location is inside a nested control statement.

Object x= selection.getFirstElement();
if (x instanceof IType) {
   while(x != y) {
      x.get|code assist

Comment 3 Martin Aeschlimann CLA 2007-09-21 08:28:55 EDT
I think cases 1.) to 4.) should also work. Otherwise it will be tricky for the user to understand when the code assist is available. And typing a cast is always a pain. Especially inside method arguments

foo(((IType)x).getFullyQualifiedName() .. 

Note that this is a case to detect:
if (!(x instanceof X)) {
  ...
}
No cast proposals in this case! ;-)


Comment 4 Dani Megert CLA 2007-09-21 09:18:55 EDT
>I think cases 1.) to 4.) should also work.
Especially case 4 ;-)
Comment 5 David Audel CLA 2007-10-01 03:17:07 EDT
To return a proposal based on a previous 'instanceof', we could use the existing kind METHOD_REF.
The proposal of the method 'getFullyQualifiedName()' would be a standard method proposal with "((IType) x).getFullyQualifiedName()" as completion string.
If we want give extra informations about this proposal we would need do add new methods on CompletionProposal (eg CompletionProposal#getReceiverSignature()).

If we don't want to use this API then we need a new one.

1) We could add a new completion kind with the same  behavior as METHOD_REF.
Something like METHOD_REF_WITH_CASTED_RECEIVER.

METHOD_REF_WITH_CASTED_RECEIVER
  completion string = "((IType) x).getFullyQualifiedName()"

We would need to add a similar API for fields.

2) We could use a new kind of proposal for the cast expression and reuse METHOD_REF for the method proposal.

METHOD_REF
  completion string = "getFullyQualifiedName()" 
  required proposals [
    CAST_EXPRESSION
      completion string = "((IType) a)" 
  ]

To give extra information perhaps existing methods of CompletionProposal are sufficient. For CAST_EXPRESSION getSignature() could be use to return the receiver type.

What do you think about the different ways to return these new proposals ? Is METHOD_REF sufficient ?
Do you prefer to use a required proposal or a completly new kind of proposal ?
Do you want to access to informations like receiver type or the completion string is sufficient ?
Comment 6 Martin Aeschlimann CLA 2007-10-01 11:36:06 EDT
For compatibility reasons I would suggest to add new completion kinds:
METHOD_REF_WITH_CASTED_RECEIVER
FIELD_REF_WITH_CASTED_RECEIVER

Comment 7 Dani Megert CLA 2007-10-05 04:35:08 EDT
I agree with Martin.
Comment 8 David Audel CLA 2007-10-08 07:18:51 EDT
I agree that propose a METHOD_REF alone could cause compatibility issue but propose a METHOD_REF with a CAST_EXPRESSION as required proposal wouldn't because proposals with required proposals are proposed only if they are asked explicitly by client (requestor.setAllowsRequiredProposals(METHOD_REF, CAST_EXPRESSION, true)).
So the API METHOD_REF_WITH_CASTED_RECEIVER or METHOD_REF+CAST_EXPRESSION have no problem of compatibility.
Comment 9 Dani Megert CLA 2007-10-10 04:57:58 EDT
Even if there's no compatibility issue I'd prefer METHOD_REF_WITH_CASTED_RECEIVER as in my opinion it's cleaner. Also, with the other approach I need to add a special case (if tests) for CAST_EXPRESSION to the method ref proposal and hence slow them for nothing.

Martin, what's your take on this?
Comment 10 Martin Aeschlimann CLA 2007-10-10 05:28:04 EDT
I agree with Dani. If I understand correctly, 'Cast expression' proposal is just used as a required proposal, but never by itself. This is a bit strange.
METHOD_REF_WITH_CASTED_RECEIVER/FIELD_REF_WITH_CASTED_RECEIVER seems easier to understand for me. All explanations will be at Javadoc of these constants.
Comment 11 David Audel CLA 2007-10-19 04:13:47 EDT
I followed the METHOD_REF_WITH_CASTED_RECEIVER approach.

With this approach we need to add two completion kinds: METHOD_REF_WITH_CASTED_RECEIVER and FIELD_REF_WITH_CASTED_RECEIVER

if (x instanceof IType) {
   x.get|code assist

To use these proposals some new informations, not currently given by CompletionProposal, would be useful for clients:
- The type of the cast (eg. IType)
- The range of the receiver (eg. the range of 'x')

The range of the completed token (eg. 'get') would be useful and is already given by the existing methods CompletionProposal#getTokenStart() and CompletionProposal#getTokenEnd() (works since the fix of bug 206336)

The proposed API is:

public class CompletionProposal {
   ...

   /**
    * Completion is a reference to a method with a casted receiver.
    * This kind of completion might occur in a context like
    * <code>"receiver.fo^();"</code> and complete it to
    * <code>""((X)receiver).foo();"</code>.
    * <p>
    * The following additional context information is available
    * for this kind of completion proposal at little extra cost:
    * <ul>
    * <li>{@link #getDeclarationSignature()} -
    * the type signature of the type that declares the method that is referenced
    * </li>
    * <li>{@link #getFlags()} -
    * the modifiers flags of the method that is referenced
    * </li>
    * <li>{@link #getName()} -
    * the simple name of the method that is referenced
    * </li>
    * <li>{@link #getReceiverSignature()} -
    * the type signature of the receiver type. It's the type of the cast expression.
    * </li>
    * <li>{@link #getSignature()} -
    * the method signature of the method that is referenced
    * </li>
    * </ul>
    * </p>
    * 
    * @see #getKind()
    * 
    * @since 3.4
    */
    public static final int METHOD_REF_WITH_CASTED_RECEIVER;

   /**
    * Completion is a reference to a field with a casted receiver.
    * This kind of completion might occur in a context like
    * <code>"recevier.ref^ = 0;"</code> and complete it to
    * <code>"((X)receiver).refcount = 0;"</code>.
    * <p>
    * The following additional context information is available
    * for this kind of completion proposal at little extra cost:
    * <ul>
    * <li>{@link #getDeclarationSignature()} -
    * the type signature of the type that declares the field that is referenced
    * </li>
    * <li>{@link #getFlags()} -
    * the modifiers flags (including ACC_ENUM) of the field that is referenced
    * </li>
    * <li>{@link #getName()} -
    * the simple name of the field that is referenced
    * </li>
    * <li>{@link #getReceiverSignature()} -
    * the type signature of the receiver type. It's the type of the cast expression.
    * </li>
    * <li>{@link #getSignature()} -
    * the type signature of the field's type (as opposed to the
    * signature of the type in which the referenced field
    * is declared)
    * </li>
    * 
    * </ul>
    * </p>
    * 
    * @see #getKind()
    * 
    * @since 3.4
    */
   public static final int FIELD_REF_WITH_CASTED_RECEIVER;

   /**
    * Returns the type signature or package name of the relevant
    * receiver in the context, or <code>null</code> if none.
    * <p>
    * This field is available for the following kinds of
    * completion proposals:
    * <ul>
    *  <li><code>FIELD_REF_WITH_CASTED_RECEIVER</code> - type signature
    * of the type that cast the receiver of the field that is referenced</li>
    *  <li><code>METHOD_REF_WITH_CASTED_RECEIVER</code> - type signature
    * of the type that cast the receiver of the method that is referenced</li>
    * </ul>
    * For kinds of completion proposals, this method returns
    * <code>null</code>. Clients must not modify the array
    * returned.
    * </p>
    * 
    * @return a type signature or a package name (depending
    * on the kind of completion), or <code>null</code> if none
    * @see Signature
    * 
    * @since 3.4
    */
   public char[] getReceiverSignature() {}

   /**
    * Returns the character index of the start of the
    * subrange in the source file buffer containing the
    * relevant receiver of the member being completed. This
    * receiver is an expression.
    * 
    * <p>
    * This field is available for the following kinds of
    * completion proposals:
    * <ul>
    *  <li><code>FIELD_REF_WITH_CASTED_RECEIVER</code></li>
    *  <li><code>METHOD_REF_WITH_CASTED_RECEIVER</code></li>
    * </ul>
    * For kinds of completion proposals, this method returns <code>0</code>.
    * </p>
    * 
    * @return character index of receiver start position (inclusive)
    * 
    * @since 3.4
    */
   public int getReceiverStart() {}

   /**
    * Returns the character index of the end (exclusive) of the subrange
    * in the source file buffer containing the
    * relevant receiver of the member being completed.
    * 
    * * <p>
    * This field is available for the following kinds of
    * completion proposals:
    * <ul>
    *  <li><code>FIELD_REF_WITH_CASTED_RECEIVER</code></li>
    *  <li><code>METHOD_REF_WITH_CASTED_RECEIVER</code></li>
    * </ul>
    * For kinds of completion proposals, this method returns <code>0</code>.
    * </p>
    * 
    * @return character index of receiver end position (exclusive)
    * 
    * @since 3.4
    */
   public int getReceiverEnd() {}

   ...
}
Comment 12 David Audel CLA 2007-10-19 04:16:25 EDT
Created attachment 80752 [details]
Proposed fix

This patch add the API proposed in comment 11.

With this patch when the completion occurs inside an if statement and the condition is an instanceof expression and a member access is completed then members of the cast type are proposed.
It will work only if:
- The condition expression is an instanceof expression.
  Support all expressions which contains an instanceof expression would require to add flow analysis to completion process which would be costly for performance and complex to add.
      if (a instanceof X) // will work
      if (a != null && a instanceof X) // won't work
      if (!(a instanceof X)) // won't work
- The completion doesn't occurs in a nested block or control statement. When we look inside JDT code more than 95% of the cast are on the first line of the if block, so it doesn't seem to be a big restriction. 
      if (a instanceof X) {
         a.foo| // will work
      if (a instanceof X) {
         while (true) {
            a.foo| // won't work

It will work if the receiver is a local variable or a field of an enclosing type.
  Object a;
  if (a instanceof X) {
      a.foo| // will work

  Object a;
  void foo() {
     if (a instanceof X) {
         a.foo| // will work

  Object a;
  void foo() {
     if (this.a instanceof X) {
         this.a.foo| // will work
Comment 13 Dani Megert CLA 2007-10-24 11:43:43 EDT
Patch is good. I'll be able to put this in on my side for M3.
Comment 14 David Audel CLA 2007-10-25 07:11:05 EDT
Released for 3.4M3.

Tests added
  CompletionTests#testCompletionAfterInstanceof01() ->
testCompletionAfterInstanceof24_2()
Comment 15 Jerome Lanneluc CLA 2007-10-29 13:50:41 EDT
Verified for 3.4M3 using I20071029-0010