Skip to content

[cxx-interop] Re-implement namespaces using enums + extensions. #35085

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

Merged
merged 1 commit into from
Feb 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
110 changes: 104 additions & 6 deletions lib/ClangImporter/ImportDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2445,19 +2445,116 @@ namespace {
auto importedName = importFullName(decl, correctSwiftName);
if (!importedName) return nullptr;

auto dc =
auto extensionDC =
Impl.importDeclContextOf(decl, importedName.getEffectiveContext());
if (!dc)
if (!extensionDC)
return nullptr;

SourceLoc loc = Impl.importSourceLoc(decl->getBeginLoc());

// FIXME: If Swift gets namespaces, import as a namespace.
auto enumDecl = Impl.createDeclWithClangNode<EnumDecl>(
DeclContext *dc = nullptr;
// If this is a top-level namespace, don't put it in the module we're
// importing, put it in the "__ObjC" module that is implicitly imported.
// This way, if we have multiple modules that all open the same namespace,
// we won't import multiple enums with the same name in swift.
if (extensionDC->getContextKind() == DeclContextKind::FileUnit)
dc = Impl.ImportedHeaderUnit;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally, I created a separate __Cxx module for this, but I think ImportedHeaderUnit (__ObjC) works fine, so there's really no reason to add another one. Once we decide how we want to handle apps that use both Objective-C and C++ interop, we might conditionally name this __Cxx instead of __ObjC.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the only question is, whether it is possible to have a conflict between any existing entity in __ObjC and these new namespace-enums. I think a conflict is only possible if someone had an actual C enum in the global namespace, and a top-level namespace of the same name; however that is not allowed in C++.

So using __ObjC makes sense. I think even when we enable C++ interop by default, the module can continue to be called __ObjC, because why break ABI.

else {
// This is a nested namespace, we need to find its extension decl
// context and then use that to find the parent enum. It's important
// that we add this to the parent enum (in the "__ObjC" module) and not
// to the extension.
auto parentNS = cast<clang::NamespaceDecl>(decl->getParent());
auto parent = Impl.importDecl(parentNS, getVersion());
// Sometimes when the parent namespace is imported, this namespace
// also gets imported. If that's the case, then the parent namespace
// will be an enum (because it was able to be fully imported) in which
// case we need to bail here.
auto cachedResult =
Impl.ImportedDecls.find({decl->getCanonicalDecl(), getVersion()});
if (cachedResult != Impl.ImportedDecls.end())
return cachedResult->second;
dc = cast<ExtensionDecl>(parent)
->getExtendedType()
->getEnumOrBoundGenericEnum();
}
auto *enumDecl = Impl.createDeclWithClangNode<EnumDecl>(
decl, AccessLevel::Public, loc,
importedName.getDeclName().getBaseIdentifier(),
Impl.importSourceLoc(decl->getLocation()), None, nullptr, dc);
enumDecl->setMemberLoader(&Impl, 0);
if (isa<clang::NamespaceDecl>(decl->getParent()))
cast<EnumDecl>(dc)->addMember(enumDecl);

// We are creating an extension, so put it at the top level. This is done
// after creating the enum, though, because we may need the correctly
// nested decl context above when creating the enum.
while (extensionDC->getParent() &&
extensionDC->getContextKind() != DeclContextKind::FileUnit)
extensionDC = extensionDC->getParent();

auto *extension = ExtensionDecl::create(Impl.SwiftContext, loc, nullptr,
{}, extensionDC, nullptr, decl);
Impl.SwiftContext.evaluator.cacheOutput(ExtendedTypeRequest{extension},
enumDecl->getDeclaredType());
Impl.SwiftContext.evaluator.cacheOutput(ExtendedNominalRequest{extension},
std::move(enumDecl));
// Keep track of what members we've already added so we don't add the same
// member twice. Note: we can't use "ImportedDecls" for this because we
// might import a decl that we don't add (for example, if it was a
// parameter to another decl).
SmallPtrSet<Decl *, 16> addedMembers;
for (auto redecl : decl->redecls()) {
// This will be reset as the EnumDecl after we return from
// VisitNamespaceDecl.
Impl.ImportedDecls[{redecl->getCanonicalDecl(), getVersion()}] =
extension;

// Insert these backwards into "namespaceDecls" so we can pop them off
// the end without loosing order.
SmallVector<clang::Decl *, 16> namespaceDecls;
auto addDeclsReversed = [&](auto decls) {
auto begin = decls.begin();
auto end = decls.end();
int currentSize = namespaceDecls.size();
int declCount = currentSize + std::distance(begin, end);
namespaceDecls.resize(declCount);
for (int index = declCount - 1; index >= currentSize; --index)
namespaceDecls[index] = *(begin++);
};
addDeclsReversed(redecl->decls());
while (!namespaceDecls.empty()) {
auto nd = dyn_cast<clang::NamedDecl>(namespaceDecls.pop_back_val());
// Make sure we only import the defenition of a record.
if (auto tagDecl = dyn_cast_or_null<clang::TagDecl>(nd))
// Some decls, for example ClassTemplateSpecializationDecls, won't
// have a definition here. That's OK.
nd = tagDecl->getDefinition() ? tagDecl->getDefinition() : tagDecl;
if (!nd)
continue;

// Special case class templates: import all their specilizations here.
if (auto classTemplate = dyn_cast<clang::ClassTemplateDecl>(nd)) {
addDeclsReversed(classTemplate->specializations());
continue;
}

auto member = Impl.importDecl(nd, getVersion());
if (!member || addedMembers.count(member) ||
isa<clang::NamespaceDecl>(nd))
continue;
// This happens (for example) when a struct is declared inside another
// struct inside a namespace but defined out of line.
assert(member->getDeclContext()->getAsDecl());
if (dyn_cast<ExtensionDecl>(member->getDeclContext()->getAsDecl()) !=
extension)
continue;
extension->addMember(member);
addedMembers.insert(member);
}
}

if (!extension->getMembers().empty())
enumDecl->addExtension(extension);

return enumDecl;
}

Expand Down Expand Up @@ -8583,6 +8680,7 @@ DeclContext *ClangImporter::Implementation::importDeclContextImpl(
auto decl = dyn_cast<clang::NamedDecl>(dc);
if (!decl)
return nullptr;

// Category decls with same name can be merged and using canonical decl always
// leads to the first category of the given name. We'd like to keep these
// categories separated.
Expand Down
8 changes: 4 additions & 4 deletions test/ClangImporter/cxx_interop_ir.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@ func indirectUsage() {
useT(makeT())
}

// CHECK-LABEL: define hidden swiftcc %swift.type* @"$s6cxx_ir14reflectionInfo3argypXpSo2nsV1TV_tF"
// CHECK: %0 = call swiftcc %swift.metadata_response @"$sSo2nsV1TVMa"({{i64|i32}} 0)
// CHECK-LABEL: define hidden swiftcc %swift.type* @"$s6cxx_ir14reflectionInfo3argypXpSo2nsV10CXXInteropE1TV_tF"
// CHECK: %0 = call swiftcc %swift.metadata_response @"$sSo2nsV10CXXInteropE1TVMa"({{i64|i32}} 0)
func reflectionInfo(arg: namespacedT) -> Any.Type {
return type(of: arg)
}

// CHECK: define hidden swiftcc void @"$s6cxx_ir24namespaceManglesIntoName3argySo2nsV1TV_tF"
// CHECK: define hidden swiftcc void @"$s6cxx_ir24namespaceManglesIntoName3argySo2nsV10CXXInteropE1TV_tF"
func namespaceManglesIntoName(arg: namespacedT) {
}

// CHECK: define hidden swiftcc void @"$s6cxx_ir42namespaceManglesIntoNameForUsingShadowDecl3argySo2nsV14NamespacedTypeV_tF"
// CHECK: define hidden swiftcc void @"$s6cxx_ir42namespaceManglesIntoNameForUsingShadowDecl3argySo2nsV10CXXInteropE14NamespacedTypeV_tF"
func namespaceManglesIntoNameForUsingShadowDecl(arg: NamespacedType) {
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// RUN: %target-swift-ide-test -print-module -module-to-print=LinkedRecords -I %S/Inputs/ -source-filename=x -enable-cxx-interop | %FileCheck %s

// CHECK: enum Space {
// CHECK: extension Space {
// CHECK: struct C {
// CHECK: struct D {
// CHECK: var B: Space.A.B
Expand Down
12 changes: 12 additions & 0 deletions test/Interop/Cxx/namespace/Inputs/classes-second-header.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#ifndef TEST_INTEROP_CXX_NAMESPACE_INPUTS_CLASSES_SECOND_HEADER_H
#define TEST_INTEROP_CXX_NAMESPACE_INPUTS_CLASSES_SECOND_HEADER_H

#include "classes.h"

struct ClassesNS1::ClassesNS2::DefinedInDefs {
const char *basicMember() {
return "ClassesNS1::ClassesNS2::DefinedInDefs::basicMember";
}
};

#endif // TEST_INTEROP_CXX_NAMESPACE_INPUTS_CLASSES_SECOND_HEADER_H
43 changes: 43 additions & 0 deletions test/Interop/Cxx/namespace/Inputs/classes.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#ifndef TEST_INTEROP_CXX_NAMESPACE_INPUTS_CLASSES_H
#define TEST_INTEROP_CXX_NAMESPACE_INPUTS_CLASSES_H

namespace ClassesNS1 {
struct BasicStruct {
const char *basicMember() { return "ClassesNS1::BasicStruct::basicMember"; }
};
struct ForwardDeclaredStruct;
} // namespace ClassesNS1

struct ClassesNS1::ForwardDeclaredStruct {
const char *basicMember() {
return "ClassesNS1::ForwardDeclaredStruct::basicMember";
}
};

namespace ClassesNS1 {
namespace ClassesNS2 {
struct BasicStruct {
const char *basicMember() {
return "ClassesNS1::ClassesNS2::BasicStruct::basicMember";
}
};
struct ForwardDeclaredStruct;
struct DefinedInDefs;
} // namespace ClassesNS2
} // namespace ClassesNS1

namespace ClassesNS1 {
struct ClassesNS2::ForwardDeclaredStruct {
const char *basicMember() {
return "ClassesNS1::ClassesNS2::ForwardDeclaredStruct::basicMember";
}
};
} // namespace ClassesNS1

namespace ClassesNS3 {
struct BasicStruct {
const char *basicMember() { return "ClassesNS3::BasicStruct::basicMember"; }
};
} // namespace ClassesNS3

#endif // TEST_INTEROP_CXX_NAMESPACE_INPUTS_CLASSES_H
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#ifndef TEST_INTEROP_CXX_NAMESPACE_INPUTS_FREE_FUNCTIONS_SECOND_HEADER_H
#define TEST_INTEROP_CXX_NAMESPACE_INPUTS_FREE_FUNCTIONS_SECOND_HEADER_H

#include "free-functions.h"

inline const char *FunctionsNS1::definedInDefs() {
return "FunctionsNS1::definedInDefs";
}

#endif // TEST_INTEROP_CXX_NAMESPACE_INPUTS_FREE_FUNCTIONS_SECOND_HEADER_H
60 changes: 60 additions & 0 deletions test/Interop/Cxx/namespace/Inputs/free-functions.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#ifndef TEST_INTEROP_CXX_NAMESPACE_INPUTS_FREE_FUNCTION_H
#define TEST_INTEROP_CXX_NAMESPACE_INPUTS_FREE_FUNCTION_H

namespace FunctionsNS1 {
inline const char *basicFunctionTopLevel() {
return "FunctionsNS1::basicFunctionTopLevel";
}
inline const char *forwardDeclared();
inline const char *definedOutOfLine();
} // namespace FunctionsNS1

namespace FunctionsNS1 {
inline const char *forwardDeclared() { return "FunctionsNS1::forwardDeclared"; }
} // namespace FunctionsNS1

inline const char *FunctionsNS1::definedOutOfLine() {
return "FunctionsNS1::definedOutOfLine";
}

namespace FunctionsNS1 {
namespace FunctionsNS2 {
inline const char *basicFunctionSecondLevel() {
return "FunctionsNS1::FunctionsNS2::basicFunctionSecondLevel";
}
} // namespace FunctionsNS2
} // namespace FunctionsNS1

namespace FunctionsNS1 {
namespace FunctionsNS2 {
namespace FunctionsNS3 {
inline const char *basicFunctionLowestLevel() {
return "FunctionsNS1::FunctionsNS2::FunctionsNS3::basicFunctionLowestLevel";
}
} // namespace FunctionsNS3
} // namespace FunctionsNS2
} // namespace FunctionsNS1

namespace FunctionsNS1 {
inline const char *definedInDefs();
}

namespace FunctionsNS1 {
inline const char *sameNameInChild() { return "FunctionsNS1::sameNameInChild"; }
inline const char *sameNameInSibling() {
return "FunctionsNS1::sameNameInSibling";
}
namespace FunctionsNS2 {
inline const char *sameNameInChild() {
return "FunctionsNS1::FunctionsNS2::sameNameInChild";
}
} // namespace FunctionsNS2
} // namespace FunctionsNS1

namespace FunctionsNS4 {
inline const char *sameNameInSibling() {
return "FunctionsNS4::sameNameInSibling";
}
} // namespace FunctionsNS4

#endif // TEST_INTEROP_CXX_NAMESPACE_INPUTS_FREE_FUNCTION_H
28 changes: 28 additions & 0 deletions test/Interop/Cxx/namespace/Inputs/module.modulemap
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
module Classes {
header "classes.h"
}

module ClassesSecondHeader {
// TODO: we shouldn't have to include both of these, and the decls defined in
// these headers should be added to the correct module: SR-14214.
header "classes.h"
header "classes-second-header.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make classes-second-header.h include classes.h? As written in the module map, classes.h belongs to two modules. Similarly in other header pairs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does include "classes.h". But both headers are required to be able to use the imported APIs. I'll investigate because we should be able to see the stuff that's "defined in defs" without "importing" both headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry indeed it does. I was thinking of templates-second-header.h.

But yes, it would be best if classes.h (and other first-level headers) could be a part of only one module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, here's the problem: when we find a declaration, we immediately look for the definition and see if we can import that. This means Swift thinks all of these APIs are part of Classes and not ClassesSecondHeader. While this is sort of problematic, I think it's at least mostly an unrelated issue. I'd like to keep the tests here with a TODO comment linking to an SR so that the problem can be fixed in its own PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thank you!

}

module FreeFunctions {
header "free-functions.h"
}

module FreeFunctionsSecondHeader {
header "free-functions.h"
header "free-functions-second-header.h"
}

module Templates {
header "templates.h"
}

module TemplatesSecondHeader {
header "templates.h"
header "templates-second-header.h"
}
18 changes: 18 additions & 0 deletions test/Interop/Cxx/namespace/Inputs/templates-second-header.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#ifndef TEST_INTEROP_CXX_NAMESPACE_INPUTS_TEMPLATES_SECOND_HEADER_H
#define TEST_INTEROP_CXX_NAMESPACE_INPUTS_TEMPLATES_SECOND_HEADER_H

template <class T>
const char *TemplatesNS1::basicFunctionTemplateDefinedInDefs(T) {
return "TemplatesNS1::basicFunctionTemplateDefinedInDefs";
}

template <class> struct TemplatesNS1::BasicClassTemplateDefinedInDefs {
const char *basicMember() {
return "TemplatesNS1::BasicClassTemplateDefinedInDefs::basicMember";
}
};

using BasicClassTemplateDefinedInDefsChar =
TemplatesNS1::BasicClassTemplateDefinedInDefs<char>;

#endif // TEST_INTEROP_CXX_NAMESPACE_INPUTS_TEMPLATES_SECOND_HEADER_H
Loading