Bug 124489 - [1.5][search] "Find unused dependencies" misses references to generic types
Summary: [1.5][search] "Find unused dependencies" misses references to generic types
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.1.1   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 3.3 M1   Edit
Assignee: Frederic Fusier CLA
QA Contact:
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks:
 
Reported: 2006-01-19 09:47 EST by Matt McCutchen CLA
Modified: 2006-08-04 09:58 EDT (History)
2 users (show)

See Also:


Attachments
seems to fix the bug by deleting code that puts <T> in pattern (5.76 KB, patch)
2006-04-09 23:50 EDT, Matt McCutchen CLA
no flags Details | Diff
Proposed patch (18.46 KB, patch)
2006-08-03 12:28 EDT, Frederic Fusier CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt McCutchen CLA 2006-01-19 09:47:29 EST
My Eclipse: version 3.1.1, build id M20050929-0840 (Fedora Core release 4 (Stentz))

The "Find unused dependencies" feature in the Plug-in Manifest Editor misses all references to generic types, so it will sometimes decide that a dependency is unused when a generic type from the dependency is actually used.

To reproduce this, create two Java plug-in projects foo and bar.  In foo, create package foo with the class below, and export package foo as visible to all plugins.

public class Foo<T> {}

In bar, list foo as a dependency in the Plug-in Manifest Editor, and then create this class:

public class Bar {
    Foo<Integer> f = new Foo<Integer>();
    Foo f2 = new Foo();
}

Click "Find unused dependencies" for bar.  Eclipse will announce that foo is unused, which is not the case; if you let it remove foo from the dependencies, "Foo cannot be resolved to a type" errors appear promptly on class Bar.

I speculated that this might have to do with bug 122995 (the access rule checker misses most references to generic types), but Maxime Daniel fixed that bug and reports that this bug remains.  Moreover, a Java search for references to class Foo correctly finds the four references in Bar.
Comment 1 Matt McCutchen CLA 2006-02-06 17:31:46 EST
I think I have found the cause of this bug.  In the example the PDE goes through all the Java types in foo to check whether each is referenced in bar using a Java search for references.  However, the SearchPattern for references to class Foo is actually for the parameterized type Foo<T>.  The pattern won't match Foo<Integer> or plain Foo, but it will match "Foo<T>" if this is typed into the code for Bar, even though there is not really a type T.

I'm not sure whether this is a JDT search bug or whether the PDE should request an erasure match in DependencyExtentOperation, which also fixes the problem.
Comment 2 Wassim Melhem CLA 2006-02-10 04:15:43 EST
Jerome, is this a jdt/search bug or should PDE modify its search query to accomodate generics?
Comment 3 Jerome Lanneluc CLA 2006-02-10 04:34:25 EST
Frederic, can you please comment ?
Comment 4 Frederic Fusier CLA 2006-02-10 05:07:56 EST
You need to set R_ERASURE_MATCH bit in match rule while creating pattern if you want to get all matches without care about type parameters match...
Then you'll get all type which erasure matches your pattern whatever type parameters you have specified in it...
Comment 5 Matt McCutchen CLA 2006-02-10 17:31:22 EST
(In reply to comment #4)
> You need to set R_ERASURE_MATCH bit in match rule while creating pattern if you
> want to get all matches without care about type parameters match...

True, but I've come to think that's not the right fix; we shouldn't make a kludge in the PDE when we can fix the semantics of JDT search.  This bug should probably be moved to the JDT Core.

If the user does a search for references on the string Foo<T>, obviously only references where the value of the type parameter is T should match.  But searching for references to a given generic type should find all the references to all forms of that type.  The actual name of the type parameter is irrelevant outside the definition of the type except as documentation.  Saying that ``new Foo<T>()'' is a more exact use of the type Foo than ``new Foo<Integer>()'' is like saying ``System.out.println(x)'' is a more exact call to PrintStream.println than ``System.out.println("hello")'' (PrintStream.println's parameter is called x).

Somewhere between
    SearchPattern.createPattern(type, IJavaSearchConstants.REFERENCES)
in DependencyExtentOperation and the resulting pattern Foo<T>, the semantics are wrong.  I'm not sure exactly where, but the methods storeTypeSignaturesInArguments and extractMethodArguments should probably be reconsidered.
Comment 6 Frederic Fusier CLA 2006-02-13 04:39:20 EST
Parallel between parameterized types and standard method argument calls is not valid... With standard methods, bindings are equals, so matches returned by search are exact. Even if you call println with "hello" or x, this is always a String otherwise, you'd get a compiler error...

But parameterized types Foo<Integer> does not have same binding than generic type Foo<T>, so search engine cannot and *should* not return an exact match!

Consider following test case:
class X<T> {
  X<T> xt;
  X<String> xs;
}
Here, field class declaration X<T> has same binding than generic type and so, Search Engine has to return an exact match. It has also to return an erasure match for X<String> because its binding is neither the same nor equivalent to X<T>...

Note however, that there's currently a problem in this area as Search Engine still returns an erasure match for X<T> but this is already referenced by bug 116459.

So, we won't modify Search Engine behavior with generics, please use R_ERASURE_MATCH to get all match. This behavior has been specified with JDT-UI for refactoring purposes, you use cas seems not to be so different.
Comment 7 Matt McCutchen CLA 2006-02-13 10:22:19 EST
(In reply to comment #6)
> Parallel between parameterized types and standard method argument calls is not
> valid... With standard methods, bindings are equals, so matches returned by
> search are exact. Even if you call println with "hello" or x, this is always a
> String otherwise, you'd get a compiler error...

True, all calls to the same method involve the same types while uses of the same generic class will generally involve different types.  However, methods and generic class definitions are similar in that both have a parameter (whether a type or value parameter) whose scope is the definition.  I argue that it makes no sense to compare the declared name of the parameter with uses of the method or generic class outside the definition.

> class X<T> {
>   X<T> xt;
>   X<String> xs;
> }

I'm not suggesting you change that behavior.  If you search for X<T>, of course you should find X<T> but not X<String>!  But I do think the behavior of
    SearchPattern.createPattern([IType for X], IJavaSearchConstants.REFERENCES)
should be changed.  To me, that line requests a pattern that matches _all_ references to X.  It should create the pattern "R_ERASURE_MATCH for X", not the pattern "X<T>", especially because the name T has no meaning outside the definition of X.

I don't think a plugin should have to be aware of generics in order to do something as simple as find all uses of a class.  Even if the JDT UI has worked around the deficiency so far by passing R_ERASURE_MATCH explicitly, the search engine should be fixed so that future plugins do not encounter this pitfall.
Comment 8 Frederic Fusier CLA 2006-02-21 11:31:51 EST
After discussion here we agreed that your request sounds reasonable.
Unfortunately it has come a little bit too late. 3.2 M5 was API freeze and change this behavior would mean API breakage.

So, I'm afraid PDE needs to live with the workaround for 3.2 to set R_ERASURE_MATCH bit in matchRule. I put this bug in my box to follow-up work on next release when we can change API again.
Comment 9 Frederic Fusier CLA 2006-02-21 11:33:03 EST
Not for 3.2 as this would be API breakage. Please reopen after 3.2 was shipped
Comment 10 Matt McCutchen CLA 2006-04-09 23:50:22 EDT
Created attachment 38116 [details]
seems to fix the bug by deleting code that puts <T> in pattern

I think I have a fix.  Simply deleting the section of storeTypeSignaturesAndArguments that tacks the <T> on the pattern for Foo seems to do the trick, but it may break something else.  (I tried to run the test suite for JDT model, but I had some trouble.)

The attached patch does three things:

(1) Deletes the section from storeTypeSignaturesAndArguments.

(2) Makes an analogous change to extractMethodArguments because I think searches for references to generic methods might suffer a similar problem, even though "Find unused dependencies" doesn't currently search for references to methods.  (But it might have to: see bug 135773.)

(3) In some places where parameterized types were treated specially because they carried generic information, it seemed that raw types should also be treated specially.  If this change doesn't make sense, please leave it out as it doesn't seem to be necessary to fix the bug.

I would appreciate a response as to whether the fix works, even though it would not be committed until after Eclipse 3.2.
Comment 11 Frederic Fusier CLA 2006-04-10 02:47:50 EDT
The fix does not work and running RunJavaSearchTests result with 82 failures.
This is not really surprising me as if you remove this part, then Search Engine has no way to distinguish raw, equivalent and exact matches.

The fix is quite more simple, just set R_ERASURE_MATCH as default... and this will be done after 3.2.
In SearchPattern, I'll change createPatter(int,int) method from:
public static SearchPattern createPattern(IJavaElement element, int limitTo) {
  return createPattern(element, limitTo, R_EXACT_MATCH | R_CASE_SENSITIVE);
}

to:
public static SearchPattern createPattern(IJavaElement element, int limitTo) {
  return createPattern(element,
    limitTo,
    R_EXACT_MATCH | R_CASE_SENSITIVE | R_ERASURE_MATCH);
}

and you'll get your expected matches wihtout changing their raw, equivalent and exact flavor and so, pass all generic search tests.
Comment 12 Matt McCutchen CLA 2006-04-10 10:18:26 EDT
OK, that fix makes more sense.  You can see I don't understand the internals of the search very well.  Thanks.
Comment 13 Frederic Fusier CLA 2006-07-05 11:30:15 EDT
Reopen for 3.3
Comment 14 Frederic Fusier CLA 2006-08-03 12:28:47 EDT
Created attachment 47342 [details]
Proposed patch

This patch also contains fix for bug 116459...
Comment 15 Frederic Fusier CLA 2006-08-03 13:21:59 EDT
Released for 3.3 M1 in HEAD
Comment 16 Matt McCutchen CLA 2006-08-03 19:10:09 EDT
Thank you very much!
Comment 17 Maxime Daniel CLA 2006-08-04 09:58:16 EDT
Verified for 3.3 M1 using build I20060804-0010.