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

Bug 351165

Summary:

[1.7] API to get the type arguments of the declaring type for constructor invocations

Product:

[Eclipse Project] JDT

Reporter:

Markus Keller <markus.kell.r>

Component:

Core

Assignee:

Satyam Kandula <satyam.kandula>

Status:

VERIFIED FIXED

QA Contact:

Severity:

enhancement

Priority:

CC:

amj87.iitr, deepakazad, Olivier_Thomann, satyam.kandula, srikanth_sankaran

Version:

3.7

Flags:

markus.kell.r: review+

Target Milestone:

3.7.1

Hardware:

OS:

Windows 7

Whiteboard:

Bug Depends on:

Bug Blocks:

351048

Attachments:

Description	Flags
Proposed patch + tests	none
Patch for ASTView	none
Proposed patch + regression test	none

Description Markus Keller

2011-07-05 06:58:54 EDT

BETA_JAVA7

For bug 351048, we'd like to render the type arguments of the declaring type for a constructor invocation in the Javadoc hover.

Example: new ArrayList<>()

codeSelect returns an IMethod with key:
Ljava/util/ArrayList<Ljava/lang/String;>;.()V

The necessary information is already available in the key, but BindingKey doesn't offer API to read the type arguments of the declaring type. Best would be an API like this:

	/**
	 * Returns the binding key of the declaring type of the element
	 * represented by this binding key. If the binding key does not represent
	 * a member or if the member doesn't have a declaring type, returns
	 * <code>null</code>.
	 * 
	 * @return the type binding key or <code>null</code>
	 * @since 3.7
	 */
	public BindingKey getDeclaringType() {
		// FIXME: this is a hack
		int dot = this.key.indexOf('.');
		if (dot == -1)
			return null;
		return new BindingKey(this.key.substring(0, dot));
	}

An alternative could be to improve IMethod#getDeclaringType(): Currently, this returns an unresolved type, but it could also return a resolved type whose key contains the type arguments.

Comment 1 Satyam Kandula

2011-07-11 09:07:04 EDT

I will work on this.

Comment 2 Satyam Kandula

2011-07-14 07:23:14 EDT

Created attachment 199650 [details]
Proposed patch + tests

Comment 3 Satyam Kandula

2011-07-14 07:23:54 EDT

Markus, Can you please review. Thanks in advance.

Comment 4 Markus Keller

2011-07-14 11:38:31 EDT

Created attachment 199675 [details]
Patch for ASTView

We have a problem with parameterized types here. Apply the attached patch to the ASTView (http://www.eclipse.org/jdt/ui/astview/, take BETA_JAVA7 branch).

I expected the new binding properties "*** getKey().getDeclaringType()" and "*** getDeclaringClass().getKey()" to always be the same.

But in some cases, the binding key doesn't contain the declaring type's type parameters (declarations of fields, methods, type parameters) while for member and local types, and for references to fields and methods, the type arguments are available.

I'm not sure yet what's the best way to proceed. Ideas welcome.

For bug 351048, I only need the type arguments for constructor invocations. Maybe we should narrow the API to just support this use case.


Here's my toy class:

package org.test;
public class BindingKeysTest<TPAR> {
	static class StaticMember { }
	class Member { }
	
	int field;
	BindingKeysTest<? extends java.util.List<TPAR>> field2;
	
	void foo() {
		int var;
		BindingKeysTest<Integer> var2= new BindingKeysTest<Integer>();
		var2.foo();
		field2= new BindingKeysTest();
		class Locl<E> {
			/**
			 * Ctor
			 * @param arg the arg!
			 */
			public Locl(E arg) {
				
			}
		}
		Locl<Integer> locl = new Locl<Integer>(42);
		System.out.println(locl);
		
		
		class Local2 {
			/**
			 * Ctor Local2
			 */
			Local2() {
				
			}
		}
		Local2 l2= new Local2();
	}
}

Comment 5 Satyam Kandula

2011-07-18 02:54:59 EDT

(In reply to comment #4)
Thanks Markus for catching the issues. I completely missed verifying the declarations. I should use the ASTview! 

As the signature doesn't have the type information of the type for declarations, this API probably doesn't make sense. I couldn't think of any good alternative apart from you doing the same in your code. 

cc'ing Olivier for any ideas.

Comment 6 Markus Keller

2011-07-26 15:12:14 EDT

I think the best solution is to go with the proposed patch but document the limitations e.g. like this:

	 * <p>
	 * Note that only binding keys for references to methods and fields
	 * are fully supported. E.g. the declaring type of a method declaration
	 * will miss any type parameters.

Comment 7 Satyam Kandula

2011-07-27 00:05:49 EDT

I agree that documenting the limitations is one option but this can be done in JDT/UI and hence why is the API needed?

Comment 8 Markus Keller

2011-07-27 05:53:16 EDT

> this can be done in JDT/UI and hence why is the API needed?

Because the format of the keys is not specified, see IBinding#getKey(). Since the format can change, parsing the key would be a illegal access to internal information.

Comment 9 Satyam Kandula

2011-07-27 06:16:35 EDT

(In reply to comment #8)
> > this can be done in JDT/UI and hence why is the API needed?
> 
> Because the format of the keys is not specified, see IBinding#getKey(). Since
> the format can change, parsing the key would be a illegal access to internal
> information.
However the key as in char[] is returned by the JDT/Core. Hence, I will be sceptical of changing that.

Comment 10 Markus Keller

2011-07-27 10:33:24 EDT

Yes, that's what IBinding#getKey() says. But it doesn't matter, since we won't add a layer breaker anyway. Parsing of the key format is a JDT Core job.

So we have 3 choices:
1. Do nothing, i.e. don't implement bug 351048.
2. Release the patch with the additional Javadoc.
3. Try to implement the alternative I mentioned in comment 0 (make IMethod#getDeclaringType() return a resolved type if the target IMethod is resolved)

My favorite is 3.

Comment 11 Markus Keller

2011-07-27 10:34:36 EDT

> My favorite is 3.
*** Bad typo ***, I meant my favorite is **2.**

Comment 12 Olivier Thomann

2011-07-27 10:37:51 EDT

(In reply to comment #11)
> > My favorite is 3.
> *** Bad typo ***, I meant my favorite is **2.**
Same for me. We should prevent external scanning of the keys as much as possible.

Comment 13 Satyam Kandula

2011-07-28 02:08:05 EDT

Created attachment 200490 [details]
Proposed patch + regression test

Same patch with the updated javadoc.

Comment 14 Srikanth Sankaran

2011-08-04 00:29:10 EDT

We are taking stock of where things stand for 3.7.1 - Markus, does the
patch posted at comment#13 meet your needs ?

Comment 15 Markus Keller

2011-08-04 06:45:41 EDT

> Markus, does the patch posted at comment#13 meet your needs ?

Yes, but you have to change it to @since 3.7.1 (+ an API filter for the warning).

Comment 16 Satyam Kandula

2011-08-08 01:29:48 EDT

Made the since tag to have 3.7.1 and added the filters and released in 3.7.1 and HEAD.

Comment 17 Markus Keller

2011-08-26 08:11:20 EDT

Verified in M20110825-0847 and I20110823-0925. Filed bug 355934 for a problem with constructor invocations that are assigned to a field.