Bug 273991

Summary:

[assist] Wrong relevance for some proposals which are not compatible with the expected type

Product:

[Eclipse Project] JDT

Reporter:

David Audel <david_audel>

Component:

Core

Assignee:

Jay Arthanareeswaran <jarthana>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

Olivier_Thomann

Version:

3.5

Flags:

frederic_fusier: review-
Olivier_Thomann: review+
kent_johnson: review+

Target Milestone:

3.5 RC2

Hardware:

OS:

Windows XP

Whiteboard:

Attachments:

Description	Flags
Proposed patch	none
Patch with updated tests	none
Patch with updated tests	david_audel: iplog+, david_audel: review+
Updated patch	david_audel: iplog+, david_audel: review+

Description David Audel

2009-04-28 05:11:48 EDT

build I20090427-1800

While verifying bug 84720, i noticed that some proposals have a too higher relevance value.

1) create Test.java
public class Test {
	String foo0 () {return null;}
	float foo1 () {return 0;}
	int foo2() {return 0;}
	Long foo3() {return null;}
	long foo4() {return 0;}
	
	void test() {
		Long i = foo|
	}
}

2) do ctrl+space at |
foo1() has the same relevance as foo4() but float is not compatible with Long.
foo1() should have the same relevance as foo0().

Comment 1 Jay Arthanareeswaran

2009-05-11 01:24:01 EDT

Created attachment 135101 [details]
Proposed patch

Swapped the parameters to the isBoxingCompatibleWith method invocation, which takes care of the incompatibility issue. Also modified the previous test cases to test the given cases.

Comment 2 David Audel

2009-05-11 08:43:37 EDT

The fix looks good but the modified regression test is not equivalent to the previous version.
The previous has a base type (int) as expected type and the new a Class (Long) as expected type. Both tests would be useful, so i would keep both.

Comment 3 Jay Arthanareeswaran

2009-05-13 01:40:36 EDT

Created attachment 135507 [details]
Patch with updated tests

There was an existing test case that had the expected type as a wrapper. I modified that to primitive. Now we have tests both with primitive and wrapper class as expected types. Hope this serves.

Comment 4 Jay Arthanareeswaran

2009-05-13 04:55:43 EDT

Created attachment 135535 [details]
Patch with updated tests

Retaining the older tests and their order as they are.

Comment 5 David Audel

2009-05-13 07:17:49 EDT

Comment on attachment 135535 [details]
Patch with updated tests

patch looks good

Comment 6 David Audel

2009-05-13 07:19:54 EDT

Frederic - Could you review the patch ?

Comment 7 Frederic Fusier

2009-05-13 07:52:53 EDT

(In reply to comment #6)
> Frederic - Could you review the patch ?
> 
The change in CompletionEngine looks good, but the test seems wrong to me:
1) the int<->Integer auto-boxing should not have been removed
2) the introduced Long and Double classes are not declared in the test case,
   hence may it slightly different than the initial one...

Comment 8 Jay Arthanareeswaran

2009-05-14 05:07:00 EDT

Created attachment 135757 [details]
Updated patch

Fred's suggestions included in the tests. Some of the redundant tests have been removed to keep the test simpler and at the same time retaining the Long as the expected type to keep the test case as close to the bug's description as possible.

Comment 9 David Audel

2009-05-20 10:29:19 EDT

Olivier, could review the last patch ?

Comment 10 David Audel

2009-05-20 10:30:06 EDT

Kent, could you review the last patch ?

Comment 11 Olivier Thomann

2009-05-20 11:13:13 EDT

+1. Patch looks good.

Comment 12 David Audel

2009-05-20 11:23:04 EDT

Comment on attachment 135757 [details]
Updated patch

Patch looks good

Comment 13 David Audel

2009-05-20 11:25:04 EDT

Released for 3.5RC2

Tests updated
  CompletionTests_1_5#testCompletionWithUnboxing_1()

Comment 14 Olivier Thomann

2009-05-22 10:27:53 EDT

Verified for 3.5RC2 using I20090521-2000.