[5.9][interop] do not import functions whose return type is not imported #65105
Conversation
(cherry picked from commit 8e0c17b)
|
@swift-ci please test |
| @@ -3391,6 +3394,14 @@ namespace { | |||
| func->setAccess(AccessLevel::Public); | |||
| } | |||
|
|
|||
| if (!isa<clang::CXXConstructorDecl>(decl) && !importedType) { | |||
There was a problem hiding this comment.
It seems like !importedType should be a sufficient condition here. Why try importing it twice?
| @@ -3391,6 +3394,14 @@ namespace { | |||
| func->setAccess(AccessLevel::Public); | |||
| } | |||
|
|
|||
| if (!isa<clang::CXXConstructorDecl>(decl) && !importedType) { | |||
| if (!Impl.importFunctionReturnType(decl, result->getDeclContext())) { | |||
There was a problem hiding this comment.
We intentionally don't eagerly import some class template specializations. These really need to be lazily evaluated to respect C++ semantics. You are allowed to have a completely broken class template specialization, as long as the method is never called.
There was a problem hiding this comment.
We are currently handling this logic on line 3304/3307: if (!bodyParams). We use body params, because we sometimes want to import the return type lazily. For better or worse, this is an invariant that we've had in the importer, we should change it with caution.
There was a problem hiding this comment.
To elaborate on the first comment, this is the case I'm talking about, where we may violate C++ "semantics" if we try to do this too eagerly:
template<class T> struct Borked { static_assert(__is_same(T, char), "I'm broken!"); };
struct HasMethod { Borked<int> test(); };
void test(HasMethod hm) {
// As long as this is never called, we're fine.
// hm.test();
}
In other words, we cannot import test's return type eagerly here.
| Impl.addImportDiagnostic( | ||
| decl, Diagnostic(diag::unsupported_return_type, decl), | ||
| decl->getSourceRange().getBegin()); | ||
| return nullptr; |
There was a problem hiding this comment.
Just for the sake of completeness, I'd like to mention that Objective-C and C will also hit this codepath, so there's a (hopefully very small?) chance of behavior change there.
There was a problem hiding this comment.
I guess the important (and oh so terrible) condition here is
(!(isa<clang::RecordType>(returnType) ||
isa<clang::TemplateSpecializationType>(returnType)) ||
// TODO: we currently don't lazily load operator return types, but
// this should be trivial to add.
clangDecl->isOverloadedOperator() ||
// Dependant types are trivially mapped as Any.
returnType->isDependentType())
If that condition is true, then we'll already apply the behavior of this patch by returning a null bodyParams. If that condition is false, that's where this patch kicks in. So thinking about C, I think we'll see a behavior change for any function that returns a record that we can't import. Now let me see if I can create one of those...
There was a problem hiding this comment.
And here's an example:
struct alignas(128) OverAligned {
char buff[128];
};
OverAligned test();
AnthonyLatsis
added
🍒 release cherry pick
|
This was reverted from main, dropping. |
(cherry picked from commit 8e0c17b)
Explanation: Clang importer can sometimes import C++ functions with the
Neverreturn type because the original type can not be imported. That is wrong, as making such calls isn't going to cleanup the return value. Therefore, such functions should not be imported into Swift. The compiler diagnoses them as unavailable with a helpful message about the return type.Scope: Swift's and C++ interoperability, Clang importer.
Issue: Resolves #64401
Risk: Medium.
Testing: Swift unit tests.
Reviewer: @egorzhdan