Skip to content

Dependencies: validate Swift imports against MODULE_DEPENDENCIES and provide fix-its (rdar://150314567) #620

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mirza-garibovic
Copy link
Contributor

Dependencies: validate Swift imports against MODULE_DEPENDENCIES and provide fix-its (rdar://150314567)

Also includes fixes for:

  • Macro: fix an issue where locations from xcconfig files are lost across condition binding
  • launch-xcode: honor DEVELOPER_DIR

@mirza-garibovic
Copy link
Contributor Author

@swift-ci test

@mirza-garibovic mirza-garibovic requested a review from artemcm June 27, 2025 21:58
}

public init(entry: String) throws {
let components = entry.split(separator: " ")
Copy link
Collaborator

@jakepetroules jakepetroules Jun 27, 2025

Choose a reason for hiding this comment

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

I suggest using Swift regex for this. Makes it much easier to read and understand the code flow (and no force unwraps that risk crashing if this becomes more complex in future).

guard let match = try #/(?<accessLevel>.+)? (?<moduleName>)/#.wholeMatch(in: entry)?.output else {
    throw StubError.error("expected 1 or 2 space-separated components in: \(entry)")
}

self.accessLevel = try match.accessLevel.map { accessLevel in
    if let a = AccessLevel(rawValue: String(accessLevel)) {
        return a
    } else {
        throw StubError.error("unexpected access modifier '\(accessLevel)', expected one of: \(AccessLevel.allCases.map { $0.rawValue }.joined(separator: ", "))")
    }
} ?? .Private
self.name = String(match.moduleName)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like a better way to define this syntax instead of "manually" splitting like this, but I'm not sure about Swift Regex here because of https://forums.swift.org/t/should-regex-be-sendable/69529 (you can't construct the regex literal once and reuse it if it isn't Sendable). I think my preference is to keep it simple until it gets more complicated and then re-evaluate. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough but then I'd at least consider doing this:

let components = entry.split(separator: " ")
switch components.count {
case 1:
    accessLevel = .Private
    self.name = components[0]
case 2:
    accessLevel = try AccessLevel(string: components[0])
    self.name = components[1]
default:
    throw StubError.error("expected 1 or 2 space-separated components in: \(entry)")
}

(elsewhere)

fileprivate extension AccessLevel {
    init(string: String) throws {
        guard let accessLevel = AccessLevel(rawValue: string) else {
            throw StubError.error("unexpected access modifier '\(string)', expected one of: \(AccessLevel.allCases.map { $0.rawValue }.joined(separator: ", "))")
        }
        self = accessLevel
    }
}

The main point being to localize checking in a way that makes it as obvious as possible at a glance that it's correct (and avoiding a force unwrap).

@jakepetroules
Copy link
Collaborator

If there's a build setting assignment in xcconfig, you get a diagnostic with a fixit, otherwise if it's set on a project model entity directly (project/target), you get a diagnostic only -- right?

@mirza-garibovic
Copy link
Contributor Author

If there's a build setting assignment in xcconfig, you get a diagnostic with a fixit, otherwise if it's set on a project model entity directly (project/target), you get a diagnostic only -- right?

Yup

@mirza-garibovic
Copy link
Contributor Author

@swift-ci test

@mirza-garibovic
Copy link
Contributor Author

@swift-ci test

@mirza-garibovic mirza-garibovic force-pushed the pr-swift-mod-deps branch 2 times, most recently from a353218 to 052f2b6 Compare June 30, 2025 17:02
Copy link
Contributor

@bob-wilson bob-wilson left a comment

Choose a reason for hiding this comment

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

I'm no expert in many aspects of this, but for the parts that I understand, it looks pretty good.

@jakepetroules
Copy link
Collaborator

Looks like there's still some test failures on Linux.

@mirza-garibovic
Copy link
Contributor Author

Looks like there's still some test failures on Linux.

I changed the test to use host instead of macOS so I can get Linux and Windows coverage, going to work through the issues.

@mirza-garibovic
Copy link
Contributor Author

@swift-ci test

@mirza-garibovic
Copy link
Contributor Author

Checks are green! @jakepetroules, any remaining concerns from you?

@@ -40,8 +40,8 @@ struct LaunchXcode: CommandPlugin {

print("Launching Xcode...")
let process = Process()
process.executableURL = URL(fileURLWithPath: "/usr/bin/open")
process.arguments = ["-n", "-F", "-W", "--env", "XCBBUILDSERVICE_PATH=\(buildServiceURL.path())", "-b", "com.apple.dt.Xcode"]
process.executableURL = URL(fileURLWithPath: "/bin/sh")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about this one.xcode-select -p does not necessarily return an Xcode instance, it could be Developer Command Line Tools, in which case this will just try to open /Developer and fail.

Could you run xcode-select -p in a separate Process() invocation, collect its output, and call open with that path if the return value ends in .app, otherwise fall back to -b com.apple.dt.Xcode?

Also suggest splitting into another PR so that concerns with that don't block your primary changes.

}

public init(entry: String) throws {
let components = entry.split(separator: " ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough but then I'd at least consider doing this:

let components = entry.split(separator: " ")
switch components.count {
case 1:
    accessLevel = .Private
    self.name = components[0]
case 2:
    accessLevel = try AccessLevel(string: components[0])
    self.name = components[1]
default:
    throw StubError.error("expected 1 or 2 space-separated components in: \(entry)")
}

(elsewhere)

fileprivate extension AccessLevel {
    init(string: String) throws {
        guard let accessLevel = AccessLevel(rawValue: string) else {
            throw StubError.error("unexpected access modifier '\(string)', expected one of: \(AccessLevel.allCases.map { $0.rawValue }.joined(separator: ", "))")
        }
        self = accessLevel
    }
}

The main point being to localize checking in a way that makes it as obvious as possible at a glance that it's correct (and avoiding a force unwrap).

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.

5 participants