Community
Participate
Working Groups
public class Try18 { public static void main(String[] args) { n| } } - Press ctrl+space after 'n'. => Proposal for 'main(..)' (substring proposal) has a higher relevance than some other Types starting with 'n'. ---------------------------- public class Try18 { public static void main(String[] args) { y| } } - Press ctrl+space after 'y'. => Proposal for 'Try18' (substring proposal) has a higher relevance than other Types starting with 'y'. ---------------------------- See screenshot in bug 487668 comment #0. Substring proposals should always be less relevant than applicable prefix proposals.
Quick glance, I see this: Relevance for Try18: BASE + RESOLVED + INTERESTING + SUBSTRING + UNQUALIFIED + NON_RESTRICTED (5 + 1 + 5 + (-1) + 3 + 3 = 16) And for YearMonth: BASE + RESOLVED + INTERESTING + NON_RESTRICTED (5 + 1 + 5 + 3 = 14) The difference is the UNQUALIFIED. Manoj, should we bump the relevance constant value for SUBSTRING? Even if we do, we need to weight it such that it neither undermines too much nor overwhelming depending on what we compare it with :)
Note: Jay would like to take this one up as he already has history of the dependent bug and has started to look at this (as discussed over private note) - assigning to Jay
(In reply to Jay Arthanareeswaran from comment #1) > Quick glance, I see this: > > Relevance for Try18: > > BASE + RESOLVED + INTERESTING + SUBSTRING + UNQUALIFIED + NON_RESTRICTED (5 > + 1 + 5 + (-1) + 3 + 3 = 16) > > And for YearMonth: > > BASE + RESOLVED + INTERESTING + NON_RESTRICTED (5 + 1 + 5 + 3 = 14) > > The difference is the UNQUALIFIED. > > Manoj, should we bump the relevance constant value for SUBSTRING? Even if we > do, we need to weight it such that it neither undermines too much nor > overwhelming depending on what we compare it with :) Jay: I think bumping up alone will not be a long term solution - in fact, I don't think a direct addition would satisfy the jdt.ui's requirement. quoting from comment 0,"Substring proposals should always be less relevant than applicable prefix proposals" -> this has an impllicit requirement hidden - that the proposals themselves need to be bucketized - A simple addition might give out mixed results - as you pointed out. [Not thinking of the external dependencies ] if we need to maintain a strict order within the proposals, then we would have to say Prefix starts at 200, Suffix Starts at 100, there by introducing a "base+offset" buckets where the prefix would always (well almost always) have a higher relevance than suffix. Going forward this base+offset may be a simpler yet effective way to enforce strict boundaries of relevance.
See bug 481752 comment #4 also: "it would be OK to make substring matches more relevant if their result is assignable in the context."
(In reply to Noopur Gupta from comment #4) > See bug 481752 comment #4 also: "it would be OK to make substring matches > more relevant if their result is assignable in the context." Well, we need to hit a common ground here :) - in other words, Is it better to make it more relevant in the above context - or is it that it doesn't matter?
(In reply to Noopur Gupta from comment #4) > See bug 481752 comment #4 also: "it would be OK to make substring matches > more relevant if their result is assignable in the context." Example: "s".st| => prefix proposals have higher relevance. int i= "s".st| => substring proposals assignable in the context have higher relevance.
I don't see an easy way out. We need to come up with a long term fix considering all possible scenarios. Moving out of 4.6.
Just to add another case here - if substring proposals as well as prefix proposals are assignable in the context, prefix proposals should still be more relevant. E.g. String msg=""; String[] parameters = {"a", "b", "c"}; System.out.println(msg+Arrays.as|); => the first proposal I got was for Arrays.deepHashCode. Expected proposal would have been Arrays.asList.
So, to summarize, here's what we expect: Prefix proposals should always have more relevance than substring except when the substring proposal is assignable in the context and the prefix proposal isn't assignable in the context. And what about camel case matches in comparison?
I guess the substring offset should be just enough to offset the R_EXPECTED_TYPE but significantly less than R_EXACT_EXPECTED_TYPE. (In reply to Carsten Reckord from comment #8) > String msg=""; > String[] parameters = {"a", "b", "c"}; > > System.out.println(msg+Arrays.as|); > > => the first proposal I got was for Arrays.deepHashCode. Expected proposal > would have been Arrays.asList. With my proposal above, it would continue to be the same. Because one of the exact expected type for the second operand in "msg + " is INT. So, all the methods that return INT would be listed ahead of asList. However, the following case would be different: System.out.println(Arrays.as); Here asList would be the first proposal. Sounds good?
So, the change that looks like will work for me is: R_SUBSTRING -1 => -20 and R_DEFAULT 5 => 30 But looks like the challenge is going to be the hard-coded relevance. Worse, some testcases use some other combination of constants that produce the same relevance. This is going to be a nightmare to fix them all.
(In reply to Jay Arthanareeswaran from comment #9) > And what about camel case matches in comparison? Camel case matches will be applicable only when a camel case pattern is typed. In that case, it should behave the same as before i.e. camel case matches should get higher priority. (In reply to Jay Arthanareeswaran from comment #10) > (In reply to Carsten Reckord from comment #8) > > String msg=""; > > String[] parameters = {"a", "b", "c"}; > > > > System.out.println(msg+Arrays.as|); > > > > => the first proposal I got was for Arrays.deepHashCode. Expected proposal > > would have been Arrays.asList. > > With my proposal above, it would continue to be the same. Because one of the > exact expected type for the second operand in "msg + " is INT. So, all the > methods that return INT would be listed ahead of asList. As summarized here: (In reply to Jay Arthanareeswaran from comment #9) > So, to summarize, here's what we expect: > > Prefix proposals should always have more relevance than substring except > when the substring proposal is assignable in the context and the prefix > proposal isn't assignable in the context. In the above case, both substring and prefix proposals are assignable in the context. So the prefix proposal (asList) should get higher relevance.
(In reply to Noopur Gupta from comment #12) > In the above case, both substring and prefix proposals are assignable in the > context. So the prefix proposal (asList) should get higher relevance. There is a clear distinction in our code between R_EXPECTED_TYPE and R_EXACT_EXPECTED_TYPE. Add to that, we don't differentiate between that case and the following: int i = Arrays.as| and I would imagine that hashCode() should get higher relevance here.
I have pushed the test changes as prep work in master. This gets rid of hard-coded value for relevant constant R_DEFAULT.
Created attachment 263706 [details] Proposed fix + Test The change is all about slightly modified relevance constant values. Note that in certain case, a prefix and substring proposals will get the same relevance where it boils down to alphabetic order. For e.g. this case asList() gets proposed first ahead of hashcode purely based on alphabetical order: System.out.println(msg + Arrays.as|) However, in the following case asLIst() gets pushed down: System.out.println(msg + Arrays.AS) because it doesn't get the relevance for case matching.
All JDT Core and UI tests pass. Released the fix here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=a9d318d978e4e9e6f3804e5aae1e81ed6e4a0136 Also requesting Manoj for review for 4.6.1.
Manoj is away at a conference, but he confirmed to me that he verified the fix on master as well as reviewed the patch. Subsequently I have pushed the change in R4_6_maintenance: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=R4_6_maintenance&id=8e5f1dd5da000ba40b2faf1025b6a742b6cb7588
Verified for 4.6.1 RC2 using M20160824-0059 build. Just one observation though, comment 13 says hashCode should have higher relevance for the example mentioned there, (and I agree with that), but asList appears at the top of proposals. Not sure if that was implemented, but just thought may warrant another bug
Verified for 4.7 using I20160824-1429 build
(In reply to Sasikanth Bharadwaj from comment #18) > Verified for 4.6.1 RC2 using M20160824-0059 build. Just one observation > though, comment 13 says hashCode should have higher relevance for the > example mentioned there, (and I agree with that), but asList appears at the > top of proposals. Not sure if that was implemented, but just thought may > warrant another bug I see #hashCode getting a higher relevance compared to #asList.
