Bugzilla – Full Text Bug Listing
| Summary: | [1.7][assist] Completion issues with multicatch (FUP of 343637) | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Srikanth Sankaran <srikanth_sankaran> | ||||||||
| Component: | Core | Assignee: | Ayushman Jain <amj87.iitr> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | raksha.vasisht, satyam.kandula | ||||||||
| Version: | 3.7 | Flags: | satyam.kandula:
review+
|
||||||||
| Target Milestone: | 3.7.1 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Srikanth Sankaran
Ayush, please follow up - TIA. Created attachment 198965 [details]
proposed fix v1.0 + regression tests
This patch does the following:
1) Ensure annotations and interfaces are not proposed inside any catch. This is done by the checks introduced in findType... methods. This only helps when the annotations and interfaces are in the same CU though. To handle the case when they're in a different CU, line 10458 makes sure that only classes are searched for when the assist node is an exception.
This filtration also removes:
1a) Annotations and interfaces declared as members of a type.
1b) Annotations and interfaces coming from static imports.
2) Ensure super/sub types of already caught exceptions in a multi-catch statement are filtered off. (Note:For multiple catch blocks, proposing a supertype when a subtype has already been caught is still allowed). This is ensured by setting the forbiddenBindingsFilter appropriately. An extra step is needed to make sure this also works for proposals coming from a different CU. This is done in org.eclipse.jdt.internal.codeassist.CompletionEngine.isForbiddenType(char[], char[], char[][]). A binding had to be constructed corresponding to the qualified name to perform the filtration.
3) When there is no prefix before the caret, we try to propose types from the actually thrown types in the try block. So the patch makes sure that even in these cases, supertypes and subtypes are not proposed in a multi-catch and that subtypes are not proposed for multiple catch blocks. This is done in org.eclipse.jdt.internal.codeassist.CompletionEngine.findTypesFromExpectedTypes(char[], Scope, ObjectVector, boolean, boolean)
Tests have been added for all scenarios.
Srikanth, can you please review? Thanks! Satyam, can you review this in my lieu please ? TIA. Ayush, It is nice to see many tests! Here are my comments: I think it is fine to show the super types in the multicatch arguments. A user may want to really catch a super type, but doesn't know the relationship with the other types and so he will want to use it. He will delete one of them when the compiler reports an error and I guess that is fine. The proposal should probably come below but I think it should not be forbidden. 2. As discussed, it is not a good idea to get the bindings for all the types that search returns. It will impact the performance very badly. 3. In findTypesAndPackages() and similar places, the checks can probably be combined with the preceding if(s). I believe just checking for !isClass() is good enough rather than search for isInterface() and isAnnotation(). Created attachment 199191 [details] proposed fix v2.0 + regression tests In consultation with Satyam, we decided that we should losen, and not tighten the constraints about proposing exceptions in either multi-catch or subsequent catch statements. Super/sub types should be proposed, albeit with lower relevance. This makes sure that: (a) A user who is catching a type A might have actually wanted to catch a parent/child type B, and using content assist when he inserts B, realises he didn't need A at the first place and goes ahead and deletes it. This flexibility is better than confounding the user by not proposing B at all. (b) It keeps performance hit for content assist at a minimum. Keeping that in mind, I have reprogrammed the solution offered above to only fix Srikanth's point (1) in comment 0. I have then eased up the constraints that we had earlier imposed via bug 343637. So now you will see all super/sub types being proposed if an exception is already in one of the catch blocks or has appeared in the current catch block. I have also not enforced the relevance lowering too much for types in different CU's so as to make sure there's no performance hit. Only exact matches coming from different CU's and also in CompletionEngine$uninterestingBindings are given low relevance. There's no sub/super type analysis done here. Ayush,
Some more minor comments:
1. I believe the super types can be removed from the thrown exception list too.
2. In findTypes and Packages, the code can be modified as under.
if (this.assistNodeIsException) {
if (!sourceType.isClass()) continue next;
if (isEmptyPrefix) {
if (sourceType.findSuperTypeOriginatingFrom(TypeIds.T_JavaLangThrowable, true) == null) {
continue next;
}
}
}
3. Don't forget to uncomment OperationCancelled.
Leaving off these minor comments, I like the changes.
Created attachment 199398 [details]
proposed fix v2.1 + regression tests
Takes care of above comments
Released in BETA_JAVA7 branch Verifying... (In reply to comment #6) > Keeping that in mind, I have reprogrammed the solution offered above to only > fix Srikanth's point (1) in comment 0. I have then eased up the constraints > that we had earlier imposed via bug 343637. So now you will see all super/sub > types being proposed if an exception is already in one of the catch blocks or > has appeared in the current catch block. But I see that the same exception is being shown in the list whether it is already caught in another catch block or even in the current catch block class MultiCatch { void foo() { try { //... throw new Exception(); //... } catch (NullPointerException r ) { } catch () { } } } class SuperException extends Exception {} class SubException extends SuperException {} 1) Insert a | after NPE in the first catch block and hit ctrl+space, You'll see NPE again in the proposal list (first one) 2) Hit ctrl+space in the second catch block , you'll see NPE in the list. => We should not show the Exceptions already caught in same or in other catch blocks. Verified. (In reply to comment #11) See bug 352556. |