Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Contribute to JDT cleanup actions #386

Open
vogella opened this issue Jun 14, 2019 · 70 comments
Open

Contribute to JDT cleanup actions #386

vogella opened this issue Jun 14, 2019 · 70 comments
Labels
question

Comments

@vogella
Copy link
Contributor

vogella commented Jun 14, 2019

Hello, I just found your plug-in and I'm already in love with it. Thanks for this awesome work.

Would you be interested in moving parts of your plug-in to JDT cleanup actions? I think this would be highly beneficial for all Eclipse users.

P.S. I'm one of the Eclipse developers and project leads. See https://projects.eclipse.org/projects/eclipse.platform/who
@vogella
Copy link
Contributor Author

vogella commented Jun 14, 2019

Btw. I'm starting to use your tool now to improve the Eclipse code.
See for example: https://git.eclipse.org/r/c/144024/ and https://git.eclipse.org/r/c/144029/

:-)

@strkkk
Copy link
Collaborator

strkkk commented Jun 14, 2019 •
edited

@vogella sounds interesting.
What refactorings are most useful?
Is there some guide how to add new JDT cleanup actions?

Btw, thank you for your site and tutorials, they were very useful for me.

@Fabrice-TIERCELIN
Copy link
Collaborator

It's the best future we hope for this plugin! We don't know if we will be here in a decade. This sounds like immortality :)

The current development version has transformed the plugin as a CleanUp Extension. It should ease the migration. There is still one limitation: in this mode, AutoRefactor only does 1 pass. I think I know how to fix it but it's not already done.

@vogella
Copy link
Contributor Author

vogella commented Jun 14, 2019 via email

@Fabrice-TIERCELIN
Copy link
Collaborator

Fabrice-TIERCELIN commented Jun 15, 2019 •
edited

Do you mean UseMultiCatchRefactoring.java rule? Push negation down rule seems to be easier to integrate. The code is more simple, there is less dependency and it's useful.

I think we should check the policy:

  1. Some rules erase some comments (not Push negation down). Perhaps, we should had a check and abort the refactoring if a comment will be lost.
  2. Some rules need little cleaning from the other rules (Push negation down for instance) like removing useless parenthesis. The user doesn't see it as all the rules are running together but it can be a problem if we pick up one rule. Perhaps we need to rework the rules to make them self sufficient.
  3. Some rules may trigger what I call zombie code (not Push negation down). Zombie code is a code that is dead because of an error above, but it can be triggered if the bug is fixed. For example, the Equals on constant rather than on variable rule avoids some null pointers, which should be a good thing, but in some case the null pointer is caught and it disables a zombie code. I think we should process the concerned rules later.

@vogella
Copy link
Contributor Author

vogella commented Jun 17, 2019

Remove useless blocks would also be a very nice addition, opened https://bugs.eclipse.org/bugs/show_bug.cgi?id=548324 for that.

@vogella
Copy link
Contributor Author

vogella commented Jun 18, 2019

Testing now Push negation down.

@vogella
Copy link
Contributor Author

vogella commented Jun 18, 2019

By the way, should we start direct communication? If you agree, please drop me an email at Lars.Vogel@vogella.com

@vogella
Copy link
Contributor Author

vogella commented Jun 18, 2019

"Push negation down" does not have one hit in the eclipse.platform code base so I think this is not the most valuable clean for Eclipse users.

I had lots of:

1.) contains vrs. indexOf cases
2.) Remove useless block
3.) Use try-with-resource
4.) Use String.contains
5.) Use multi-catch
6.) Remove empty ifs
7.) Remove unnecessary local variables before return statement
8.) All in one method, rather than loop

Do you have a convert to method reference refactoring? That would also be beneficial.

@Fabrice-TIERCELIN
Copy link
Collaborator

LambdaRefactoring.java converts a lambda expression into a method reference if possible.

@vogella
Copy link
Contributor Author

vogella commented Jun 24, 2019

What is the next step?

Would you like start contributing cleanup actions / quickfixes to JDT? For this we use Gerrit, if you have issues with this process, I can help with that. JDT is not very active in terms of reviewing community contributions but I could ask the Redhat team to review. If you start this, I think it would be great if you would gain JDT committership at some point in time to improve these cleanups directly.

@Fabrice-TIERCELIN
Copy link
Collaborator

You said "One of our employees recently added a cleanup action to JDT". Can I see the diff?

Another question: is cleanup action multi-pass? I mean:

Given

boolean reducedValue = !!!!true;

When

Cleanup is applied

Then

  • A single pass get this:
boolean reducedValue = !!!false;
  • A multi-pass get this:
boolean reducedValue = true;

A multi-pass process can reevaluate an already changed part of code. It's coded in AutoRefactor. If it is not the case in cleanup, I think it would be a good idea to implement it.

@jjohnstn
Copy link

Hi Fabrice, I have recently been made a JDT committer. I can help if you have code to be reviewed or have questions though I am relatively new and am still learning myself.

@vogella
Copy link
Contributor Author

vogella commented Jun 25, 2019 via email

@jjohnstn
Copy link

With regards to single-pass vs multi-pass, the cleanups are not recursively checked, however, cleanups should be smart (e.g. I wrote a redundant semi-colon cleanup that will remove multiple extraneous trailing semi-colons at once and not just one at a time). Your example above could be calculated to a single change. An example where this doesn't work is removing unused private members/fields where a private field is being used by an unused private method. First cleanup removes the unused method, second cleanup will remove the unused field.

My gerrit change for redundant semi-colons can be found at:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=539437

@Fabrice-TIERCELIN
Copy link
Collaborator

@vogella, here we are, I have made a first contribution. I searched the simplest useful rule. I have complete the cleanup that removes the unnecessary modifiers. Now it will also remove the abstract modifier on interface :
Remove_abstract_modifier_on_interface.patch

Now, I don't know how I'm supposed to contribute. Can I create an entry in Eclipse bugzilla or you create it?

@Fabrice-TIERCELIN
Copy link
Collaborator

I have been able to test the feature on Eclipse but I haven't been able to run the unitary tests. Maybe you can do it.

@vogella
Copy link
Contributor Author

vogella commented Jul 8, 2019

@Fabrice-TIERCELIN awesome. I'm already in summer vacation, so it would be great if you can open a bug directly. Please use "UI" and the following link: https://bugs.eclipse.org/bugs/enter_bug.cgi?product=JDT

This requires a bugzilla user.

We use Gerrit for code contributions, as for the setup, please see https://www.vogella.com/tutorials/EclipsePlatformDevelopment/article.html#exericse-eclipse-user-creation-and-gerrit-server-configuration

The setup usually only takes 10 minutes in our hackerthons, we do from time to time in Hamburg.

For the Gerrit, please add Jeff (Jeff Johnston from Redhat) as reviewer. Review times in JDT can be very long, adding Jeff (or Roland Grunberg if Jeff is not available) as reviewer should result in faster review.

I'm happy to help with any Gerrit questions, important rule is to use the same Change-Id if you want to update the review. Eclipse Staging View has nice Gerrit support and the ammend option can be used to update Gerrits.

@vogella
Copy link
Contributor Author

vogella commented Jul 8, 2019 via email

@vogella
Copy link
Contributor Author

vogella commented Aug 7, 2019

Awesome @Fabrice-TIERCELIN, thanks for https://bugs.eclipse.org/bugs/show_bug.cgi?id=549242. Any plans to contribute more cleanups?

@Fabrice-TIERCELIN
Copy link
Collaborator

I'm preparing the migration of Autoboxing and Unboxing.

@akurtakov
Copy link

@Fabrice-TIERCELIN thanks for doing it - would you please watch https://twitter.com/ZhekaKozlov/status/1135506701438857217 . I'm interested whether there are other of your cleanup actions that can help us achieve parity (boxing/unboxing should be one of them).

@Fabrice-TIERCELIN
Copy link
Collaborator

@akurtakov What is this tool? What is the list of the features?

@PyvesB
Copy link

PyvesB commented Aug 14, 2019

Awesome work, looking forward to seeing more features getting migrated! 👍

@Fabrice-TIERCELIN
Copy link
Collaborator

Hi @vogella, do you know how your Jenkins is configured to feature the build SUCCESS as green? On my Jenkins, it appears in blue...

Or do you know who knows?

@PyvesB
Copy link

PyvesB commented Sep 1, 2019

Hi @vogella, do you know how your Jenkins is configured to feature the build SUCCESS as green? On my Jenkins, it appears in blue...

Or do you know who knows?

You'll have to install the Green Balls plugin. 😉

@Fabrice-TIERCELIN
Copy link
Collaborator

OK, thanks @PyvesB.

@Fabrice-TIERCELIN
Copy link
Collaborator

Awesome work, looking forward to seeing more features getting migrated! 👍

@PyvesB, @akurtakov and @vogella, don't support the coder, support the reviewer. The migration time is estimated to 6 years and it's mostly review time.

Or be a merger 😺

@vogella
Copy link
Contributor Author

vogella commented Sep 8, 2019 via email

@Fabrice-TIERCELIN
Copy link
Collaborator

Do you want to remove also the final modifier on method parameters?

@vogella
Copy link
Contributor Author

vogella commented Oct 23, 2019 via email

@Fabrice-TIERCELIN
Copy link
Collaborator

How can I ask commit rights?

@jjohnstn
Copy link

jjohnstn commented Nov 7, 2019

Hi Fabrice, it is not just a simple case of asking for commit rights. What is required is a nomination to the JDT group. Before that, a body of work consisting of bug fixes / features needs to be established that prove you require minimal oversight. At this point, you are definitely on the right track to becoming nominated but there aren't enough merged bug fix changes yet (excludes code cleanups) and you also need to get a chance to demonstrate administrative aspects for bugs (milestones, status). I am confident you will get there in a shorter time than it took me.

@Fabrice-TIERCELIN
Copy link
Collaborator

I discovered a bug yesterday and I saw that my notifications on bugzilla were disabled in my preferences. Is there any other tickets I'm in CC?

@Fabrice-TIERCELIN Fabrice-TIERCELIN mentioned this issue Nov 11, 2019
Open
@vogella
Copy link
Contributor Author

vogella commented Nov 12, 2019 •
edited

Is there any other tickets I'm in CC?

I asked you a while ago if you could try your cleanups on Platform code and provide patches for them. But I also asked you already here and you also did not react so I assume this is your way of saying no.

@Fabrice-TIERCELIN
Copy link
Collaborator

I admit I'm not very OK to refactor a huge code I don't know... You also asked me to create a rule to remove the final keyword. As there are lots of activities around AutoRefactor, I prefer making rules for anyone than this rule...

I think you're right and that's probably the reason I haven't answered :) Anyway, which is the ticket for cleaning the Platform code?

@vogella
Copy link
Contributor Author

vogella commented Nov 13, 2019

I admit I'm not very OK to refactor a huge code I don't know.
Understandable and thanks again for converting our cleanups to platform. Looking forward to see more of them, we starting to run the cleanups on the platform code.

@Fabrice-TIERCELIN
Copy link
Collaborator

Yet another cleanup: Use var keyword

It's the first cleanup I have coded that does not come from AutoRefactor. Meanwhile, here is my cleanups that can already been reviewed:

And also:

@PyvesB
Copy link

PyvesB commented Nov 21, 2019

I've picked up the number suffix patch, hopefully that will help a bit. Though as I'm not a committer either, there's only so far we'll be able to go.

Reviewing and merging patches does seems to be a big problem with the JDT project at the moment. I also submitted one back in July, it's looking increasingly likely that it won't get shipped until at least March next year. 😕

@akurtakov
Copy link

We are doing our best to improve the situation with reviews. I have to admit it's still not in acceptable state but at least compared to a year ago the improvements in review time are huge. I know these are empty words if your patch hasn't been looked at. Please add me on CC to the gerrit so I can push JDT committers to do a revew (I'm not JDT committer myself so can't do it myself).

@Fabrice-TIERCELIN
Copy link
Collaborator

I'm handling some bugs so that the mergers can even more focus their work on merge. How to self assigning a bug?

And when will be the frozen zone? Where to know it?

@PyvesB
Copy link

PyvesB commented Nov 21, 2019

How to self assigning a bug?

I don't think you can if you're not the one who created the bug/not part of the project. I generally simply put a comment saying I'm working on something.

And when will be the frozen zone? Where to know it?

Next Wednesday apparently. See here.

@vogella
Copy link
Contributor Author

vogella commented Nov 21, 2019 via email

@PyvesB
Copy link

PyvesB commented Nov 22, 2019

Hopefully bug management will become much smoother when Buzilla is replaced. There are discussions about this on Bug 537913.

Gerrit is somewhat a pain-point as well IMO, the old 2017 version the Foundation is running doesn't compete with what GitHub or Gitlab provide nowadays (upgrading is tracked by Bug 541221).

But we're getting slightly off-topic here!

@vogella
Copy link
Contributor Author

vogella commented Nov 22, 2019 via email

@PyvesB
Copy link

PyvesB commented Nov 22, 2019

Please comment in the Bug 541221 to remind the foundation that users care.

Done.

Random thought: wouldn't it be great to have the Source -> Clean Up... command directly accessible through EGit in the Git Staging view? Like that you could only cleanup the files you have modified since since last commit and also have a reminder to do so.

@vogella
Copy link
Contributor Author

vogella commented Nov 23, 2019 via email

@Fabrice-TIERCELIN
Copy link
Collaborator

Random thought: wouldn't it be great to have the Source -> Clean Up... command directly accessible through EGit in the Git Staging view? Like that you could only cleanup the files you have modified since last commit and also have a reminder to do so.

To do this:

  1. Select all in Git Staging view
  2. Right-click on the files
  3. Click on Show in -> Package explorer
  4. Right-click on the selected files
  5. Click on Source -> Cleanup...

It's not obvious so I don't know if it is worth to add a shortcut. It is asked but not highly voted. I think that this way lot's of shortcuts should be added...

@Fabrice-TIERCELIN
Copy link
Collaborator

@vogella, if you want to cleanup a huge part of the Eclipse code base, I advise you the following cleanups. They all improve performance and I filtered the ones I consider as dangerous. Improving the performance would be a benefit for the contributors and the users:

  • Int primitive rather than wrapper
  • Short primitive rather than wrapper
  • Long primitive rather than wrapper
  • Double primitive rather than wrapper
  • Float primitive rather than wrapper
  • Char primitive rather than wrapper
  • Byte primitive rather than wrapper
  • Primitive wrapper creation
  • Boolean primitive rather than wrapper
  • Lazy logical rather than eager
  • Boolean constant rather than value of
  • String rather than new string
  • String
  • String value of rather than concat
  • Opposite comparison rather than negative expression
  • Remove empty if
  • No loop iteration rather than empty check
  • Do while rather than while
  • Break rather than passive iterations
  • String builder
  • String builder rather than string buffer
  • Hash map rather than hashtable
  • Array list rather than vector
  • Array deque rather than stack
  • Array list rather than linked list
  • Set rather than list
  • Hash map rather than tree map
  • Hash set rather than tree set
  • Static constant rather than instance constant
  • Remove overridden assignment
  • Log parameters rather than log message
  • Remove unnecessary local before return
  • Remove unnecessary cast
  • Local variable rather than field
  • Simplify expression

@vogella
Copy link
Contributor Author

vogella commented Nov 24, 2019 via email

@vogella
Copy link
Contributor Author

vogella commented Jan 7, 2020

I tried staring running the suggested improved refactoring but face the issues described in #427

  • Int primitive rather than wrapper
  • Short primitive rather than wrapper
  • Long primitive rather than wrapper
  • Double primitive rather than wrapper
  • Float primitive rather than wrapper
  • Char primitive rather than wrapper
  • Byte primitive rather than wrapper

@vogella
Copy link
Contributor Author

vogella commented Jan 8, 2020

cc @ruspl-afed

@ruspl-afed
Copy link

WOW, looks like a secret treasury!
I love open source :)

@Fabrice-TIERCELIN
Copy link
Collaborator

Fabrice-TIERCELIN commented Jan 13, 2020 •
edited

If you're interested, here is an AutoRefactor mod that lombokifies a Java project:

If I remember well, it lacks a feature (static field handling or boolean getter, I don't remember). Give me feedback and I can improve it.

Beware! Don't execute other rules on lombok code! It would destroy it.

@Fabrice-TIERCELIN
Copy link
Collaborator

Hi @vogella,

Here is your feature that removes all the optional final modifiers

To use it:

  1. Remove the .txt extension
  2. Put the files in your eclipse/dropins folder
  3. Launch Eclipse
  4. Click on project -> Source (AutoRefactor) -> Choose... -> Remove optional final

I still think it's foolish to do so... It even changes constants into variables. Do as you want.

@vogella
Copy link
Contributor Author

vogella commented Feb 26, 2020 via email

@Fabrice-TIERCELIN
Copy link
Collaborator

I have also used it to clean up AutoRefactor, modifying the rules.

@vogella
Copy link
Contributor Author

vogella commented Feb 27, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@vogella @akurtakov @jjohnstn @strkkk @PyvesB @ruspl-afed @Fabrice-TIERCELIN