Bug 100772 - [1.5][search] Search for declarations in hierarchy reports too many matches
Summary: [1.5][search] Search for declarations in hierarchy reports too many matches
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.1   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.1.1   Edit
Assignee: Frederic Fusier CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-06-20 05:11 EDT by Dirk Baeumer CLA
Modified: 2005-09-26 10:51 EDT (History)
0 users

See Also:


Attachments
Patch to fix this issue (5.62 KB, patch)
2005-06-21 12:28 EDT, Frederic Fusier CLA
no flags Details | Diff
Complete patch to fix this issue (17.00 KB, patch)
2005-07-08 07:11 EDT, Frederic Fusier CLA
no flags Details | Diff
Test cases added to JavaSearchBugsTests (21.72 KB, patch)
2005-07-08 07:30 EDT, Frederic Fusier CLA
no flags Details | Diff
Additional changes which fix other issues (5.10 KB, patch)
2005-07-08 12:04 EDT, Frederic Fusier CLA
no flags Details | Diff
Addtional test cases on JavaSearchBugsTests (10.37 KB, patch)
2005-07-08 12:05 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 Dirk Baeumer CLA 2005-06-20 05:11:28 EDT
RC3

public class A<T> {
	public void foo(T t) {
	}
}

class B extends A<String> {
	public void foo(String s) {
	}
	public void foo(Integer i) {
	}
}

- search for declaration in hierarchy on foo(Integer i) using the context
  menu.

observe that foo(String s) is reported as a match as well.

Please note that search declaration in hierarchy specifies the two flags 
IJavaSearchConstants.IGNORE_DECLARING_TYPE |
IJavaSearchConstants.IGNORE_RETURN_TYPE.

However, using the search dialog (Ctrl+H) which uses the Java element without
any flags and search for declarations in workspace reports foo(String s) as well. 

If B doesn't extend A<String> foo(String s) isn't found.
Comment 1 Frederic Fusier CLA 2005-06-21 12:28:25 EDT
Created attachment 23641 [details]
Patch to fix this issue

While looking for possible overridden method in pattern hierarchy, we still
need to verify that matching ones have same parameters than pattern.
In bug test case, method foo(String) was a method which may override but
parameters do not match pattern and must be filtered...

Fix not trivial (but not too complicated) and very localized.
It will not have too much impact on existing search behavior.
As this is not critical, I think it's a good candidate for 3.1.1
Comment 2 Philipe Mulet CLA 2005-06-21 12:34:41 EDT
Dirk - do you agree it can wait until 3.1.1 ?
Comment 3 Frederic Fusier CLA 2005-07-08 07:11:08 EDT
Created attachment 24462 [details]
Complete patch to fix this issue

Previous patch was not enough to fix the problem entirely.

In fact there was a mixed in initial implementation (bug 79990, bug 96761 and
bug 96763) between method which overrides possible match and this which must
match pattern. So, previous implementation (including previous proposed patch)
was too permissive.

This new patch fixes definitely the problem.
New algorithm is:
1) If compiler is in 1.5 mode, store method declaration which parameters types
do no match pattern and set them as possible match
2) Before report match, for stored method declarations only:
2.1) Look in method declaring class hierarchy to see if there are overridden
     methods in parameterized super classes or super interfaces.
     Report match if one of these overridden methods has its original method
     which equals the pattern.
2.2) If method declaration declaring class is in pattern super types hierarchy
     then look in pattern declaring class hierarchy to see if there are
     overridden method in parameterized super classes or super interfaces.
     Report match if one of these overridden methods has its original method
     which equals the method declaration.

Basic sample is:
class X<T> {
  void foo(T t) {}
  void foo(Class c) {}
}
class Y extends Y<String> {
  void foo(String s) {}
  void foo(Exception e) {}
}

Search for declaration in hierarchy for X.foo(T t) should return 2 matches:
X.foo(T t) and Y.foo(String s)
X.foo(T t) matches pattern, so no problem, but Y.foo(String s) does not!
With implemented algorithm, 1) make all "foo" methods possibles matches.
- X.foo(Class c) and Y.foo(Exception e) will be not be reported because they
fail 2.1) and 2.2).
- Y.foo(String s) will be reported because it passes 2.1).

Search for declaration in hierarchy for Y.foo(String) should also return 2
matches: X.foo(T t) and Y.foo(String s)
Y.foo(String s) matches pattern, so no problem, but X.foo(T t) does not!
With implemented algorithm, 1) make all "foo" methods possibles matches.
- X.foo(T t) will be reported because it passes 2.2).
- X.foo(Class c) and Y.foo(Exception e) will be not be reported because they
fail 2.1) and 2.2).

Search for declaration in hierarchy for Y.foo(Exception) (e.g. comment 0 test
case) should return a single match: Y.foo(Exception e)
Only Y.foo(Exception e) matches pattern, but with new algorithm, 1) make all
"foo" methods possibles matches.
- X.foo(T t) and X.foo(Class c) will be not be reported because they fail 2.1)
and 2.2): neither pattern nor possible matches override any method in
hierarchy...

Note that this patch also includes fix for bug 100695...
Comment 4 Frederic Fusier CLA 2005-07-08 07:30:22 EDT
Created attachment 24463 [details]
Test cases added to JavaSearchBugsTests

This patch also includes test cases for bug 100695
Comment 5 Frederic Fusier CLA 2005-07-08 07:30:39 EDT
Fixed and released in R3_1_maintenance.
Comment 6 Frederic Fusier CLA 2005-07-08 12:04:24 EDT
Created attachment 24476 [details]
Additional changes which fix other issues

Clear locator caches was done too early and made following test case fail:
I.java
interface I<T> {
  void foo(T t);
}
X.java
class X<T> {
  void foo(T t) {}
}
class Y extends X<String> {
  void foo(String s) {}
  void foo(Exception s) {}
}

While searching for Y.foo(String) declaration in project (and not in hierarchy)
then Y.foo(Exception) was found!

Another test case was failing and is also fixed with this patch:
A.java
class <T> {
  void foo(T t);
}
class B extends A<String> {
  void foo(String s) {}
}
X.java
class X<T> {
  void foo(T t) {}
}
class Y extends X<String> {
  void foo(String s) {}
}

Search for declaration of B.foo(String) in project reported only A.foo(T t),
B.foo(String s) and Y.foo(String s) matches.
However, search for declaration of A.foo(T) in project correctly reported
A.foo(T t), B.foo(String s), X.foo(T t) and Y.foo(String s).

With this additional patch, search for declaration of B.foo(String) in project
reports same matches (ie. 4) than search for declaration of A.foo(T)...
Comment 7 Frederic Fusier CLA 2005-07-08 12:05:05 EDT
Created attachment 24477 [details]
Addtional test cases on JavaSearchBugsTests
Comment 8 Frederic Fusier CLA 2005-07-08 13:06:42 EDT
Patches of additional changes released in R3_1_maintenance.
Comment 9 Frederic Fusier CLA 2005-08-03 10:20:24 EDT
Backported to HEAD stream.
Comment 10 Olivier Thomann CLA 2005-08-09 10:25:26 EDT
Reopen.
From comment 6,  void foo(Exception s) is still reported as a match.
Comment 11 Frederic Fusier CLA 2005-08-15 15:07:04 EDT
Fixed and released in HEAD stream.
Comment 12 Frederic Fusier CLA 2005-08-16 10:28:01 EDT
No change done in R3_1_maintenance stream. Problem occurred during the backport
to HEAD stream => back to FIXED status
Comment 13 Olivier Thomann CLA 2005-09-20 14:43:12 EDT
Verified using I20050920-0010 for 3.2M2
Comment 14 Maxime Daniel CLA 2005-09-26 10:18:04 EDT
Verified for 3.1.1 using build M20050923-1430.