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

Add use declaration re-ordering #1104

Merged
merged 7 commits into from
Jul 26, 2016
Merged

Add use declaration re-ordering #1104

merged 7 commits into from
Jul 26, 2016

Conversation

studoot
Copy link
Contributor

@studoot studoot commented Jul 19, 2016

This partially addresses #298, adding sorting of contiguous runs of use declarations depending on the value and type of path referenced. As an example (taken from the added integration test files), the following code

use std::str;
use std::cmp::{d, c, b, a};
use std::ddd::aaa;
use std::ddd::{d as p, c as g, b, a};
// This comment should stay with `use std::ddd:bbb;`
use std::ddd::bbb;

gets transformed to

use std::cmp::{a, b, c, d};
use std::ddd::{a, b, c as g, d as p};
use std::ddd::aaa;
// This comment should stay with `use std::ddd:bbb;`
use std::ddd::bbb;
use std::str;

The reorder_imports config item has changed from a boolean to {None, Lines, Items, LinesAndItems}, to allow combinations of use declaration lines and use declaration items to be selected.

I would have to say that this isn't a finished item - there are several areas that I'm not 100% happy with:

  • Any 'missing spans' before the run of use declarations remain at the start of the run, even after the declarations are reordered. I can see situations where a comment is expected to stay attached to the first use in a run. I'm thinking situations like:

    // This is a file header comment
    
    // We need to use the filesystem
    use std::fs;
    use std::cmp;

    which will currently become

    // This is a file header comment
    
    // We need to use the filesystem
    use std::cmp;
    use std::fs;
  • Similarly, any end of line comment after a use will stay with the code after that use:

    use std::fs; // We need to use the filesystem
    use std::cmp;

    which will currently become

    // We need to use the filesystem
    use std::cmp;
    use std::fs;
  • Contents of import lists are currently ignored when comparing imports, meaning that use a::b::{c, d}; is considered equal to use a::b::{e, f}; (but that is relatively easy to implement).

  • Comparison functions for AST items such as paths etc are clustered at the top of visitor.rs. Is there either a) a preferable place to put them, or b) a preferable way to implement them?!

  • Looking at the code again, some refactoring would be beneficial so that the use declarations in functions were processed the same as those in modules (that would mean changing the code around the call to visit_item in visit_stmt)

Anyway - it's a start...

@nrc
Copy link
Member

nrc commented Jul 22, 2016

Also todo for later is that I would expect imports to be squished together, e.g.,

use std::ddd::aaa;
use std::ddd::{d as p, c as g, b, a};
// This comment should stay with `use std::ddd:bbb;`
use std::ddd::bbb;

becomes

use std::ddd::{aaa, a, b, c as g, d as p};
// This comment should stay with `use std::ddd:bbb;`
use std::ddd::bbb;

Note the second import stays separate due to the comment.

@nrc
Copy link
Member

nrc commented Jul 22, 2016

Contents of import lists are currently ignored when comparing imports, meaning that use a::b::{c, d}; is considered equal to use a::b::{e, f}; (but that is relatively easy to implement).

Could you file an issue for this and add a FIXME(#nn) to the code please?

@nrc
Copy link
Member

nrc commented Jul 22, 2016

Comparison functions for AST items such as paths etc are clustered at the top of visitor.rs. Is there either a) a preferable place to put them, or b) a preferable way to implement them?!

I would put them in imports.rs, I guess the more generic stuff (e.g., Paths) should be somewhere else, but we can deal with that if they become more widely used.

Items,
// Order `use` statements by their path and items within `use` statements by their name.
LinesAndItems
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to add another option, rather than have a single enum like this - this way lies combinatorial explosion!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely - I had a look to see if there was a way that a list of enumerations could be supplied rather than just one (something like reorder_imports = [ "Lines", "Imports" ] or reorder_imports = "Lines,Imports"), but that was looking a bit involved, so I'll change to separate options.

@nrc
Copy link
Member

nrc commented Jul 22, 2016

This looks great, thanks for the PR! A few very minor comments to address.

@studoot
Copy link
Contributor Author

studoot commented Jul 22, 2016

  • Sorting on import item lists was simple enough, so I've added that to this PR...
  • Moved format_imports and format_import to imports.rs
  • Added comment to walk_mod_items
  • Changed reorder_imports options to two separate boolean options

@nrc nrc merged commit 78b52ec into rust-lang:master Jul 26, 2016
@nrc
Copy link
Member

nrc commented Jul 26, 2016

Thank you!

jefftt pushed a commit to jefftt/rustfmt that referenced this pull request Jul 26, 2016
* Add config options for combinations of lines and items

* Reordering of import lines implemented.

* Changed nested matches to tuple pattern matching

* Added ordering of path list items to the ordering of use declarations

* Move `format_imports` and `format_import` methods to `imports.rs`

* Add comment to explain how `use` declarations are split off while walking through a module

* Change `ImportReordering` config option to separate boolean options
@studoot studoot deleted the order-lines-and-items branch July 26, 2016 15:50
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

Successfully merging this pull request may close these issues.

2 participants