Bug 348860

Comment 1 Deepak Azad 2011-06-16 11:38:18 EDT

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

Comment 2 Deepak Azad 2011-06-16 11:39:08 EDT

Markus, the new quick fixes look ok to you ?

Comment 3 Deepak Azad 2011-06-24 13:05:25 EDT

Created attachment 198552 [details]
fix + tests

Last patch got outdated.

Comment 4 Markus Keller 2011-06-24 14:15:56 EDT

(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.

Comment 5 Markus Keller 2011-06-24 14:18:42 EDT

And the quick assist doesn't appear when I have a fully-qualified exception name, like java.lang.IllegalArgumentException.

Comment 6 Deepak Azad 2011-06-24 14:25:46 EDT

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 ?

Comment 7 Markus Keller 2011-06-24 14:46:33 EDT

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.

Comment 8 Ayushman Jain 2011-06-25 04:10:54 EDT

(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.

Comment 9 Deepak Azad 2011-06-28 10:59:46 EDT

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 ?

Comment 10 Markus Keller 2011-06-29 14:17:47 EDT

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
		}

Comment 11 Deepak Azad 2011-07-06 22:45:39 EDT

Created attachment 199219 [details]
final fix + tests

The patch also contains fix for bug 350713 and bug 350311.

Comment 12 Deepak Azad 2011-07-06 22:46:16 EDT

Fixed in BETA_JAVA7.

Comment 13 Satyam Kandula 2011-07-19 11:46:56 EDT

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.

Comment 14 Satyam Kandula 2011-07-19 11:50:54 EDT

Verified using patch 1.0.0.v20110714-1400