Community
Participate
Working Groups
BETA_JAVA7, follow-up to bug 348402 In an 1.7 or earlier project, have: package multicatch; import java.io.FileNotFoundException; import java.io.IOException; import java.io.InterruptedIOException; class MultiCatch { void foo() { try { throw new FileNotFoundException(); } catch (FileNotFoundException | IOException ex) { } } } => Error "The exception FileNotFoundException is already caught by the alternative IOException". Add a quick fix to remove the redundant caught type.
Created attachment 198121 [details] fix + tests So far we had the following 2 quick fixes/assists for unneeded catch blocks - Remove catch clause - Replace catch clause with throws In this patch I have added the following 2 quick fixes/assists for unneeded exceptions in a multi-catch block. These new quick fixes have higher relevance than the above 2 quick fixes. - Remove exception - Replace exception with throws
Markus, the new quick fixes look ok to you ?
Created attachment 198552 [details] fix + tests Last patch got outdated.
(In reply to comment #1) > Created attachment 198121 [details] [diff] > fix + tests Looks promising, but the patch applies too many formatting changes if I remove an exception here: try { String.class.getConstructor().newInstance(); } catch (InstantiationException | IllegalAccessException | IllegalArgumentException | InvocationTargetException | NoSuchMethodException | SecurityException e) { // TODO Auto-generated catch block e.printStackTrace(); } QuickAssistProcessor#removeException(..) should use a ListRewrite instead of creating a new UnionType node. See the two methods below for how to do that.
And the quick assist doesn't appear when I have a fully-qualified exception name, like java.lang.IllegalArgumentException.
Are you happy with offering '4 proposals' in the multi-catch case? I was not so sure about offering 'Remove catch clause' and 'Replace catch clause with throws' in the multi-catch case. Can we offer only two fixes all the time - Remove - Replace with throws and just do the right thing ? A similar issue is there in bug 349665. Do we offer 2 quick assists or simply use a multi-catch block if more than one exception is thrown ?
If we know what "do the right thing" is, then that's always preferred. But the user could really want either of the proposals. But I was also a bit uneasy about the flood of proposals, so I guess it would be best to hide the 'catch clause' proposals when an 'exception' proposal is there. > A similar issue is there in bug 349665. Do we offer 2 quick assists or simply > use a multi-catch block if more than one exception is thrown ? There, my gut feeling still says we should offer both. Especially since offering only the multi-catch one would remove an existing feature. It's true that converting is easy now with the quick assists, but it still a manual step, and not everyone might find the QA.
(In reply to comment #6) > Are you happy with offering '4 proposals' in the multi-catch case? I was not so > sure about offering 'Remove catch clause' and 'Replace catch clause with > throws' in the multi-catch case. > > Can we offer only two fixes all the time > - Remove > - Replace with throws > and just do the right thing ? I also think its not a good idea to offer the quick fixes with "catch clause". Earlier, we would have separate catch clauses for each exception and in that scenario it made sense to talk about a "catch clause", but now with many exceptions within the same catch clause (i.e. a multi-catch clause), those quick fixes cause more confusion to the user. "Will this quick fix remove the complete multi catch clause?" is a probable question that may crop up in his/her mind.
Created attachment 198743 [details] fix + tests Ok, this solution makes me happy! - Quick fix: When there is an error only 2 proposals are offered i.e. for the example mentioned in comment 0 you will see only "Remove exception" and "Replace exception with throws". - Quick assist: When there is no error all 4 are offered, as the user could be looking for any one of the 4 proposals. Also fixed the problems from comment 4 and comment 5. Markus, looks ok to you ?
I still don't see the "Remove exception" proposal on a fully-qualified exception, e.g. when I change comment 4 to use java.lang.IllegalAccessException and then try to remove only that exception with the caret e.g. after "Illegal". I like the behavior when there is an error, but I still think showing all 4 proposals is too much e.g. in comment 4. I'd like to see: - "Remove exception" and "Replace exception with throws" iff a single exception of a multi-catch block is selected - "Remove catch clause" and "Replace catch clause with throws" otherwise Implementation hint: SimpleType selectedMultiCatchType= null; if (type.isUnionType() && node instanceof Name) { Name topMostName= ASTNodes.getTopMostName((Name) node); ASTNode parent= topMostName.getParent(); if (parent instanceof SimpleType) { selectedMultiCatchType= (SimpleType) parent; } } Then you can just do if (selectedMultiCatchType != null) { // new impl to remove/replace single exception } else { // old impl }
Created attachment 199219 [details] final fix + tests The patch also contains fix for bug 350713 and bug 350311.
Fixed in BETA_JAVA7.
I personally think there should be a quick fix to separate the catch argument where there is an error. I had filed bug 352477 before reading this bug.
Verified using patch 1.0.0.v20110714-1400
