Delete duplicate imports.

[dylansturg]

The criteria for deletion is:

• Has an identical path, excluding whitespace, as an existing import
• No more than 1 of the matching imports have a trailing comment

The trailing comment rule is necessary because it's impossible to safely combine multiple trailing comments such that they all stay grouped to the relevant import. Leading comments are safe to combine, because OrderedImports keeps multiple lines of leading comments grouped to the next import. Rather than trying to keep the trailing comments on 1 line together or promote 1 trailing comment to a leading comment, it's safer to raise the diagnostic and keep both imports.

[allevato]

What happens if someone has this?

import Foo
@testable import Foo

and is the result different than if they had this?

@testable import Foo
import Foo

I guess there are two questions here:

1. Is that valid, or does the compiler complain (at Sema time) if you import the same module both testably and non-testably; I don't know off the top of my head what happens here.
2. Does order matter here, or do we need to prioritize them in some way?

I suppose the same thing could happen for other attributes that aren't technically supported yet, like @_exported or @_implementationOnly.

Now that I've written that last sentence, maybe the right thing to do for attributed imports is not to deduplicate them at all; that way we don't have to deal with which attributes "semantically override" the others.

Can you add a test or two that covers these cases?

[SDGGiesbrecht]

What happens if someone has this?

import Foo
@testable import Foo

and is the result different than if they had this?

@testable import Foo
import Foo

Last I checked, the compiler mistook the second for having already been covered by the first and skipped it. The second example was actually testable; the first actually wasn’t. But that is probably a bug. I first ran into it when I wrote something like this...

import Module
@testable import struct Module.Structure

...and nothing was testable, but I changed it to...

@testable import struct Module.Structure
import Module

...and only the structure was testable, but everything was available. That had been my original intention.

[dylansturg]

I guess there are two questions here:

1. Is that valid, or does the compiler complain (at Sema time) if you import the same module both testably and non-testably; I don't know off the top of my head what happens here.

It looks like this is fine, at least from a compilation perspective and doesn't even raise a warning. Based on the comments from @SDGGiesbrecht, we might want to reconsider the fact that @testable imports are always sorted below other imports.

2. Does order matter here, or do we need to prioritize them in some way?

It sounds like order does matter, again based on the previous comment.

Now that I've written that last sentence, maybe the right thing to do for attributed imports is not to deduplicate them at all; that way we don't have to deal with which attributes "semantically override" the others.

I agree. I've updated the implementation to consider all pieces of the import decl: the attributes, modifiers, kind, and path for de-duplication.

Interestingly, we have special handling for the @testable attribute but imports with any other attribute are treated as "regular" imports and are sorted based on the import path disregarding attributes. We might want to consider some additional effort to support the order requirements of @testable import and how to handle unrecognized attributes (keep mixing them in with regular imports? group by attribute?).

[allevato]

Based on the comments from @SDGGiesbrecht, we might want to reconsider the fact that @testable imports are always sorted below other imports.

Yeah, as much as I don't want to have us worry too much about questionably-defined behavior, if order matters in that situation, then our current behavior for @testable would break that code without an ignore directive.

Let's merge this for now and then consider what to do for @testable imports long-term.