Skip to content
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.

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

Add support for adding missing imports #29

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

keith
Copy link
Member

@keith keith commented May 24, 2023

This works by collecting all definition USRs from all units, and cross
referencing all reference USRs from each file, until there are no
unhandled USRs remaining.

This works by collecting all definition USRs from all units, and cross
referencing all reference USRs from each file, until there are no
unhandled USRs remaining.
@keith
Copy link
Member Author

keith commented May 24, 2023

there are a few cases this adds imports for that might be surprising:

  • In this case module c gets an import for module a
// module a
class A {
   let property: String
}

// module b
class B: A {}

// module c
self.b.property
  • typealiases + extensions are a bit weird, this case gets an import to A because the extension is technically on A (AFAIUI):
// module a
struct A {}

// module b
typealias B = A

extension B {
  func foo() {}
}

// module c
B().foo()

maybe some more, but most were pretty understandable and more surprising that they could be missing (mostly the usual suspects like extensions on system types, operators, uses of callAsFunction etc)

if !newImports.isEmpty {
let lines = newImports.sorted().map { "import \($0)" }.joined(separator: "\\\n")
let relativePath = unitReader.mainFile.replacingOccurrences(of: pwd + "/", with: "")
print("/usr/bin/sed -i \"\" '1i\\\n\(lines)' \(relativePath)")

Choose a reason for hiding this comment

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

Breaks on paths with spaces and stomps on the current line contents

Suggested change
print("/usr/bin/sed -i \"\" '1i\\\n\(lines)' \(relativePath)")
print("/usr/bin/sed -i \"\" '1i\\\n\(lines)\n' '\(relativePath)'")

Choose a reason for hiding this comment

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

Inserting these on line 1 ends up colliding with our copyright preamble:

-//
+import SomeModuleName//
 // Copyright © Square, Inc. All rights reserved.
 //

Maybe we find the line of the first import statement instead? Even better would be to examine the current list and insert it alphabetically into the right place.

Copy link
Member Author

Choose a reason for hiding this comment

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

yea this was just a quick and easy solution. as far as importing them in order it's probably easier to just let swiftlint handle that if you enforce that (it even autocorrects that case)

}

if !definitions.usrs.intersection(remainingUSRs).isEmpty {
usedImports.insert(dependentUnit.moduleName)

Choose a reason for hiding this comment

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

When dependentUnit is a .m file the moduleName is the empty string, is that expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

hrm. interesting. i would expect that with -fmodules it might be set, but maybe that's just not at the right level for this. i guess in the meantime worst case is we ignore units with empty module names so we don't try to do anything weird and insert an empty string. i don't think we would be able to trace back to a header, but if you're using modules it seems like it should be possible to trace back to that (but then i would expect the unit to be for the module maybe, not the single .m file, similar to the SDK's modules). I didn't hit this since we don't have any objc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants