Community
Participate
Working Groups
IllegalArgumentException is NOT offered in the second catch block => Good ------------------------------------------------------------------ class MultiCatch { void foo() { try { } catch (IllegalArgumentException e) { } catch (IAE<Ctrl+Space>) } } ------------------------------------------------------------------ IllegalArgumentException IS offered again => Bad ------------------------------------------------------------------ class MultiCatch { void foo() { try { } catch (IllegalArgumentException | IAE<Ctrl+Space>) { } } } -----------------------------------------------------------------
Created attachment 196106 [details] proposed fix v1.0 + regression tests There are 2 parts to the fix: 1) Making sure that in the completion node parent we create, we include the whole disjunctive type in the catch arguments and not just the completionOnException node. This is done in CompletionParser 2) Making sure that the exceptions already declared in the disjunctive type ref uptil now are added to the forbidden bindings in the completion engine, so that they can be filtered off from the list of expected types. The CompletionEngine.computeForbiddenBindings() has this change. In the normal scenario with multiple catch blocks, the already caught exceptions are filtered via org.eclipse.jdt.internal.codeassist.ThrownExceptionFinder.find(TryStatement, BlockScope) which is called from computeExpectedTypes(). But I noticed that this is a duplication of effort since we're already adding the caught exceptions into the forbidden bindings. So i removed the call to org.eclipse.jdt.internal.codeassist.ThrownExceptionFinder.removeCaughtExceptions(TryStatement) made inside the find() method. Instead, the if (!isForbidden(bindings[i])) check has been added in computeExpectedTypes().
Satyam, please review. Thanks!
(In reply to comment #1) FYI, You will find out but DisjunctiveTypeReference has been renamed to UnionTypeReference. removeCaughtExceptions() is getting called even after you had removed it. The one you had removed is a duplicate call and it is still getting called through the visitor now. I think the removeCaughtExceptions() will be needed. I also think the check for UnionTypeReference should be done in ThrowExceptionFinder#removeCaughtExceptions(). This could help if an inner try/catch block catches an union type. If this is done, I don't think the added isForbidden() check is needed.
(In reply to comment #3) > (In reply to comment #1) > FYI, You will find out but DisjunctiveTypeReference has been renamed to > UnionTypeReference. Yeah, I'm aware. > removeCaughtExceptions() is getting called even after you had removed it. The > one you had removed is a duplicate call and it is still getting called through > the visitor now. Yes, I know. Thats why the inner call was sufficient and i removed the other one. > I think the removeCaughtExceptions() will be needed. I also > think the check for UnionTypeReference should be done in > ThrowExceptionFinder#removeCaughtExceptions(). This could help if an inner > try/catch block catches an union type. You're right. I'll make this change and release the patch.
(In reply to comment #4) Sure, make the changes and release.
Created attachment 196335 [details] proposed fix v1.1 + regression tests This fix changes the way the forbidden bindings are computed and also fixes many existing problems in the way computeForbiddenBindings() and ThrownExceptionFinder together try to filter out exceptions. These are shown by newly added tests testBug343637c() to f. Besides making sure that the existing functionality is preserved the new patch tries to do the following: (1) It makes sure if an inner catch block contains disjunctive type ref, all 'checked' exceptions from it are not proposed in an outer catch, but unchecked ones can be proposed again. Even in single catch case, whatever's caught in the inner catch block does not get proposed again, unless its an unchecked exception. The new overloaded method ThrownExceptionFinder.removeCaughtExceptions() takes care of only filtering unchecked exceptions from the current context (2) Even when the superclass of a thrown exception is caught, the thrown exception is not proposed again. (3) Even an exception that is not thrown by a method but has been caught is not proposed again.
Satyam, request you to take a look once again before I release.
Btw, there's no new change in CompletionParser and AssistParser
I liked the idea of getting all the thrown and caught exception from ThrowsExceptionFinder. Here are small issues that you would want to fix. 1. In removeCaughtExceptions(), the special processing should also done for non-union types. 2. removeCaughtExceptions(TryStatement tryStatement) is not needed. 3. I don't like the idea of returning a 2-dimensional array from ThrowsExceptionFinder#find(). As these are anyways members of the class, these values could be fetched after the call to find(). Maybe the function name find() could be changed for a better name!
Created attachment 196463 [details] proposed fix v1.2 + regression tests (In reply to comment #9) > Here are small issues that you would want to fix. > 1. In removeCaughtExceptions(), the special processing should also done for > non-union types. Done, Also added 2 more tests. > 2. removeCaughtExceptions(TryStatement tryStatement) is not needed. Done. > 3. I don't like the idea of returning a 2-dimensional array from > ThrowsExceptionFinder#find(). As these are anyways members of the class, these > values could be fetched after the call to find(). Maybe the function name > find() could be changed for a better name! Done. Now another function getAlreadyCaughtExceptions() returns the array of caught exceptions. Changed the name of find to findThrownExceptions
Created attachment 196512 [details] released patch Minor change. Created a new function in ThrownExceptionFinder to return the thrown exceptions separately.
Released in BETA_JAVA7.
Verified using "Eclipse Java Development Tools Patch for Java 7 Support (BETA) 1.0.0.v20110623-0900 org.eclipse.jdt.patch.feature.group Eclipse.org"
