Skip to content

ClangImporter: teach clang importer to import Clang SPI symbols and model them similarly as Swift SPIs #39068

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 1 commit into from
Aug 28, 2021
Merged

ClangImporter: teach clang importer to import Clang SPI symbols and model them similarly as Swift SPIs #39068

merged 1 commit into from
Aug 28, 2021

Conversation

nkcsgexi
Copy link
Contributor

@nkcsgexi nkcsgexi commented Aug 27, 2021 •
edited
Loading

For clang symbols marked with SPI_AVAILABLE, we add SPIAccessControlAttr to them so they will be considered as SPIs in the AST. To be able to use all these symbols, we also add an implicit SPI import statement for all clang modules. All clang SPIs belong to the same SPI group named <objc> because clang currently doesn't support custom SPI group.

rdar://73902734

@nkcsgexi nkcsgexi requested review from xymus and beccadax August 27, 2021 00:02
@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@xymus
Copy link
Contributor

xymus commented Aug 27, 2021 •
edited
Loading

The Objective-C logic for these macros is far from ideal. Instead of bringing it over to Swift could we type-check this as part of the TypeCheckAccess logic and plug this into getDisallowedOriginKind?

@nkcsgexi
Copy link
Contributor Author

Yeah, I think we could set a bit here and actually diagnose in getDisallowedOriginKind.

@nkcsgexi nkcsgexi force-pushed the 73902734 branch 2 times, most recently from 9a66626 to a1158b1 Compare August 27, 2021 14:58
@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@xymus The approach I implemented was slightly different: (1) an SPIAccessControlAttr with the group name objc was added to each SPI_AVAILABLE symbol, and (2) Clang modules were imported by including SPIs with the group name objc. Thus the clang symbols are modeled in the same way as Swift ones so we don't need to modify type checker logics to check clang symbols differently. Any objections?

@nkcsgexi
Copy link
Contributor Author

@swift-ci Please smoke test OS X platform

@nkcsgexi
Copy link
Contributor Author

@swift-ci Please smoke test OS X platform

nkcsgexi
nkcsgexi commented Aug 27, 2021
View reviewed changes
xymus
xymus reviewed Aug 27, 2021
View reviewed changes
xymus
xymus reviewed Aug 27, 2021
View reviewed changes
xymus
xymus approved these changes Aug 27, 2021
View reviewed changes
Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

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

This looks good!

Should we wait for library-level inference in the build system to land before we integrate this? I'm worried that as such it will raise false positive on SPI use in private frameworks.

adrian-prantl
adrian-prantl requested changes Aug 27, 2021
View reviewed changes
Copy link
Contributor

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

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

As discussed offline, the change in debug info behavior is a regression.

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@adrian-prantl adrian-prantl self-requested a review August 28, 2021 00:55
adrian-prantl
adrian-prantl approved these changes Aug 28, 2021
View reviewed changes
Copy link
Contributor

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

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

Looks like you were able fix the missing ImportDecls, thanks!

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi nkcsgexi changed the title ClangImporter: teach clang importer to import Clang symbols with SPI_AVAILABLE as unavailable symbols ClangImporter: teach clang importer to import Clang SPI symbols and model them similarly as Swift SPIs Aug 28, 2021
@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
ClangImporter: teach clang importer to import Clang SPI symbols and m…
36f25c3 
…odel them similarly as Swift SPIs

For clang symbols marked with SPI_AVAILABLE, we add SPIAccessControlAttr to them so they will be
considered as SPIs in the AST. To be able to use all these symbols, we also add an implicit SPI import
statement for all clang modules. All clang SPIs belong to the same SPI group named "OBJC_DEFUALT_SPI_GROUP" because clang
currently doesn't support custom SPI group.

rdar://73902734
@nkcsgexi
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor Author

@xymus @adrian-prantl Thanks for taking a look! All tests passed. Merging.

@nkcsgexi nkcsgexi merged commit 3616872 into swiftlang:main Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xymus xymus xymus approved these changes

@adrian-prantl adrian-prantl adrian-prantl approved these changes

@beccadax beccadax Awaiting requested review from beccadax beccadax is a code owner

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

Successfully merging this pull request may close these issues.
3 participants
@nkcsgexi @xymus @adrian-prantl