Community
Participate
Working Groups
Build ID: M20090211-1700 Steps To Reproduce: 1.Create a method that throws two Exceptions, one a subclass of the other; 2.Select the method body; Extract Method; 3.Both Exceptions will be listed in the throws clause of the extracted method. More information: public class ExtractMethodRefactoringIssue { private class A extends Exception {}; private class B extends A {}; private void aThrower() throws A {throw new A();} private void bThrower() throws B {throw new B();} private void bothThrower() throws A {extractedBothThrower();} /* * This method, extracted from bothThrower, * includes both A and B in its THROWS clause, * although B is a subclass of A so simply * declaring A would suffice. */ private void extractedBothThrower() throws A, B { aThrower(); bThrower(); } public static void main(String[] args) throws Exception { new ExtractMethodRefactoringIssue().bothThrower(); } }
Move to JDT/UI
I'm not quite sure if this is something the Extract method refactoring should offer. I'd rather expect that the compiler would flag throws A,B as unneccessary and we provide a quickfix to remove it from the declaration. In any case this may not be the expected behavior. Markus, any thoughts?
This may even be marked as dup of bug 6251
Yep, this is a dup of bug 6251, and I agree that Extract Method should just extract the method, and exception folding should be offered as a separate feature. I'll abuse this bug and move it to JDT/Core as a request for a new optional compiler warning for "Redundant thrown exception". If the diagnostic is implemented, we will add a quick fix that just removes the redundant exception.
This would be an enhancement.
Satyam, could you take stab at this ?
While I am trying to work on this, here are my thoughts.. If I take the context independent of this problem -- Should the compiler give any warning if the throws clause has exceptions where one is a subclass of another? I don't see any benefit or need for someone to remove one of these exceptions from the throws clause. If so, why should there be a warning in the first place? We could argue that we could turn this off by default, but it defeats the whole purpose. I think the problem here is strictly with the refactoring and it better not be handled as a warning.
As I understand warnings are expensive and hence I feel that this is not the right way to solve this problem -- comments?
> I don't see any benefit or need for someone to remove one of these > exceptions from the throws clause. It's just another problem of unnecessary code that just bloats the source. Similar to e.g. redundant super interfaces or any other problem in the 'Unnecessary code' section, there's theoretically no need to clean up, but it's good practice.
(In reply to comment #9) > > I don't see any benefit or need for someone to remove one of these > > exceptions from the throws clause. > It's just another problem of unnecessary code that just bloats the source. > Similar to e.g. redundant super interfaces or any other problem in the > 'Unnecessary code' section, there's theoretically no need to clean up, but it's > good practice. (1) While mentioning only the base class would suffice, isn't there loss of non-trivial information in not choosing to list the subclass ? I don't have a concrete example, but there could arguably be some client that could arguably benefit from knowing and differently handling the different types of exception thrown. If the throws spec mentions only the base class, this information is gathered only by studying the source code in detail. Of course, a developer may choose to mention only the base class while explicitly coding: so it appears that eclipse refactoring is currently implementing a better practice and "fixing" it would dilute it. (2) There is also the performance angle, checking every exception listed to see if it is a subclass of another. In the light of this, is there consensus one way or other for going forward ?
> so it appears that eclipse refactoring is > currently implementing a better practice and "fixing" it would dilute > it. I fully agree, that's why I don't want to mix this into the refactoring. > (2) There is also the performance angle, checking every exception listed > to see if it is a subclass of another. The check only needs to be done when the option is enabled, so only those who enable it have to pay the price. But WONTFIX would also be OK with me. In that case, someone could still write a quick assist that removes redundant exceptions. But without a warning, this is hard to discover, and I'll not spend resources to implement this.
I think the detection should be off by default. Since it is not possible to refer to a thrown exception without having its hierarchy resolved, adding this detection won't trigger new errors for classes not found on the classpath. Satyam, please investigate. This is a low priority though.
Shankha, please investigate
Working on it.
Hi, I had the proposed solution: for (int i = 0; i < raisedCount; i++) { TypeBinding base = raisedExceptions[i]; //base.findSuperTypeOriginatingFrom(otherType) if (base == null) continue; for (int j = 0; j < raisedCount; j++) { TypeBinding subclass = raisedExceptions[j]; if (subclass == null) continue; if (base == subclass) continue; ReferenceBinding rbase = (ReferenceBinding)base; ReferenceBinding rsubclass = (ReferenceBinding)subclass; while (rsubclass != null) { rsubclass = rsubclass.superclass(); if (rsubclass == rbase) break; } if (rsubclass != null) { System.out.println("Redundant declaration : + " + subclass + "\n"); // report the problem. } } Is the solution on the correct path? The code belongs to checkExceptionHandlers in FlowContext.java. I still have to think if we need code to handle erasure. There are two other issues remaining: 1) I am not comfortable with downcasting (casting a TypeBinding type to ReferenceBinding type). 2) The loop I have is O(n^3). IS there a way of simplifying it. Thanks Thanks
(In reply to comment #15) > 1) I am not comfortable with downcasting (casting a TypeBinding type to > ReferenceBinding type). If you are sure that raised exceptions can only be ReferenceBinding, casting should be fine. If not, you can guard it with an 'instanceof' check. > 2) The loop I have is O(n^3). IS there a way of simplifying it. How about ReferenceBinding.isSuperClassOf(...) ?
(In reply to comment #16) > How about ReferenceBinding.isSuperClassOf(...) ? Some times method invocations can hide the computational complexity while in reality making it worse or the same. Just beware - I don't know about this case.
Created attachment 234829 [details] WIP: Patch Few missing missing: 1) Batch compiler tests. 2) A way to switch on the option. Rest of the test cases and code is in place. Thanks
