Deduplicate imports

[IceTDrinker]

Hello!

I searched the issues and could not find a similar issue, if there is already one on that topic I missed feel free to close this one.

I find myself solving some merge conflicts lately that happen for use statements, sometimes ending up with more than one import of the same name e.g.:

use std::vec::{Vec, Vec};

And running rustfmt on that does not deduplicate the import.

In practice the use statements causing issues are much bigger and it can be a bit of pain to deduplicate imports manually. In addition, the duplicate import prevents compilation because of the name Vec being redefined

error[E0252]: the name Vec is defined multiple times

Rust playground:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0cd8ef07ca6cde1a2892c9b8e1697ff4

It feels like it should be possible as I've already seen rustfmt merging multiple imports from the same module into one use statement, so there is a mechanism to group names together and sort them, I'm guessing that switching the underlying container for the grouped names to some Set-like object should do the trick.

I haven't had time to check the source for that particular case, but I thought I might as well open the issue here if someone knew where the code for this is and how it could be changed to manage that case 🙂

Cheers!

[ytmimi]

Thanks for reaching out.

If you're able to use a nightly version of rustfmt you can set the imports_granularity option to anything other than the default of Preserve and the import should be deduplicated.

Let me know if that helps.

@calebcartwright when set to Preserve should we deduplicate?

We recently merged #5380, which focused on deduplicating imports for the Item case so I imagine a similar change would be needed to resolve this in the Preserve case.

Linking the tracking issue #4991

[ytmimi]

looking into this a little bit more, I don't think we can take the exact same approach as #5380, however I think a good place to perform the deduplication would be normalize_use_trees_with_granularity for the Preserve case.

rustfmt/src/imports.rs

Lines 213 to 223 in c4416f2

	pub(crate) fn normalize_use_trees_with_granularity(
	use_trees: Vec<UseTree>,
	import_granularity: ImportGranularity,
	) -> Vec<UseTree> {
	let merge_by = match import_granularity {
	ImportGranularity::Item => return flatten_use_trees(use_trees, ImportGranularity::Item),
	ImportGranularity::Preserve => return use_trees,
	ImportGranularity::Crate => SharedPrefix::Crate,
	ImportGranularity::Module => SharedPrefix::Module,
	ImportGranularity::One => SharedPrefix::One,
	};

[ytmimi]

Although I have a proof of concept for how we could go from use std::vec::{Vec, Vec}; -> use std::vec::Vec; We currently have a test case in tests/target/multiple.rs that goes against this kind of normalization:

rustfmt/tests/target/multiple.rs

Lines 19 to 23 in c4416f2

	use std::{
	self, any, ascii, borrow, borrow, borrow, borrow, borrow, borrow, borrow, borrow, borrow,
	borrow, borrow, boxed, boxed, boxed, boxed, boxed, boxed, boxed, boxed, boxed, boxed, char,
	char, char, char, char, char, char, char, char, char,
	};

Because of that I'm not sure if we should try to change this for the Preserve case, and it might be worth considering a different imports_granularity value if you're using a nightly version of rustfmt.

[IceTDrinker]

What is the rationale behind keeping duplicates? (as it's not valid rust code it seems?)

[ytmimi]

What is the rationale behind keeping duplicates? (as it's not valid rust code it seems?)

Great question. Just want to say up front that I'm just enumerating some thoughts on why we might consider leaving this alone. I'm not trying to suggest that this is a problem that shouldn't be solved.

From the standpoint of the rustfmt project we need to ensure that we don't introduce breaking formatting changes. Given that this is the default behavior that's been around since the start of the project this would be a breaking change. That being said, we have options to introduce breaking changes. We can version gate them using the version config. We can also add new unstable options or variants to existing options. Recently there's been more discussion about stabilizing the imports_granularity config, so I don't think adding a new unstable option is possible at the moment #4991 (comment)

From the perspective of the implementation, we'd need to weigh the added complexity of the fix on the codebase against other options the developer has to fix the problem. In this case one could use a different imports_granularity level or remove the duplicates manually. We'd also need to consider the fix against other relevant configuration options. In this particular case we need to make sure the fix is viable when reorder_imports=true (the default) and when reorder_imports=false

Lastly, the case you posed is simple enough, but the problem of removing duplicate imports gets complicated quickly. Are we only going to consider the simple case use std::vec::{Vec, Vec};? Here are some others that illustrate the complexity:

• nested duplicates use a::b::{c::{d, e}, c::{e, f}};
• duplicates with comments use std::vec::{Vec /* do you keep me? */, Vec /* do you drop me? */};
• consecutive nested duplicates use a::b::c::{d, e}; use a::b::c::{e, f};

[IceTDrinker]

Thanks a lot for the insights 🙏

From afar apart the comment one which seems a bit unavoidable and pathological, the others can be deduplicated with the normalization/flattening part I think I saw in the code? and then recompose them with the proper import granularity

[calebcartwright]

Thank you for sharing @IceTDrinker and @ytmimi for providing context and details.

There's been some prior discussions on this topic which I can't find at the moment so will attempt to recap.

The historical, and thus the continued, behavior of rustfmt was to only attempt to deduplicate with the user gave rustfmt the permission to merge/modify imports, originally via the merge_imports option which was rebranded and extended as imports_granularity. So long as imports_granularity is disabled (the default), rustfmt isn't going to attempt in-place deduplication.

As such my inclination is that we should close this. The only thing we could do would be to introduce yet another imports related config option that allowed for dedupes but banned any form of granularity normalization, and while I acknowledge there's probably a non-zero set of users that would want that, I don't think there's anywhere near enough volume to justify the added complexity of the config surface/mental model around import formatting and associated code maintenance.

[jollygreenlaser]

At the moment Module also does not dedupe: #6243

I agree with leaving Preserve as is. Semantically, Preserve is essentially leaving things as they are. Per the docs, preserve the original structure written by the developer. It is also the default option. If there were ever bugs/edge cases in the other options, there must be some "disabled" state for this part of format.