Community
Participate
Working Groups
We got Rename and move refactoring for classes - but there does not seem to be a "Delete" refactoring! Usecase: I have an old exception class that is not used anymore but is declared to be thrown multiple places - I then remove the class and now I have to manually wander around in the source code and remove the code that references it.
+ 1 from me. A delete refactoring would be useful. The exception case is a good example.
This would be a Safe Delete refactoring with the semantics to also delete references to the class (when it is safe). The UI should present the references to the user allow to selectively decide which ones should be deleted.
to reconsider after 2.1
reop
external contribution opportunity
I haven't seen (actually searched for) this bugreport before. I came up with the same idea and here are the three posts I wrote about it. Feedback and answer to the questions on searching are most welcome. 1) The search for candidates posting the idea is to remove members (classes, methods and class/instance variables) which are not referenced (anymore). The collected candidates for removal are to be displayed to let the user pick the methods s/he wants to remove. Hopefully I was able to express myself clearly. If not, please ask. I am more than happy to try a different approach of explaining or thinking about it. I believe at least the following Issues have to be taken care of... [ Simple Example ] Let's start with one of the simplest cases. public class A { private void a() {} // referenced from inside this class public void b() { a();} // referenced from the outside? private void c() {}; // never referenced } This would result in the following collection of items to analyze: Name Containing Type Access of Item and Supertypes Modifier declaring the item ------------------------------------------------------------ a() A private b() A public c() A private The results of this case are pretty obvious: A.b() is not referenced from inside A. According to the public access modifier of A.b() the whole workspace has to be searched for items referring to this method. If none is found it is a candidate for removal. A.a() is referenced from A.b(). Assuming A.b() is referenced from the outside of this class, it will _not_ be a removed. Hence A.a() will _not_ be a candidate for removal. A.c() is not referenced from inside A, there is no named inner class and as A.c() is declared private the search ends here. Hence it _is_ a candidate for removal. [ Taking Supertypes into Account ] An example with three super types: interface I { void i(); } public class SuperSuperA { public void ssaa() {} public void ssab() {} } abstract class SuperA extends SuperSuperA implements I { void i() {} } public class A extends SuperA { public void i() {} public void ssab() {} } Name Containing Type Access Remark of Item and Supertypes Modifier declaring the item ------------------------------------------------------------------------------ i() A, SuperA, I public The implementation of A.i() could be used when referring to SuperA or I. ssab() A, SuperA, public The implementation of A.ssab() could be SuperSuperA used when referring to SuperA or SuperSuperA. Note that SuperSuper.ssaa() didn't make it into the collection of items as it is not overridden in A. Resulting in: As A.i() is public the whole workspace has to be searched for references to A.i(), SuperA.i() and I.i(). This is also true for A.ssab(). A.ssab(), SuperA.ssab() and SuperSuperA.ssab() have to be searched in whole workspace. [ Recursive Dependencies ] An example with recursive dependencies. public class A { private void a() { b(); } private void b() {} } A.b() can not be a candidate for removal as it is referenced by A.a(). A.a() can be removed though, but after doing so, A.a() is also a candidate for removal. As the user may not select to delete A.a(), A.b() cannot be removed in every case. (any case? my English su... I mean sometimes it can, sometimes it can't). Therefore it is necessary to record the candidates differently, including the dependencies. Name Containing Type Access Depends on of Ite m and Supertypes Modifier the removal of declaring the item -------------------------------------------------------------------------------------------- a() A private - b() A private A.a() The collection of removal candidates is to be presented to the user in a way reflecting the dependencies. One possible implementation would be to show the candidates in a hierarchical view. The independent candidates are shown on the first level and the respective depending items are below the candidate they depend on. If a containing candidate is deselected all depending candidates and candidates depending on the latter are also deselected and cannot be selected without selecting the candidate they depend on. A typical example for recursive dependencies might be accessors and the respective instance variable or (inner) classes which are only referenced by a Widget using them as XXListeners and can be removed when the button reaches its end of life. [ When to stop searching for References ] Please consider the following code. public class A { public static void aa() { } public static void ab() { A.aa(); } // main() and all methods // of Object are blacklisted // and will not be used as // candidates. public static void main(String[] args) {A.ab(); } } public class B { public static void ba() { A.aa();} } public class C { public static void ca() { A.aa();} } Assumption: As A.aa() is referenced by A.ab(), B.ba() and C.ca() it is not a candidate for removal. To get this result the whole workspace must be searched as A.aa() is declared public, but this would mean a serious performance penalty. The first possible optimization would be to stop looking for more references after the first is found. In the am scenario that would mean to stop to after A.ab() is found. As a consequence A.aa() would be in a unknown state when A.ab() (another candidate) will not be confirmed by the user, because when A.ab() is not removed it is unknown if A.aa() is referenced by more callers than A.ab(), as the search has been stopped prematurely. Therefore the search can only be stopped, after the first caller is found which is not also a candidate. Hence the callers, which are also candidates, have not to be recorded in case a caller outside the candidate list is found. [ Performance ] I believe that displaying the search results and executing the removal are relatively cheap compared to the search, so I will just focus on the search for now. - general search scope - For non-static private members only the class itself and the named, non-static, inner classes have to be searched. The search for members declared with the package modifier will start like a search for private members, but will also the search the declaring package. Regarding protected members the private and package search has to be performed and furthermore all subclasses, which are not in the same package as the member itself. Public members are the worst. They can be referenced by everything. So as worst case the whole workspace has to be searched. This is just a rough outline to build on for the below mentioned issues. It would be important to show the user a tip helping him to understand that it is important to define an appropriate workingset, which includes only his/her own code, not all the libraries etc. Also the default settings should already narrow down the search, e.g. "public" as a search criteria should be deselected. - batching the search - The two factors having the most impact on the performance will likely be disk io and parsing, therefore an ideal search would touch each ressource only once and also parse it only once. - stopping early / redefining search scope - That would mean that search scope needs to reflect the maximum scope universe of all members to search for. If there are 10 members to search for declared private and one public this could mean to search the whole workspace. Still for this particular public member it is the only choice there is. Therefore it is necessary to suspend the search after the first reference to the public member is found (where the calling member is not a candidate itself) _and_ recalculate the maximum search scope for the remaining candidates (in the a.m. case it should be none, as private members should be searched first, but there are other cases, like having more than one public member to search for). To make a long story short, the search scope needs to be redefined during the search and it also must be possible for the caller to stop the search for the case that relevant references are found for all candidates before having searched the remaining scope completely. [ Performance :: Implementing the search using the Java Search Engine ] I had a quick peek at the JavaSearchEngine and although I am not through yet, I have found the OrPattern to build the maximum search scope, but haven't found anything yet to suspend/stop a running search and to reduce the scope of it. 2) How the user experience should look like and the answers to some questions of Adam The user should be able to select an arbitrary JavaElement and invoke the refactoring on it. I would start with methods only, but increase the scope later on to types, packages etc. I believe the only limitation is performance. [ Searching the candidates ] After selecting a JavaElement a Wizard is fired up where the scope of searching for _candidates_ is to be defined. Here comes an outline of some of the _visible_ filter criterias to select: Selection of Candidate: [ includes ] access modifiers: - public (off) - protected (on) - package (on) - private (on) other contained elements: - include members of subclasses (on) - etc. by type: - methods (on) - anonymous and names inner classes (on) - fields (on) - etc. (on) [ excludes ] by member names: - getters (off) - setters (off) - regexp method names (off) Search for references to candidates: - working set etc. (will be inspired by the java search) Furthermore the user should be presented with a tip explaining the importance to narrow down the search and possibly define a working set, which just contains code with the potential to reference the candidates, not also third-party-libraries etc. It should also be mentioned what the consequences of a to narrow search would be. It should also be possible to set _invisible_ excludes on a preference page. The defaults will include all methods from Object and the main() method. Note: Please bear in mind, that this is only the search for candidates. During the search for references to the candidates the excludes and deselected includes will obviously not being honored. [ Searching the candidates ] Here is the beef. See my previous posting (not the accidently sent one though ;-). When a resource cannot be read (e.g. out of sync) then the search will be cancelled and the user will be presented with an error message saying what the problem is. The same holds true, if a candidate is read-only. The only unanswered question crossing my mind is how to deal with multiple errors? Will it make more sense to fail early or to collect all errors (might be very slow). I guess it makes sense to make this a preference. I believe the multiple-error approach would pay off in the real world. E.g. in my current day job project we are forced to use a scm tool utilizing the pessimistic approach and so this information is needed for checking out the affected ressources. [ Displaying the candidates ] The candidates, i.e. java elements only referenced by other candidates or not referenced at all, should be displayed in a tree view. Unreferenced java elements not referenced by anything in the whole workspace are to be shown on the first level. Candidates that will only be unreferenced when another candidate will be removed are to be shown below the candidate they depend on. The next level will show the candidates depending on candidate depending on a top-level candidate etc. etc. A depending candidate can be deselected independently of its parent, but can only be selected if the parent is also selected. If a parent is deselected all children will be deselected too. Beneath the candidate will be a check box, triggering the removal. extension (from Olivier): The checkbox for removal will be replaced by a radio button or list with two options: "Remove Element" and "Deprecate Element". Remove Element says it all, deprecate element means to change the Java Doc of the element. A @deprecated keyword will be added an a TODO marker to be replaced by a good explanation why this method has been deprecated and probably what is to be used instead. For this extension it would also be necessary to rethink the tree view. The rule would the read as follows: A child is/can only be selected when the parent is marked for removal, i.e. a parent marked as deprecated will still reference the child. [ Executing the removal / deprecation ] That looks pretty straight-forward to me. Of course it has to be checked for read-only files (see above), but nothing special I can think of. OK, after the full picture back to the check list: - how do people invoke the refactoring? They select a JavaElement and invoke the first wizard page from the context menu. The refactoring will be available for all Java Elements. Well, in the final implementation. It would be already a worthy refactoring if would just work on individual methods. > - what does the ui look like? It's golden with red stripes from the upper left to the lower right. There will be nice mp3 song playing, downloaded from an illegal server, selected by consulting a chart list website and accounting for the current time of the day and if is a weekend or not ;-) There will also be a mpeg playing. The selection of content will be based on the users age. Apart from that the first page will ask the user to define the scope of candidate selection. > -- what is the input that the refactoring needs? a) The raw candidates from the selection and its children and the filter. b) the Java Elements in the search scope. > -- is all the input available at once, or do we need to ask additional > questions after some part of the analysis has been performed? Well, the filter wizard page does have all input. The display of the removal candidates will need the selection of the user how to proceed. > - what are the possible errors and how to present them to the user? Unreadable resources. They should be displayed altogether after the search is finished. Probably a preference to fail early makes sense here. > - an application has many 'root' entry points normally (elements that are > always 'reachable') e.g. the main method is the most obvious one, but every > test... method is an entry point in a class that subclasses You should be able to exclude some elements defined by name (regexp) on a preference page. The default settings might also include suite() etc. > everything in not-internal packages in eclipse's > source code is an entry point, etc. We need a way to collect that > information from the user in an intuitive way. I am not sure if I got this right ... Is it? We need to care for references outside of the workspace, like framework methods not used inside the framework or the user code in the workspace? I don't see a way to compute that. Or do you mean to select for example setUp() on some testcase and fear that it will be a candidate for removal as it is not referenced in the user code? That is something the user needs to know about. See the "tip" I want to display when defining the search scope. The user needs to know that s/he needs to select the junit.jar in the working set. It will be found then in the collection process and not removed (see other posting). On the sunny side the removal is not done automatically. The user needs to opt out what elements s/he doesn't want to be removed. 3) And another thing for the "tip" explaining to the user the consequences of narrowing down the scope of the search for candidates I just thought about something else to tell in the "tip". It probably makes sense to exclude JUnit-Tests from the search, as if they would be the only ones referencing the candidate it's propably still worth to remove the method and the tests. I don't see that automatic support for the removal would be a) much needed and b) hard to implement as the tests likely uses the candidate to build something better. Hence when removing the candidates the test code would break (compile error). Which can be fine or not. The refactoring is still preserving the behaviour of the "productive" code. Another approach would be to change the second wizard page, presenting the user with the candidates for removal. The extension would be to also show elements which are referenced. When selecting a referenced member then the references will be shown in the lower part of the screen and the user can make an informed to decision to deprecate (even remove?) the element anyway. But I believe that would be to complicate to understand for some users. Maybe a setting on the search page may help. From my point of view this is a bit ugly and eventually something to do after the first release in any way.
>We need to care for references outside of the workspace, >like framework methods not used inside the framework >or the user code in the workspace? >I don't see a way to compute that. in the long run we need to care about these. one test case that should work is: run the refactoring on jdt.ui code and find all unused classes. Things contributed by plugin.xml are not referenced from java code but still cannot be deleted.
I am not sure if I understand the topic properly. I'd like to rephrase what I got: class SomeException extends Exception {} class A { public static void doesNotRaiseSomeExceptionDirectly() throws SomeException { B.doesNotRaiseSomeExceptionToo(); } } class B { public static void doesNotRaiseSomeExceptionToo() throws SomeException { // no reference to SomeException in here } } class C { public void doesRaiseSomeException() throws SomeException { throw new SomeException(); } } 1) So when selecting SomeException the refactoring should check doesNotRaiseSomeExceptionDirectly(), descend to doesNotRaiseSomeExceptionToo(), learn that both aren't using SomeException (anymore) and mark both as removal candidates to let the user decide later on if s/he wants to remove them. Then the refactoring goes into C.doesRaiseSomeException() and will show an error message (or a summary of the references) that SomeException is still used and that there is no way of deleting SomeException safely without the user manually removing the offending code in C.doesRaiseSomeException? 2) What if C wouldn't exist? The user is presented with the list of changes to do and can deselect each proposed change individually? That would yield in the removal of SomeException being unsafe again. So only the preliminary steps are to be performed, but not the removal of the initially selected exception?! That would also mean that in case 1) the prelimary steps can be executed also. Is that the story? 3) What should happen with comments, javadoc etc.? Sure, the javadoc around the declaration of the exception should be eliminated, but what about references in javadoc of a calling method, e.g. @throws? It has to go too, right? What about just the word SomeException in a comment of a method where SomeException will be removed from the throws clause or just about any comment refering to it by either qualified or simple name?
i will respond more fully after i get back from vaction but i think it's best if you first implement the first, early version and we will be able to discuss and improve things more concretely, on the working tests and code. If you attach the code here i'll be able to look at it and comment on it.
I think the unused exception case is not so interesting. unused type/method/field/package, however, is. I'd start by implementing the first cut of this functionality, along with test cases. My candidate for the first cut would be finiding unused classes (with classes with 'main' methods used as the only entry points). We could then improve incrementally (add support for real entry point selection, unused method detection etc. ). I'm afraid this item will only be included if we receive it as a contribution from the community - otherwise it is unlikely that it will make it into the 3.0 plan. We can provide guidance, however.
Hi Adam, >>I think the unused exception case is not so interesting. ? It is an exceptional case and more complicated than the others in terms of reference usage analysis and cleaning (remove unused Exceptions from throws clause etc.). Shortly after I tried to rephrase what I believe was the scope from this report (16th of July), I implemented a first cut of the non-ui part. It is handling every cases I could think of. I was cutting a lot of corners though and the code is not cleaned up at all. There are lots of TODO markers, which are actually questions. Maybe you can look at the code and answer one or two of the marked questions. As the the scope just changed and the implementation of the new scope would be very much different (no usage analyzer, no exception usage cleaner etc.), I am not gonna clean up the old code and start over again. The current code demonstrates its cases by using half a dozen tests. I'll upload it tonight. But again ... it's not cleaned up ;-) Last time you said to base it on SelfEncapsulateField. Is this still valid, or would it make more sense to go for the processor/participants approach? >I'm afraid this item will only be included if we receive it as a contribution >from the community - otherwise it is unlikely that it will make it into the 3.0 >plan. Due to constraints from my day job I am not able to commit myself to it. >We can provide guidance, however. Appreciated a lot.
Created attachment 5770 [details] see filename
Created attachment 5771 [details] see filename
Adam, sorry, but as mentioned in my last email this communication thing doesn't seem to work out for me. I am out of this. Cheers.
*** Bug 45205 has been marked as a duplicate of this bug. ***
*** Bug 72924 has been marked as a duplicate of this bug. ***
Bug 72924 asked for save delete/remove of method references if a method gets deleted.
*** Bug 134881 has been marked as a duplicate of this bug. ***
As of now 'LATER' and 'REMIND' resolutions are no longer supported. Please reopen this bug if it is still valid for you.
.
*** Bug 368917 has been marked as a duplicate of this bug. ***
Ping. How about starting small? The simplest use case would be deleting a field along with its getter/setter methods. The user should be warned if: (1) the field is used outside the getter/setters or (2) the getter/setters are used outside the class.
(In reply to Dirk Baeumer from comment #17) > Bug 72924 asked for save delete/remove of method references if a method gets > deleted. It would be very useful if one wants to remove a no longer used method from an interface. Currently one has to delete the method and then remove it everywhere in implementations. If there are alot this can become a nightmare...
