Expand unnecessary-testable to unnecessary-import

[sebastianv1]

A superficial look at unnecessary-testable makes it seem somewhat trivial to also potentially write an unnecessary-import tool that exposes unused imports similar to SwiftLint's UnusedImport rule. A few fixes like removing the isPublic check seemed to work on a sample project, but curious if there be dragons. Would be cool to add it into the repo

[keith]

Yea good question. At Lyft we're using SwiftLint for this, so I have a slight preference to improving SwiftLint's version (if needed) vs adding another option for that here. I think you're probably right that we could do something like what you suggest, but there are some outlier issues that require imports but don't appear in the index.

Did you hit issues with SwiftLint's? cc @jpsim

[jpsim]

Yes there are dragons, but I don't remember exactly what they look like. I think it's possible and worthwhile to build but haven't looked at it in a while.

[sebastianv1]

but there are some outlier issues that require imports but don't appear in the index.

As in they don't show up in the referenced USRs and would return a false positive (thinks something is unused but actually is)? Additionally, does SourceKit use a different mechanism to fetch USRs? I'm curious if those kind of false positives would also exist in SwiftLint's rule 🤔

In the meantime I'll just go ahead and run it over our main repo and see what happens. Will report back

[keith]

Well at least with the current implementation of unnecessary-testable we just manually opt out the broken ones in our codebase. You might not have as many depending on how much you hit the weird cases. Maybe operator overloading is another case right now?

[sebastianv1]

I ran my implementation of unnecessary-import over our repo and it seemed to work well, it ran into the same false positives SwiftLint's unused imports rule had and even picked up a couple more than SwiftLint 🤷

Beneficially, it's much faster than SwiftLint w/ Bazel. We only run SwiftLift over the set of changed files since it's pretty slow over our whole repo (which also isn't 100% accurate since moved files can cause unused imports to go unchecked).

[keith]

Nice. Yea that's where I wonder if we should try to improve swiftlint however we can, even if that means potentially depending on the library here, since I think that fits in really well there. Can you submit a PR with your changes? I can try it on our codebase as well.

[sebastianv1]

Here's just what I had stashed locally: #2

I was thinking of cleaning it up by creating a new Driver class with a lot of the shared logic and pulling out changes into protocols that are injected. Thoughts? Makes sense about improving in SwiftLint too.

[keith]

thanks, testing it out on our project. it would probably be nice to be less restrictive about the import format, and removing it if it did have other annotations etc, but that could be a future improvement. I think doing it as a separate tool would be fine. the only thing i would like to avoid is us adding that separate tool, immediately fixing swiftlint, and then deprecating it instead, but maybe the work there will be too significant for that to be a problem.

[keith]

quite a few false positives in our case, i think the case i mentioned above about initializers in extensions was a major issue, i'd have to debug more to see what other common cases it hit

[sebastianv1]

Is this a sample false positive scenario you're describing?

import Bar // <-- Potentially marked as unused?
import Baz // <-- Potentially marked as unused?

extension Foo {
    init(a: Bar.A, b: Baz.B) { ... }
} 

[keith]

I think it's:

• module A defines the type
• module B extends and adds an initializer
• module C imports both and calls the initializer from B, the B import is marked as unused

[sebastianv1]

I tried that scenario that it seemed to work (attached a sample small project if you want to play around with it), which makes me think there might be some hidden intricacies.
UnusedTest.zip

[keith]

so actually i debugged a bit in our case and I think right now this tool is just entirely wrong for this use case? the problem being currently for unnecessary-testable it just assumes the import is necessary, and was just checking whether or not it should remove the @testable, so in our case it just removes super clear imports for everything, because it's not checking all the modules being referenced

[sebastianv1]

Okay interesting. I have a pretty hand-wavy understanding of units/records, but is the call to getReferenceUSRs not gathering all potential record references (and thus modules) and just some? For it to work beyond just testable I removed the isPublic check, but I guess I don't follow from the code how "it just assumes the import is necessary"?

[keith]

You're right, I was tricked by an invalid index store that produced invalid results on our project, sorry about that. I'm editing a bit and see some wild results on our project, need to debug more

[sebastianv1]

Testing our repo, and one case this doesn't cover well (and maybe what you're seeing) are imports behind a macro. We read the file and regex for imports, but the required units may or may not exist depending on compilation flags (i.e #if RELEASE).

The correct approach is to probably retrieve the imports out of the units themselves instead of the raw file, is there a good way to do this? From the usr symbols I noticed all imports have the format c:@M@<modulename>$ (end of line for $), but not sure if this is a stable way to retrieve the imports.

[keith]

I'm not sure if those references are only for imports, you might also get them if you use the module name to fully qualify a type as well? Maybe that doesn't matter though because to fully qualify a type you'd have to have done that import as well.

But yea in general #ifs make any unused import logic harder.

[sebastianv1]

you might also get them if you use the module name to fully qualify a type as well

Yeah you were exactly right about this. Getting some Swift "imports" with that

[sebastianv1]

So a bit of an update: I've been running this over our repo on CI and have noticed some flakiness where it'll fail for some modules thinking the module is unused when in fact it's not, and pass on a subsequent run with effectively the same code.

I added logging and it appears that some of the unit files won't be included in the IndexStore's UnitReaderSequence. For instance, the unit reader sequence will only have 10 out of the 15 files in a module. Runs where the script succeeds will have the same count of units being iterated over, whereas when the script flakes it's always a lower number of units than expected. I've even zipped the IndexStore and downloaded it from runs that flaked but haven't been able to repro locally as it'll correctly load all the units and pass 🤔

Have you ever seen behavior like this before? Maybe there's something weird happening by calling into indexstore_store_units_apply_f?

[keith]

Interesting, I haven't ever spotted that, although for the past few years this has run on a cron for us and if it fails one day and passes the next I likely wouldn't investigate either. I'm very surprised that given you downloaded the index store and it had the right units that that would happen. Could it be a difference in Xcode version or anything? (internally we bundle a static version of the dylib we reference to avoid that dependency entirely)

[sebastianv1]

Re the Xcode version, we're using Bazel here and have an xcode_config set on all bazel runs in our rc to a specific version so I would expect some consistency in toolchain versions; however I didn't bundle it statically and am just referencing it like so in the BUILD file. Do you think that could cause issues (I think our CI machines do have multiple versions installed at the moment too)?

macos_command_line_application(
    name = "unused_imports",
    minimum_os_version = MINIMUM_MANAGE_MACOS_VERSION,
    platform_type = "macos",
    linkopts = [
    	"-L__BAZEL_XCODE_DEVELOPER_DIR__/Toolchains/XcodeDefault.xctoolchain/usr/lib", 
    	"-rpath", 
    	"__BAZEL_XCODE_DEVELOPER_DIR__/Toolchains/XcodeDefault.xctoolchain/usr/lib"
    ],
    deps = [
        ":UnusedImports",
    ],
)

[keith]

i think that looks right, should be invalidated with xcode version changes too

[sebastianv1]

Follow up: I figured out that the flakiness was due to index-import running async and so blocking on that operation seems to have resolved it. I have our tool running on all developers diffs now and things seem to be going well, much faster than SwiftLint.

I'll continue to monitor over the next week or so, but if there's interest I could open a PR with my implementation removing some of our repo specific stuff behind protocols.

[keith]

I would definitely be interested to see the new diff! Because I haven't looked more but still see those various edge cases

[jpsim]

If it's possible at all to do this only with information from the index store, it should definitely be orders of magnitude faster than what SwiftLint is doing.

When we moved from SwiftLint's unused_declaration rule to one based on swift-index-store, processing our codebase went from a few hours to a few seconds.

[keith]

done as part of https://github.com/lyft/swift-index-store/releases/tag/1.2.0, please report individual issues that you find with this new tool!