Skip to content

Dedup imports_granularity = "Item" #4737

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

Merged
merged 2 commits into from
Mar 16, 2021
Merged

Dedup imports_granularity = "Item" #4737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57c2dd8
Fix for issue 4725 - dedup Item imports_granularity (2nd version)
davidBar-On Mar 10, 2021
bd045d8
Use unique() instead of unique_by()
davidBar-On Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/formatting/imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ use std::borrow::Cow;
use std::cmp::Ordering;
use std::fmt;

use core::hash::{Hash, Hasher};

use itertools::Itertools;

use rustc_ast::ast::{self, UseTreeKind};
use rustc_span::{
symbol::{self, sym},
Expand Down Expand Up @@ -87,7 +91,7 @@ impl<'a> FmtVisitor<'a> {
// sorting.

// FIXME we do a lot of allocation to make our own representation.
#[derive(Clone, Eq, PartialEq)]
#[derive(Clone, Eq, Hash, PartialEq)]
pub(crate) enum UseSegment {
Ident(String, Option<String>),
Slf(Option<String>),
Expand Down Expand Up @@ -183,6 +187,8 @@ pub(crate) fn merge_use_trees(use_trees: Vec<UseTree>, merge_by: SharedPrefix) -
}

pub(crate) fn flatten_use_trees(use_trees: Vec<UseTree>) -> Vec<UseTree> {
// Return non-sorted single occurance of the use-trees text string;
// order is by first occurance of the use-tree.
use_trees
.into_iter()
.flat_map(UseTree::flatten)
Expand All @@ -197,6 +203,7 @@ pub(crate) fn flatten_use_trees(use_trees: Vec<UseTree>) -> Vec<UseTree> {
}
tree
})
.unique()
.collect()
}

Expand Down Expand Up @@ -673,6 +680,12 @@ fn merge_use_trees_inner(trees: &mut Vec<UseTree>, use_tree: UseTree, merge_by:
trees.sort();
}

impl Hash for UseTree {
fn hash<H: Hasher>(&self, state: &mut H) {
self.path.hash(state);
}
}

impl PartialOrd for UseSegment {
fn partial_cmp(&self, other: &UseSegment) -> Option<Ordering> {
Some(self.cmp(other))
Expand Down
0 tests/source/imports-impl-only-use.rs → ...s/source/imports/imports-impl-only-use.rs
View file
Open in desktop
File renamed without changes.
0 ...source/imports-reorder-lines-and-items.rs → ...mports/imports-reorder-lines-and-items.rs
View file
Open in desktop
File renamed without changes.
0 tests/source/imports-reorder-lines.rs → ...s/source/imports/imports-reorder-lines.rs
View file
Open in desktop
File renamed without changes.
0 tests/source/imports-reorder.rs → tests/source/imports/imports-reorder.rs
View file
Open in desktop
File renamed without changes.
0 tests/source/imports.rs → tests/source/imports/imports.rs
View file
Open in desktop
File renamed without changes.
0 tests/source/imports_2015_edition.rs → tests/source/imports/imports_2015_edition.rs
View file
Open in desktop
File renamed without changes.
0 tests/source/imports_block_indent.rs → tests/source/imports/imports_block_indent.rs
View file
Open in desktop
File renamed without changes.
0 tests/source/imports_granularity_crate.rs → ...urce/imports/imports_granularity_crate.rs
View file
Open in desktop
File renamed without changes.
6 changes: 6 additions & 0 deletions tests/source/imports/imports_granularity_default-with-dups.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
use crate::lexer;
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some test files that have multiple groups, and perhaps some that have comments too? Maybe something like this:

use crate::lexer::{tokens::TokenData /* foo */ };
use crate::lexer::{tokens::TokenData};

It might also be useful to have some duplicates but with varying visibility levels (we may not handle this well with the other variants either FWIW, but would be good to check while we're working on this one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you add some test files that have multiple groups, and perhaps some that have comments too?

I didn't add such test files as there are some issues, especially related to comments, so maybe it is better to discuss first (here or in a new issue) which of these issues should be resolved, whether there are cases where the original code should be used, what should be the expected results, etc.

For example, the for the following input:

use crate::foo;
use crate::foo::self;
use crate::foo::self /* foo 1 */;
use crate::foo::{self /* foo 2 */ };
use crate::foo::{tokens::TokenData};
use crate::foo::{tokens::TokenData /* foo 3 */ };
use crate::foo::{self, tokens::TokenData /* foo 4 */};
use crate::bar::self /* bar 1 */;
use crate::bar::self;
use crate::foobar::self; /* foobar 1 */
use crate::foobar::self;

"Crate" granularity produce the following output:

use crate::bar; /* bar 1 */;
use crate::foo; /* foo 1 */;
use crate::foobar; /* foobar 1 */
use crate::{
    bar, foo,
    foo::{self /* foo 2 */, tokens::TokenData},
    foobar,
};

Comments foo 3 and foo 4 disappeared and there is an extra ; after comments bar 1 and foo 1.

"Module" granularity produce the following output:

use crate::bar; /* bar 1 */;
use crate::foo; /* foo 1 */;
use crate::foo::tokens::TokenData;
use crate::foo::{self /* foo 2 */, self};
use crate::foobar; /* foobar 1 */
use crate::{bar, foo, foobar};

In addition to the same issues as for "Crate", foo::{...} includes self twice: one with foo 2 comment and one without a comment.

"Item" granularity produce the following output:

use crate::bar; /* bar 1 */;
use crate::foo;
use crate::foo::tokens::TokenData;
use crate::foo::{self /* foo 2 */};
use crate::foobar; /* foobar 1 */

Comment foo 1 is missing because in the original code crate::foo::self is before crate::foo::self /* foo 1 */. The bar 1 comment does appear, since crate::bar::self is before crate::foobar::self; /* foobar 1 */. As above, foo 3 and foo 4 comments disappeared and there is an extra ; after comments bar 1.

Copy link
Member

Choose a reason for hiding this comment

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

as there are some issues, especially related to comments

wow there sure is 🙃

We've always had comment issues with this option (which was named merge_imports prior to a recent rename/restructure) so I'm not too surprised by some of those, but the emission of invalid code in this type of case is particularly concerning:

use rustc_ast::ast /* abc */;

I didn't add such test files as there are some issues, especially related to comments, so maybe it is better to discuss first (here or in a new issue) which of these issues should be resolved, whether there are cases where the original code should be used, what should be the expected results, etc.

Good call 👍

Copy link
Contributor Author

@davidBar-On davidBar-On Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Submitted PR #4759 to fix the extra semicolon issue.

I will also try to resolve the issue where in some cases if the same import module appears twice - once with a comment and once without a comment, it appears twice in the formatted output. E.g. in the above output for Crate, 'create::barandcrate:::foo` appear twice.

Update:
The reason some import items appear twice for Crate and Module granularity, is because merge_use_trees() don't try to merge items with comments and just push them to the output:

rustfmt/src/formatting/imports.rs

Lines 170 to 172 in a9876e8

if use_tree.has_comment() || use_tree.attrs.is_some() {
result.push(use_tree);
continue;

This is probably a patch done because imports comments are not handled well, but it ignores duplicates.

There is a relatively simple way to remove these duplicates by ignoring all non-commented flattened items that have a duplicate with comment. However, because duplicates with and without comments are not expected to be common in practice, and because no issue was opened by users against this problem, it seems that it is not worth trying to add this enhancement.

use crate::lexer::tokens::TokenData;
use crate::lexer::{tokens::TokenData};
use crate::lexer::self;
use crate::lexer::{self};
use crate::lexer::{self, tokens::TokenData};
13 changes: 13 additions & 0 deletions tests/source/imports/imports_granularity_item-with-dups-StdExternalCrate-no-reorder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// rustfmt-imports_granularity: Item
// rustfmt-reorder_imports: false
// rustfmt-group_imports: StdExternalCrate

use crate::lexer;
use crate::lexer;
use crate::lexer::tokens::TokenData;
use crate::lexer::{tokens::TokenData};
use crate::lexer::self;
use crate::lexer;
use crate::lexer;
use crate::lexer::{self};
use crate::lexer::{self, tokens::TokenData};
11 changes: 11 additions & 0 deletions tests/source/imports/imports_granularity_item-with-dups.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// rustfmt-imports_granularity: Item

use crate::lexer;
use crate::lexer;
use crate::lexer::tokens::TokenData;
use crate::lexer::{tokens::TokenData};
use crate::lexer::self;
use crate::lexer;
use crate::lexer;
use crate::lexer::{self};
use crate::lexer::{self, tokens::TokenData};
0 tests/source/imports_granularity_item.rs → ...ource/imports/imports_granularity_item.rs
View file
Open in desktop
File renamed without changes.
0 tests/source/imports_granularity_module.rs → ...rce/imports/imports_granularity_module.rs
View file
Open in desktop
File renamed without changes.
0 tests/source/imports_raw_identifiers.rs → ...source/imports/imports_raw_identifiers.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/import-fencepost-length.rs → ...target/imports/import-fencepost-length.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/imports-impl-only-use.rs → ...s/target/imports/imports-impl-only-use.rs
View file
Open in desktop
File renamed without changes.
0 ...target/imports-reorder-lines-and-items.rs → ...mports/imports-reorder-lines-and-items.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/imports-reorder-lines.rs → ...s/target/imports/imports-reorder-lines.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/imports-reorder.rs → tests/target/imports/imports-reorder.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/imports.rs → tests/target/imports/imports.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/imports_2015_edition.rs → tests/target/imports/imports_2015_edition.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/imports_2021_edition.rs → tests/target/imports/imports_2021_edition.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/imports_block_indent.rs → tests/target/imports/imports_block_indent.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/imports_granularity_crate.rs → ...rget/imports/imports_granularity_crate.rs
View file
Open in desktop
File renamed without changes.
6 changes: 6 additions & 0 deletions tests/target/imports/imports_granularity_default-with-dups.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
use crate::lexer;
use crate::lexer;
use crate::lexer::tokens::TokenData;
use crate::lexer::tokens::TokenData;
use crate::lexer::{self};
use crate::lexer::{self, tokens::TokenData};
7 changes: 7 additions & 0 deletions tests/target/imports/imports_granularity_item-with-dups-StdExternalCrate-no-reorder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// rustfmt-imports_granularity: Item
// rustfmt-reorder_imports: false
// rustfmt-group_imports: StdExternalCrate

use crate::lexer;
use crate::lexer::tokens::TokenData;
use crate::lexer::{self};
5 changes: 5 additions & 0 deletions tests/target/imports/imports_granularity_item-with-dups.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// rustfmt-imports_granularity: Item

use crate::lexer;
use crate::lexer::tokens::TokenData;
use crate::lexer::{self};
0 tests/target/imports_granularity_item.rs → ...arget/imports/imports_granularity_item.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/imports_granularity_module.rs → ...get/imports/imports_granularity_module.rs
View file
Open in desktop
File renamed without changes.
0 tests/target/imports_raw_identifiers.rs → ...target/imports/imports_raw_identifiers.rs
View file
Open in desktop
File renamed without changes.