[ModuleTrace] Emit import graph to ease debugging import issues. #33980
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It used to be a method because it used a cache in ABIDependencyEvaluator; however that cache has been removed.
When looking at a module being shadowed by a separate (cross-import) overlay:
before:
filter (potentially) = {ShadowedBySeparateOverlay}
requiredFilter = {Private, ShadowedBySeparateOverlay}
filter.contains(requiredFilter) = false // incorrect
after:
filter (potentially) = {ShadowedBySeparateOverlay}
requiredFilter = {ShadowedBySeparateOverlay}
filter.contains(requiredFilter) = true
This should make it easier to spot cycles in import graphs, as well as funky behavior involving both default/private imports + SPI imports.
|
@swift-ci please test |
|
Build failed |
|
Build failed |
Comment on lines
+1194
to
-1199
| else if (!separatelyImportedOverlays.lookup(desc.module.importedModule).empty()) | ||
| requiredFilter |= ModuleDecl::ImportFilterKind::ShadowedBySeparateOverlay; | ||
| else | ||
| requiredFilter |= ModuleDecl::ImportFilterKind::Private; | ||
|
|
||
| if (!separatelyImportedOverlays.lookup(desc.module.importedModule).empty()) | ||
| requiredFilter |= ModuleDecl::ImportFilterKind::ShadowedBySeparateOverlay; | ||
|
|
There was a problem hiding this comment.
This "fix" is incorrect. The ShadowedBySeparateOverlay filter is essentially a different axis altogether.
| @_exported | private | @_implementationOnly | @_spi
-------------+------------+---------+----------------------+------
Not shadowed | EN | PN | IN | SN
Shadowed | ES | PS | IS | SS
Ideally, we'd be passing in two OptionSets, not one IMO. If you pass in newFilter = {{@_exported}, {not shadowed}}, you get EN. If you pass in newFilter = {{@_exported | private}, {shadowed}}, you'd get ES and PS.
Today, if you pass in filter = {@_exported}, you get EN whereas if you pass in filter = {shadowed}, you get nothing. That's unnecessarily confusing.
There was a problem hiding this comment.
Also, @_spi is a different axis, so the table in ^ is wrong.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds functionality to see an import graph describing the imports of different modules.
Here is an example for a module only importing
Combine, which has a small dependency footprint.In practice, you'll likely have a much larger dependency footprint. For example, here's what you're dragging in if you import
MapKitandSwiftUI.There are still some non-determinism issues in the graphviz output, need to look at the printing code again more carefully.
Marking as a draft because there are some unresolved questions:
jqbut for GraphViz graphs), but unfortunately I don't think such a tool exists. It seems like it would be easy functionality to add though, maybe even within the scope of aStarterBug.Current TODOs:
Answering potential questions:
Q: Why have redundancy in the colors and the "(S)"/"(C)"?
A: The colors make it easier to spot things for people who can distinguish orange/grey. If you cannot visually distinguish the colors, the additional "(S)" and "(C)" are helpful.
Q: Can you change the edges labels to use a different scheme/convention instead?
A: Yes, I'm happy to change those, so long as the new scheme shows both default + SPI when both imports are used along the same edge (leading to a compiler warning). Simply showing the attribute (@_exported/@_implementationOnly/@_spi), and hence not showing "Def" at all, doesn't have that property.