Add a code action to remove unused imports in a source file

[ahoppen]

The idea is pretty simple: When MemberImportVisibility is enabled, we know that imports can only affect the current source file. So, we can just try and remove every single import declaration in the file, check if a new error occurred and if not, we can safely remove it.

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows

[ahoppen]

Sleeping over this, I think there is at least one case where this action could change behavior.

LibA.swift

func foo(_ x: Int) -> Int { "Wrong" }

LibB.swift

func foo(_ x: Double) -> Int { "Correct" }

Test.swift

import LibA
import LibB

print(foo(1.2))

The action will remove the import to LibB because the code still compiles fine without it (we now pick the foo(_:Int) overload instead of foo(_:Double)). But I think these cases are extremely rare, so I wouldn’t worry about them for now.

[tshortli]

It's probably not too impactful to your goal, but MemberImportVisibility does not really guarantee that a source file only depends on its direct imports. Conformances are one example of something that can be used from a source file without a direct import. Re-exports also make the analysis hard in the general case since the same declarations may be reachable through multiple imports in the same file.

[tshortli]

As a concrete example of the difficulty with conformances, an "unused" import in one source file could be the only import in the entire module that makes a necessary conformance visible in another source file. So removing that unused import would cause an error in another file.

[ahoppen]

Oh, thanks for mentioning the conformance issue. I only thoughts about members in extensions, not about the actual conformance that might be coming from a different file in the same module.

I’m on the edge now whether we should take this, knowing that it’s technically incorrect or if we say it’s good enough in most cases (which I suspect it is).

[bnbarham]

I’m on the edge now whether we should take this, knowing that it’s technically incorrect or if we say it’s good enough in most cases (which I suspect it is).

If this was a compiler fix-it then I'd definitely be in the "no" boat, but for a refactoring... it's probably still fine? Worst case the build has an error, which should result in the import being added to the correct file anyway.

[tshortli]

How will the refactoring handle code like this?

import Foundation
import Dispatch

func test(_ queue: DispatchQueue) {
  queue.sync { }
}

Either import can be removed since Foundation is not used directly anywhere but re-exports Dispatch. This is what I mean about re-exports making the analysis difficult. I think ideally only the Dispatch import would be kept as the most specific import that is required by the code, but that can't really be determined outside of the compiler.

[ahoppen]

The refactoring action just tries to remove imports in the reverse order, so in this case it would remove import Dispatch. If the imports would have been spelled the other way round, import Foundation would have been removed. Which seems good enough for me. Otherwise you could argue that this action would try and minimize imports and should thus also replace import Foundation with import Dispatch if only re-exported declarations are used. And I think that’s out-of-scope (at least for now).

[ahoppen]

Also added a few comment lines on the known drawbacks of this refactoring action and why we decided to include it despite these.

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows

[ahoppen]

@swift-ci Please test Windows

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows