Skip to content

don't assert when a module re-exports a module and one of its symbols#64479

Merged
QuietMisdreavus merged 1 commit intomainfrom
QuietMisdreavus/duplicate-reexport
Mar 22, 2023
Merged

don't assert when a module re-exports a module and one of its symbols#64479
QuietMisdreavus merged 1 commit intomainfrom
QuietMisdreavus/duplicate-reexport

Conversation

@QuietMisdreavus
Copy link
Copy Markdown
Contributor

Resolves rdar://106807038

SymbolGraphGen currently asserts if a module includes re-exports like the following:

@_exported import OtherModule
@_exported import class OtherModule.SomeClass

This is because there's a validity check in the code that handles re-exports to ensure that duplicate symbol data does not end up in the symbol graph. However, this is valid Swift and should be treated as such.

This PR updates the exported-import handler to skip qualified re-exports that come from a module that is also being re-exported as a whole. The qualified symbol in question should already be counted because it is in the module being blanket exported.

@QuietMisdreavus
Copy link
Copy Markdown
Contributor Author

@swift-ci Please smoke test

@QuietMisdreavus
Copy link
Copy Markdown
Contributor Author

I double-checked the symbol graph generated by Swift 5.7.2 (in Xcode 14.2) in macOS where assertions were disabled, and it seems like the "duplicate" symbols were already being filtered out by SymbolGraphGen's use of a DenseSet to store symbols. It may be worthwhile to remove this assertion entirely, especially since it would remove the extra iteration of the symbol listing and creation of a new set in assert builds.

Copy link
Copy Markdown
Contributor

@artemcm artemcm left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable change to me.

@QuietMisdreavus QuietMisdreavus merged commit f3ad288 into main Mar 22, 2023
@QuietMisdreavus QuietMisdreavus deleted the QuietMisdreavus/duplicate-reexport branch March 22, 2023 15:39
etcwilde pushed a commit to etcwilde/swift that referenced this pull request Apr 19, 2023
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