Skip to content

[Fast Dependency Scanner] Ensure Swift modules don't depend on self. #32083

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

Merged
merged 1 commit into from
May 29, 2020

Conversation

artemcm
Copy link
Contributor

@artemcm artemcm commented May 29, 2020

When resolving direct dependencies for a given Swift module, we go over all Clang module dependencies and add, as additional dependencies, their Swift overlays. We find overlays by querying ASTContext::getModuleDependencies with the Clang module's name. If the Clang module in question is a dependency of a Swift module with the same name, we will end up adding the Swift module as its own dependence.

e.g.

  • Swift A depends on Clang A
    • Add Clang A to dependencies of Swift A
  • Look for overlays of Clang A, by name, and find Swift A
    • Add Swift A to dependencies of Swift A

From what I can tell, the logic upstream is sound, and getModuleDependencies is doing the right thing, so this change is simply restricting what gets added when we are resolving direct dependencies and looking for overlays.

Resolves rdar://problem/63731428

@artemcm artemcm requested review from DougGregor and nkcsgexi May 29, 2020 17:14
@artemcm
Copy link
Contributor Author

artemcm commented May 29, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 14051e0d5b5bee5da52a0d911147d04a758a579c

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 14051e0d5b5bee5da52a0d911147d04a758a579c

@artemcm
Copy link
Contributor Author

artemcm commented May 29, 2020

@swift-ci test

@@ -125,12 +125,15 @@ static std::vector<ModuleDependencyID> resolveDirectDependencies(
knownModules);
}

// Look for overlays for each of the Clang modules. The Swift module
// Look for overlays for each of the Clang modules. The Swift module
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Look for overlays for each of the Clang modules. The Swift m�odule
// Look for overlays for each of the Clang modules. The Swift module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch. I need to figure out why my Xcode keeps inserting these in strange places.

When resolving direct dependencies for a given Swift module, we go over all Clang module dependencies and add, as additional dependencies, their Swift overlays. We find overlays by querying `ASTContext::getModuleDependencies` with the Clang module's name. If the Clang module in question is a dependency of a Swift module with the same name, we will end up adding the Swift module as its own dependence.

e.g.
- Swift A depends on Clang A
  - Add Clang A to dependencies of Swift A
- We look for overlays of Clang A, by name, and find Swift A
  - Add Swift A to dependencies of Swift A

From what I can tell, the logic upstream is sound, and `getModuleDependencies` is doing the right thing, so this change is simply restricting what gets added when we are looking for overlays.

Resolves rdar://problem/63731428
@artemcm artemcm force-pushed the RecycleYourSwiftModules branch from 14051e0 to 99a0919 Compare May 29, 2020 18:55
@artemcm
Copy link
Contributor Author

artemcm commented May 29, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 14051e0d5b5bee5da52a0d911147d04a758a579c

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 14051e0d5b5bee5da52a0d911147d04a758a579c

@artemcm artemcm merged commit 26125f5 into swiftlang:master May 29, 2020
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.

3 participants