-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[ClangImporter] Check for builtin conformance in importNumericLiteral
#85409
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,6 +125,11 @@ static ValueDecl *importNumericLiteral(ClangImporter::Implementation &Impl, | |
| return nullptr; | ||
| } | ||
|
|
||
| auto &ctx = DC->getASTContext(); | ||
| auto *constantTyNominal = constantType->getAnyNominal(); | ||
| if (!constantTyNominal) | ||
| return nullptr; | ||
|
|
||
| if (auto *integer = dyn_cast<clang::IntegerLiteral>(parsed)) { | ||
| // Determine the value. | ||
| llvm::APSInt value{integer->getValue(), clangTy->isUnsignedIntegerType()}; | ||
|
|
@@ -140,6 +145,16 @@ static ValueDecl *importNumericLiteral(ClangImporter::Implementation &Impl, | |
| } | ||
| } | ||
|
|
||
| // Make sure the destination type actually conforms to the builtin literal | ||
| // protocol before attempting to import, otherwise we'll crash since | ||
| // `createConstant` expects it to. | ||
| // FIXME: We ought to be careful checking conformance here since it can | ||
| // result in cycles. Additionally we ought to consider checking for the | ||
| // non-builtin literal protocol to allow any ExpressibleByIntegerLiteral | ||
| // type to be supported. | ||
| if (!ctx.getIntBuiltinInitDecl(constantTyNominal)) | ||
| return nullptr; | ||
|
|
||
| return createMacroConstant(Impl, MI, name, DC, constantType, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to pass down the result of the lookup to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We cache the result on success, so it shouldn't matter much. There are also a bunch of different clients of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interestingly the caching logic in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm nope, Float conforms to both
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to also include the kind in the key |
||
| clang::APValue(value), | ||
| ConstantConvertKind::None, | ||
|
|
@@ -158,6 +173,16 @@ static ValueDecl *importNumericLiteral(ClangImporter::Implementation &Impl, | |
| value.changeSign(); | ||
| } | ||
|
|
||
| // Make sure the destination type actually conforms to the builtin literal | ||
| // protocol before attempting to import, otherwise we'll crash since | ||
| // `createConstant` expects it to. | ||
| // FIXME: We ought to be careful checking conformance here since it can | ||
| // result in cycles. Additionally we ought to consider checking for the | ||
| // non-builtin literal protocol to allow any ExpressibleByFloatLiteral | ||
| // type to be supported. | ||
| if (!ctx.getFloatBuiltinInitDecl(constantTyNominal)) | ||
| return nullptr; | ||
|
|
||
| return createMacroConstant(Impl, MI, name, DC, constantType, | ||
| clang::APValue(value), | ||
| ConstantConvertKind::None, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| // RUN: %empty-directory(%t) | ||
| // RUN: split-file %s %t | ||
| // RUN: %target-swift-frontend -typecheck -verify %t/main.swift -I %t -verify-additional-file %t/cmodule.h | ||
|
|
||
| // REQUIRES: objc_interop | ||
| // REQUIRES: OS=macosx | ||
|
|
||
| //--- cmodule.h | ||
| #import <CoreGraphics/CoreGraphics.h> | ||
hamishknight marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #define intLiteralCGFloat ((CGFloat)0) | ||
| // expected-note@-1 {{invalid numeric literal}} | ||
| // expected-note@-2 {{macro 'intLiteralCGFloat' unavailable (cannot import)}} | ||
| #define floatLiteralCGFloat ((CGFloat)0.0) | ||
| // expected-note@-1 {{invalid numeric literal}} | ||
| // expected-note@-2 {{macro 'floatLiteralCGFloat' unavailable (cannot import)}} | ||
|
|
||
| //--- module.modulemap | ||
| module CModule [system] { | ||
| header "cmodule.h" | ||
| export * | ||
| } | ||
|
|
||
| //--- main.swift | ||
| import CModule | ||
|
|
||
| // Make sure we don't crash when attempting to import these. | ||
| _ = intLiteralCGFloat // expected-error {{cannot find 'intLiteralCGFloat' in scope}} | ||
| _ = floatLiteralCGFloat // expected-error {{cannot find 'floatLiteralCGFloat' in scope}} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an assert at the other call sites too then, to avoid propagating nulls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not sure I understand what you mean by this