Skip to content

[5.6][AST] scan @_exported imports of source files for display decls #40866

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 4, 2022
Merged

[5.6][AST] scan @_exported imports of source files for display decls #40866

merged 5 commits into from
Feb 4, 2022

Conversation

QuietMisdreavus
Copy link
Contributor

@QuietMisdreavus QuietMisdreavus commented Jan 15, 2022 •
edited
Loading

Cherrypick of #40810

Explanation: In -emit-module builds, symbols declared in headers were not being emitted in symbol graphs, due to SourceFile-based modules not including decls from the underlying Clang module.

Scope: This is a modification of getDisplayDecls, but it has been restricted to only affect SymbolGraphGen.

Issue: rdar://85230067

Risk: Low. While this is a change to getDisplayDecls, the change has been restricted to only occur during SymbolGraphGen, and should not affect regular compilation.

Testing: Source compatibility has been tested with this change, and no regressions have occurred. In addition, the following lit tests have been updated or added:

  • SymbolGraph/ClangImporter/EmitWhileBuilding has been updated to ensure that Clang-based symbols appear during an -emit-module build.
  • SymbolGraph/ClangImporter/Submodules has been added to ensure that re-exporting a Clang submodule does not crash the process, and to ensure that re-exporting multiple Clang modules does not trip the "duplicate decls" assertion added to the recursive getDisplayDecls.
  • ModuleInterface/exported-import has been added to ensure that re-exporting a module does not add those symbols to the generated module interface file.

@QuietMisdreavus QuietMisdreavus requested a review from a team as a code owner January 15, 2022 00:46
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test source compatibility

@QuietMisdreavus QuietMisdreavus force-pushed the QuietMisdreavus/5.6/display-decl-recur branch from 7d055bd to 97b46c4 Compare January 19, 2022 23:49
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test source compatibility

@QuietMisdreavus
Copy link
Contributor Author

This PR now includes the change from #40841, as it was reverted in #40880.

@QuietMisdreavus
Copy link
Contributor Author

It looks like there were problems i'd overlooked in the source-compatibility since it had passed for main. I'm going to investigate them this morning.

(FAIL_Doggie_5.0_BuildSwiftPackage.log)

error: emit-module command failed due to signal 6 (use -v to see invocation)Assertion failed: (inserted && "there should be no duplicate decls"), function getDisplayDecls, file Module.cpp, line 967.

@QuietMisdreavus
Copy link
Contributor Author

It seems like when a module re-exports two different Clang modules, there are repeat declarations of the implicit items added by the Clang importer: this specific case was failing on __NSConstantString. I imagine we'll specially need to handle these declarations.

@bitjammer
Copy link
Contributor

@swift-ci Please test source compatibility

@franklinsch
Copy link
Contributor

@swift-ci please test

@franklinsch
Copy link
Contributor

@swift-ci please test source compatibility

@QuietMisdreavus
Copy link
Contributor Author

The issue in the previous source-compatibility testing is the same one that is being fixed in #41115. I'm not cherry-picking it onto this branch (yet) since it only affects builds with assertions in it, but it will still be helpful to assist with developing on the branch generally.

@QuietMisdreavus QuietMisdreavus force-pushed the QuietMisdreavus/5.6/display-decl-recur branch from adaa063 to ca473b1 Compare February 2, 2022 23:42
@QuietMisdreavus
Copy link
Contributor Author

I've rebased the branch and reworked the commits to properly reflect the changes being cherry-picked. Since #40696 was cherry-picked separately in #41017, the initial commit has been changed to only include #40860.

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test source compatibility

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test source compatibility

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please build toolchain macOS platform

@QuietMisdreavus QuietMisdreavus requested review from airspeedswift and a team February 3, 2022 16:59
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus QuietMisdreavus force-pushed the QuietMisdreavus/5.6/display-decl-recur branch from 33e3aea to 827e94f Compare February 3, 2022 22:11
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please build toolchain macOS platform

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test source compatibility

@QuietMisdreavus QuietMisdreavus force-pushed the QuietMisdreavus/5.6/display-decl-recur branch from 827e94f to 79f8b48 Compare February 3, 2022 22:37
@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please build toolchain macOS platform

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test source compatibility

@QuietMisdreavus QuietMisdreavus changed the title [5.6][AST] check modules before recursing for display decls [5.6][AST] scan @_exported imports of source files for display decls Feb 3, 2022
@swift-ci
Copy link
Contributor

swift-ci commented Feb 4, 2022

macOS Toolchain
Download Toolchain
Git Sha - 79f8b48

Install command
tar -zxf swift-PR-40866-1342-osx.tar.gz --directory ~/

@franklinsch franklinsch merged commit 7744433 into release/5.6 Feb 4, 2022
@franklinsch franklinsch deleted the QuietMisdreavus/5.6/display-decl-recur branch February 4, 2022 10:02
@QuietMisdreavus QuietMisdreavus mentioned this pull request Feb 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DougGregor DougGregor DougGregor approved these changes

@airspeedswift airspeedswift Awaiting requested review from airspeedswift

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@QuietMisdreavus @bitjammer @franklinsch @swift-ci @DougGregor @airspeedswift