Bug 436155 - [type hierarchy] No type hierarchy shown for org.eclipse.swt.widgets.Text
Summary: [type hierarchy] No type hierarchy shown for org.eclipse.swt.widgets.Text
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.4   Edit
Hardware: All All
: P1 critical (vote)
Target Milestone: 4.4 RC4   Edit
Assignee: Jay Arthanareeswaran CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 439093 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-05-29 07:36 EDT by Noopur Gupta CLA
Modified: 2014-07-08 05:48 EDT (History)
9 users (show)

See Also:
markus.kell.r: review+
manoj.palat: review+
srikanth_sankaran: review+


Attachments
Proposed fix (1013 bytes, patch)
2014-05-31 08:39 EDT, Jay Arthanareeswaran CLA
no flags Details | Diff
Same patch with test (3.37 KB, patch)
2014-05-31 16:52 EDT, Jay Arthanareeswaran CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noopur Gupta CLA 2014-05-29 07:36:32 EDT
package bug;

import org.eclipse.swt.widgets.Text;

class T2 {
	Text text = new Text(null, 0);

	private void bar() {
		text.setSelection(0);
	}
}
--------------------------------------------------

Place the caret at #setSelection in the above example and press Ctrl+T.
The resulting quick type hierarchy pop-up is empty.

Place caret at "Text" and press F4. 
Type Hierarchy view shows only "Object".

This used to work before Java 8 related code changes (up to I20140311-1200).
Comment 1 Dani Megert CLA 2014-05-29 08:18:35 EDT
This is a must fix.

If I open only Text (as binary), the hierarchy is fine. As soon as I open the source which refers to Text, e.g. T2 from comment 2, it is broken, even if the hierarchy is requested in the editor for Text.class.
Comment 2 Noopur Gupta CLA 2014-05-29 11:27:52 EDT
The problem seems to be in org.eclipse.jdt.internal.core.hierarchy.HierarchyResolver with the changes in commit 76226936180e26d9e682e909ba765fff61d327d9.

When the change in HierarchyResolver at line 684 is reverted back from:

containsLocalType = cu.isWorkingCopy() ? true /* presume conservatively */ : localTypes.contains(path.toString());

to:

containsLocalType = localTypes.contains(path.toString());

the issue does not happen anymore.
Comment 3 Jay Arthanareeswaran CLA 2014-05-29 11:53:48 EDT
(In reply to Noopur Gupta from comment #2)
> The problem seems to be in
> org.eclipse.jdt.internal.core.hierarchy.HierarchyResolver with the changes
> in commit 76226936180e26d9e682e909ba765fff61d327d9.
> 
> When the change in HierarchyResolver at line 684 is reverted back from:
> 
> containsLocalType = cu.isWorkingCopy() ? true /* presume conservatively */ :
> localTypes.contains(path.toString());
> 
> to:
> 
> containsLocalType = localTypes.contains(path.toString());
> 
> the issue does not happen anymore.

Thanks so much Noopur for narrowing down on the commit.

As for the original fix, the variable containsLocalType (whose value is assumed to true if/when the CU is working copy) has far reaching impact. It is used not just for setting the LOCAL_TYPE flag while building CU, but it is also stored and used further down the method including for completing bindings. I will look at this.
Comment 4 Jay Arthanareeswaran CLA 2014-05-31 08:39:06 EDT
Created attachment 243750 [details]
Proposed fix

Here's what's going:

The commit Nooper mentioned forces the fields and method bodies of type T2 (example in comment #0) to be parsed. This forces two things:

1. The type org.eclipse.swt.widgets.Text to be resolved and cached [1]
2. The superclass of Text is set (but the sub types are not yet computed) [2] 

[2] happens via this call stack [3]:

	BinaryTypeBinding.cachePartsFrom(...) line: 435	
	LookupEnvironment.createBinaryTypeFrom(...) line: 695	
	LookupEnvironment.cacheBinaryType(...) line: 213	
	HierarchyResolver.resolve(...) line: 755	
	IndexBasedHierarchyBuilder.buildForProject(...) line: 227	
	IndexBasedHierarchyBuilder.buildFromPotentialSubtypes(...) line: 344	

[1] means, the following condition [4] at line no 843 in HierarchyResolver fails:

	if (focusBinaryBinding == null)
				return;
				
And because of that, we go ahead and report the incomplete hierarchy for Text.
				
Later when the call returns to IndexBasedHierarchyBuilder.buildFromPotentialSubtypes(...), we check for the type to be in the hierarchy (line no: 335). The following condition fails, due to which we don't proceed with the buildForProject() call (line no: 344)

	if (!this.hierarchy.contains(focusType)) {...}

And that's how we end up with incomplete hierarchy.

The solution: simply enhance the condition [4] with the check for TagBits.HasUnresolvedSuperclass, which would have been set at [4].

Question: Why do we need this special treatnment for lambdas and not for anonymous?
Ans: I think it's because the SourceElementNotifier and CompilationUnitStructureRequester together ensure full parse happens and cached in Java model when there are local types. This will be a worthwhile exercise post Luna to make similar things for Lambda.

With this fix, all JDT Core and UI tests pass. I will add a new test for this case.
Comment 5 Jay Arthanareeswaran CLA 2014-05-31 16:52:49 EDT
Created attachment 243756 [details]
Same patch with test

Test added to the fix
Comment 6 Srikanth Sankaran CLA 2014-06-02 01:45:40 EDT
(In reply to Jayaprakash Arthanareeswaran from comment #4)

> Question: Why do we need this special treatnment for lambdas and not for
> anonymous?
> Ans: I think it's because the SourceElementNotifier and
> CompilationUnitStructureRequester together ensure full parse happens and
> cached in Java model when there are local types. This will be a worthwhile
> exercise post Luna to make similar things for Lambda.
> 
> With this fix, all JDT Core and UI tests pass. I will add a new test for
> this case.

Looks reasonable to me.
Comment 7 Jay Arthanareeswaran CLA 2014-06-02 04:11:50 EDT
(In reply to Jayaprakash Arthanareeswaran from comment #5)
> Created attachment 243756 [details]
> Same patch with test

This passes all JDT Core and UI tests. To further add a word about the patch as to why this is safe:

The presence of bit TagBits.HasUnresolvedSuperclass means the type was resolved but not fully processed for type hierarchy as I mentioned in comment #4.

In certain cases (like the one we are dealing with here), a type can have it's hierarchy partially set, such as only it's super types computed but not sub types. At this point, we should delay reporting it's hierarchy.

Looking at IndexBasedHierarchyBuilder.buildFromPotentialSubtypes(), line no: 335, it is guaranteed that buildForProject() is called one more time which will ensure that the complete hierarchy will be reported.

This fix also ensures that the commit that is mentioned in comment #2 is left untouched, which means we continue to report local types in the hierarchy.
Comment 8 Markus Keller CLA 2014-06-03 08:30:48 EDT
+1 for RC4.

The patch also fixes problems with this field declaration in a working copy:

	org.eclipse.swt.custom.MovementListener mListener = null;

Without the patch, the supertype hierarchy for interface MovementListener contained SWTEventListener as super*class*. With the patch, everything is fine.
Comment 10 Dani Megert CLA 2014-06-04 04:38:45 EDT
Verified in I20140603-2300.
Comment 11 Jay Arthanareeswaran CLA 2014-07-08 05:48:57 EDT
*** Bug 439093 has been marked as a duplicate of this bug. ***