Skip to content

[5.9][interop] do not import functions whose return type is not imported #65105

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

Closed
wants to merge 1 commit into from
Closed

[5.9][interop] do not import functions whose return type is not imported #65105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/swift/AST/DiagnosticsClangImporter.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ NOTE(record_field_not_imported, none, "field %0 unavailable (cannot import)", (c
NOTE(invoked_func_not_imported, none, "function %0 unavailable (cannot import)", (const clang::NamedDecl*))
NOTE(record_method_not_imported, none, "method %0 unavailable (cannot import)", (const clang::NamedDecl*))
NOTE(objc_property_not_imported, none, "property %0 unavailable (cannot import)", (const clang::NamedDecl*))
NOTE(unsupported_return_type, none, "C++ function %0 is unavailable: return type is unavailable in Swift", (const clang::NamedDecl*))

NOTE(placeholder_for_forward_declared_interface_member_access_failure, none,
"class '%0' will be imported as an opaque placeholder class and may be "
Expand Down
8 changes: 1 addition & 7 deletions lib/ClangImporter/ClangImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5622,13 +5622,7 @@ importName(const clang::NamedDecl *D,

Type ClangImporter::importFunctionReturnType(
const clang::FunctionDecl *clangDecl, DeclContext *dc) {
bool isInSystemModule =
cast<ClangModuleUnit>(dc->getModuleScopeContext())->isSystemModule();
bool allowNSUIntegerAsInt =
Impl.shouldAllowNSUIntegerAsInt(isInSystemModule, clangDecl);
if (auto imported =
Impl.importFunctionReturnType(dc, clangDecl, allowNSUIntegerAsInt)
.getType())
if (auto imported = Impl.importFunctionReturnType(clangDecl, dc).getType())
return imported;
return dc->getASTContext().getNeverType();
}
Expand Down
25 changes: 18 additions & 7 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3048,13 +3048,16 @@ namespace {

if (auto recordType = dyn_cast<clang::RecordType>(
decl->getReturnType().getCanonicalType())) {
Impl.addImportDiagnostic(
decl, Diagnostic(diag::reference_passed_by_value,
Impl.SwiftContext.AllocateCopy(
recordType->getDecl()->getNameAsString()),
"the return"),
decl->getLocation());
return recordHasReferenceSemantics(recordType->getDecl());
if (recordHasReferenceSemantics(recordType->getDecl())) {
Impl.addImportDiagnostic(
decl,
Diagnostic(diag::reference_passed_by_value,
Impl.SwiftContext.AllocateCopy(
recordType->getDecl()->getNameAsString()),
"the return"),
decl->getLocation());
return true;
}
}

return false;
Expand Down Expand Up @@ -3391,6 +3394,14 @@ namespace {
func->setAccess(AccessLevel::Public);
}

if (!isa<clang::CXXConstructorDecl>(decl) && !importedType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like !importedType should be a sufficient condition here. Why try importing it twice?

if (!Impl.importFunctionReturnType(decl, result->getDeclContext())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

And here's an example:

struct alignas(128) OverAligned {
  char buff[128];
};

OverAligned test();

}
}
result->setIsObjC(false);
result->setIsDynamic(false);

Expand Down
9 changes: 9 additions & 0 deletions lib/ClangImporter/ImportType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2074,6 +2074,15 @@ applyImportTypeAttrs(ImportTypeAttrs attrs, Type type,
return type;
}

ImportedType ClangImporter::Implementation::importFunctionReturnType(
const clang::FunctionDecl *clangDecl, DeclContext *dc) {
bool isInSystemModule =
cast<ClangModuleUnit>(dc->getModuleScopeContext())->isSystemModule();
bool allowNSUIntegerAsInt =
shouldAllowNSUIntegerAsInt(isInSystemModule, clangDecl);
return importFunctionReturnType(dc, clangDecl, allowNSUIntegerAsInt);
}

ImportedType ClangImporter::Implementation::importFunctionReturnType(
DeclContext *dc, const clang::FunctionDecl *clangDecl,
bool allowNSUIntegerAsInt) {
Expand Down
3 changes: 3 additions & 0 deletions lib/ClangImporter/ImporterImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1340,6 +1340,9 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
const clang::FunctionDecl *clangDecl,
bool allowNSUIntegerAsInt);

ImportedType importFunctionReturnType(const clang::FunctionDecl *clangDecl,
DeclContext *dc);

/// Import the parameter list for a function
///
/// \param clangDecl The underlying declaration, if any; should only be
Expand Down
65 changes: 65 additions & 0 deletions test/Interop/Cxx/class/returns-unavailable-class.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// RUN: rm -rf %t
// RUN: split-file %s %t
// RUN: %target-swift-ide-test -print-module -module-to-print=CxxModule -I %t/Inputs -source-filename=x -enable-experimental-cxx-interop | %FileCheck %s

// RUN: %target-swift-frontend -typecheck -verify -I %t/Inputs -enable-experimental-cxx-interop %t/test.swift

// RUN: not %target-swift-frontend -typecheck -I %t/Inputs -enable-experimental-cxx-interop %t/test.swift 2>&1 | %FileCheck --check-prefix=NOTE %s


//--- Inputs/module.modulemap
module CxxTypes {
header "types.h"
requires cplusplus
}

module CxxModule {
header "header.h"
requires cplusplus
}

//--- Inputs/types.h

template<class T>
class TemplateInTypesModule {
public:
T x, y;
};

//--- Inputs/header.h

#pragma clang module import CxxTypes

class Struct {
public:
int x, y;

TemplateInTypesModule<int> returnsClassInTypesModules() const;

void takesClassInTypesModules(TemplateInTypesModule<int>) const;
void takesClassInTypesModulesRef(const TemplateInTypesModule<int> &) const;
};

// CHECK: struct Struct {
// CHECK-NEXT: init()
// CHECK-NEXT: init(x: Int32, y: Int32)
// CHECK-NEXT: var x: Int32
// CHECK-NEXT: var y: Int32
// CHECK-NEXT: }

// CHECK-NOT: funcWithClass

TemplateInTypesModule<int> funcWithClassInTypesModules();
void funcWithClassInTypesModulesParam(TemplateInTypesModule<int>);
void funcWithClassInTypesModulesParamRef(const TemplateInTypesModule<int> &);

//--- test.swift

import CxxModule

func test() {
funcWithClassInTypesModules() // expected-error {{cannot find 'funcWithClassInTypesModules' in scope}}
Struct().returnsClassInTypesModules() // expected-error {{value of type 'Struct' has no member 'returnsClassInTypesModules'}}
}

// NOTE: note: C++ function 'funcWithClassInTypesModules' is unavailable: return type is unavailable in Swift