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.

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

Share use normalisation code between ordering and formatting #2531

Closed
nrc opened this issue Mar 15, 2018 · 5 comments
Closed

Share use normalisation code between ordering and formatting #2531

nrc opened this issue Mar 15, 2018 · 5 comments

Comments

@nrc
Copy link
Member

nrc commented Mar 15, 2018

In order to accurately re-order, we need to apply a lot of the normalisation transfomations. This is a waste since we then throw them all away (worse we do it on every compare, not just once for the whole search). We should instead do the conversion to our own data types earlier, sort then output the re-ordered (and normalised) imports. I think that means we need to include either span info or comments and newlines and visibility with our representation of use paths.

@nrc
Copy link
Member Author

nrc commented Mar 15, 2018

There actually looks to be a whole lot of duplicate code between imports.rs and reorder.rs and its not clear to me how to resolve it.

@toolness
Copy link

Hi @nrc, you mentioned in #2536 that I might be interested in pursuing this, and it looks interesting! Do you think this issue could be resolved in an incremental way, e.g. via multiple PRs that each refactor one small thing?

My only caveat right now is that I don't have a ton of time over the next 3-4 days, so if you need this issue resolved ASAP, I'm probably not the person to do it. But I'll have more free time near the end of the week, and over the weekend, so if this issue isn't super urgent then I'm happy to help.

@nrc
Copy link
Member Author

nrc commented Mar 18, 2018

Do you think this issue could be resolved in an incremental way, e.g. via multiple PRs that each refactor one small thing?

Yes, but it is probably a few small PRs, a large PR, and maybe a few more small ones. I'm not entirely sure.

My only caveat right now is that I don't have a ton of time over the next 3-4 days, so if you need this issue resolved ASAP, I'm probably not the person to do it. But I'll have more free time near the end of the week, and over the weekend, so if this issue isn't super urgent then I'm happy to help.

That's fine, this is not super-urgent since afaik nothing is actually broken, it's just sub-optimal code. I'll try and add a few pointers here some time in the next few days.

A good place to start would be to update rustc-ap-syntax dependency. That should bring in this PR which I think will break the code in reorder.rs (hopefully that will show up in tests, I think there will be compilation breakage too). Fixing reorder.rs should be fairly easy - if you look at the changes to ast.rs in the rustc PR, you should be able to change the from_ast function in reorder.rs to match. That should get you familiar with the reorder code.

@rleungx
Copy link
Contributor

rleungx commented Mar 19, 2018

Hi @nrc. I am tring to update the rustc-ap-syntax dependency to 71.0.0, but there is a test failed. Would you mind taking a look at this and give me any suggestions?

@nrc nrc mentioned this issue Mar 22, 2018
@topecongiro
Copy link
Contributor

I have implemented this in https://github.com/topecongiro/rustfmt/tree/merge-imports. All tests are passing, although the code needs more refactorings and the commit history is not well organized. I will create a PR once I make it tidier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants