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

zoecarver · 2020-12-14T20:02:56Z

C++ namespaces are module-independent, but enums are owned by their module's in Swift. So, to prevent declaring two enums with the same name, this patch implements a new approach to namespaces: enums with extensions.

Here's an example:

// Module A
namespace N { void test1(); }
// Module B
namespace N { void test2(); }
// __ObjC module
enum N { }
// Imported module A
extension N { func test1() }
// Imported module B
extension N { func test1() }

Thanks to @gribozavr for the great idea.

zoecarver · 2020-12-14T20:06:50Z

lib/ClangImporter/ImportDecl.cpp

+      // 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;


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.

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.

test/Interop/Cxx/namespace/Inputs/namespaces.h

gribozavr · 2020-12-21T23:26:22Z

test/Interop/Cxx/namespace/Inputs/module.modulemap

+  header "namespaces.h"
+}
+
+module NamespaceDefs {


What does 'defs' mean and why does the first module not have it in the name? I'd suggest to name the two modules Namespaces1 and Namespaces2, they seem to be siblings in the sense that one is not more important than the other.

Yeah, I fixed this. I meant to include the one header in the other so that we could test with two headers (because then the decls sometimes become unlinked, i.e., redecls isn't all the decls). Do the names make sense now?

Oh I see. Could you add a comment to namespace-defs.h that explains that the only purpose of that header is to define APIs that are forward declared in namespaces.h?

test/Interop/Cxx/namespace/Inputs/namespaces.h

zoecarver · 2020-12-22T20:15:28Z

@swift-ci please test Windwos.

zoecarver · 2020-12-22T20:15:44Z

@swift-ci please test Windows.

zoecarver · 2020-12-22T20:16:01Z

@swift-ci please test Windows platform.

zoecarver · 2020-12-22T20:16:29Z

@swift-ci test Windows

zoecarver · 2020-12-22T21:43:42Z

@swift-ci test Windows

test/Interop/Cxx/namespace/Inputs/namespaces.h

test/Interop/Cxx/namespace/namespaces-irgen.swift

lib/ClangImporter/ImportDecl.cpp

gribozavr · 2021-01-14T13:28:46Z

lib/ClangImporter/ImportDecl.cpp

+        // This will be re-set as the enum after we return from
+        // "VisitNamespaceDecl."


Suggested change

// This will be re-set as the enum after we return from

// "VisitNamespaceDecl."

// This will be reset to the EnumDecl after we return from

// VisitNamespaceDecl.

But by whom?

A call to importDecl will "cache" the imported decl (the result from the visitor). We set Impl.ImportedDecls[redecl] here so that subdecls will have the correct decl context, but once this function returns an enum, importDecl's implementation will "cache" or reset Impl.ImportedDecls[decl] with the imported enum.

Thanks! Please add this information to the comment.

"This will be reset" -> "importDecl will reset the imported decl to the EnumDecl..."

lib/ClangImporter/ImportDecl.cpp

test/Interop/Cxx/namespace/namespaces-module-interface.swift

zoecarver · 2021-01-17T19:50:52Z

@gribozavr Thanks for the review! I think I addressed all you're comments. Let me know what you think.

zoecarver · 2021-01-17T19:51:01Z

@swift-ci please smoke test.

gribozavr · 2021-01-18T09:55:50Z

test/Interop/Cxx/namespace/Inputs/namespaces.h

+} // namespace ParentWithChildWithFunctions
+
+namespace ParentWithChildWithFunctions {
+inline void ChildWithFunctions::forwardDeclared(){};


Suggested change

inline void ChildWithFunctions::forwardDeclared(){};

inline void ChildWithFunctions::forwardDeclared() {}

gribozavr · 2021-01-18T09:57:28Z

test/Interop/Cxx/namespace/Inputs/namespaces.h

+
+namespace SecondWithSameName {
+inline int hasSameName() { return 2; }
+} // namespace SecondWithSameName


Could you group this test together with UniqueOuterName1? It is essentially a very similar idea, but with less nesting.

gribozavr · 2021-01-18T10:09:11Z

test/Interop/Cxx/namespace/Inputs/namespaces.h

+inline int hasSameName() { return 2; }
+} // namespace SecondWithSameName
+
+#endif // TEST_INTEROP_CXX_NAMESPACE_INPUTS_NAMESPACE_H


I'm sorry, this is going to be a big request, but I hope you agree that it is going to be worth it.

I'm getting a feeling that this test header is hard to understand because it tries to test many combinations of many complex features, and there is no visible structure in this header that makes it obvious that we are testing all necessary combinations. Also, naming is a bit ad-hoc, which makes it also harder to understand what is covered and what isn't.

I'd suggest to maybe restructure it into sections (or separate headers!) based on the features we're testing.

For example, the first section (or the first header) would test free functions in namespaces. Define a namespace structure:

namespace FunctionsNS1 {/* here */} namespace FunctionsNS2 {/* here */} namespace FunctionsNS1 { namespace FunctionsNS2 {/* here */} } namespace FunctionsNS2 { namespace FunctionsNS1 {/* here */} }

Now, inside every point marked "here" insert the same repertoire of functions: defined in .cc file, defined inline, forward declared and defined as out of line later, marked static, etc. Use repeated function names so that we can test that the importer does not confuse any two of them. Have each function return a unique const char * with its fully qualified name so that execution tests can easily validate that they are calling the right function. Call every function in the irgen test and check its mangled name. Call each function in the execution test and check the string that it returns. Check the complete imported API in the module interface test.

Repeat similarly for function templates, classes, class templates, typedefs.

@gribozavr you were correct on both accounts: this was a big overhaul of the tests and it was well worth it.

I ran into several problems while adding these tests, luckily none were related to namespaces themselves, per se. Anyway, I'm going to try to remove the reliance on #34993 to make the diff easier to read.

Let me know what you think. I really hope the tests are easier to read now :)

(You probably know this already but just in case...) To make the diff easier to read, you can click on the commit where all this is implemented, then the other two commits (which are both separate PRs) won't distract from what's actually being implemented here.

test/Interop/Cxx/namespace/free-functions.swift

zoecarver · 2021-01-22T04:37:21Z

OK, that should hopefully clean up the diff a little. (Removed dependence on #34993.)

@swift-ci please smoke test.

zoecarver · 2021-01-25T22:06:10Z

+1, but I'd humbly suggest to add these tests in a separate PR.

Sounds good. I'll make this a separate change.

zoecarver · 2021-01-25T22:50:08Z

@swift-ci please smoke test.

zoecarver · 2021-01-26T03:53:40Z

@swift-ci please smoke test Windows.

zoecarver · 2021-01-26T06:12:13Z

@swift-ci please smoke test Windows.

zoecarver · 2021-01-27T04:20:07Z

@swift-ci please smoke test Windows.

zoecarver · 2021-01-27T06:06:35Z

@swift-ci please smoke test Windows.

zoecarver · 2021-02-03T22:30:19Z

test/Interop/Cxx/templates/swift-class-instantiation-in-namespace-module-interface.swift

@@ -2,6 +2,9 @@
 // RUN: %target-swiftxx-frontend -emit-module -o %t/SwiftClassTemplateInNamespaceModule.swiftmodule %S/Inputs/SwiftClassTemplateInNamespaceModule.swift -I %S/Inputs -enable-library-evolution -swift-version 5
 // RUN: %target-swift-ide-test -print-module -module-to-print=SwiftClassTemplateInNamespaceModule -I %t/ -source-filename=x -enable-cxx-interop | %FileCheck %s

+// We need to be able to properly serialize the __ObjC module holding the "Space" enum.
+// REQUIRES: SR-XXXXX


This test is failing because we can't find the enum decl because it's in the __ObjC module which isn't getting serialized. I think the best solution is to serialize that module as well and save it along side whatever the Swift module is that we're saving (SwiftClassTemplateInNamespaceModule in this case). But that's going to be a big/involved change, so I'd rather get this in first, and figure that out later.

If we agree that this is the best path forward, I'll create an SR and update the comment.

The contents of the __ObjC module should be generated by the importer, so I'm not yet convinced that serializing it is the right approach. I'd suggest to update the comment to say that there is a bug, and put the details into jira.

OK good to know. I'll have to spend some more time thinking about this, then :) Here's the bug I filed: SR-14211.

zoecarver · 2021-02-03T22:30:41Z

@swift-ci please smoke test.

zoecarver · 2021-02-04T00:07:45Z

@swift-ci please test Windows platform.

zoecarver · 2021-02-04T00:07:53Z

@swift-ci please test Windows platform.

zoecarver · 2021-02-04T01:55:00Z

@swift-ci please test Windows platform.

zoecarver · 2021-02-04T01:55:11Z

@swift-ci please test Windows platform.

zoecarver · 2021-02-04T01:55:36Z

@swift-ci please test Windows platform.

zoecarver · 2021-02-13T04:09:50Z

@gribozavr friendly ping.

zoecarver · 2021-02-13T04:10:41Z

The tests are passing on macOS and the windows tests are just a matter of updating the regex (I'll have to fire up the VM for that, though, so I'm waiting for this to get approved first).

zoecarver · 2021-02-14T04:02:32Z

@gribozavr, thank you so much for all you're help/guidance with this patch!

@gribozavr

C++ namespaces are module-independent, but enums are owned by their module's in Swift. So, to prevent declaring two enums with the same name, this patch implements a new approach to namespaces: enums with extensions. Here's an example: ``` // Module A namespace N { void test1(); } // Module B namespace N { void test2(); } // __ObjC module enum N { } // Swift module A extension N { func test1() } // Swift module B extension N { func test1() } ``` Thanks to @gribozavr for the great idea.

zoecarver · 2021-02-15T00:54:47Z

@swift-ci please test.

zoecarver added the c++ interop Feature: Interoperability with C++ label Dec 14, 2020

zoecarver requested review from slavapestov, hlopko, gribozavr, scentini and varungandhi-apple December 14, 2020 20:02

zoecarver commented Dec 14, 2020

View reviewed changes

gribozavr reviewed Dec 21, 2020

View reviewed changes

zoecarver force-pushed the cxx/central-cxx-module branch 2 times, most recently from 95998c9 to 44ffb2f Compare December 22, 2020 05:28

zoecarver commented Dec 22, 2020

View reviewed changes

test/Interop/Cxx/namespace/Inputs/namespaces.h Outdated Show resolved Hide resolved

zoecarver force-pushed the cxx/central-cxx-module branch 2 times, most recently from ba9a7bd to 6f5c6e3 Compare December 22, 2020 20:12

zoecarver force-pushed the cxx/central-cxx-module branch from 6f5c6e3 to e578027 Compare December 22, 2020 21:43

gribozavr reviewed Jan 14, 2021

View reviewed changes

zoecarver force-pushed the cxx/central-cxx-module branch from e578027 to 929ca8e Compare January 17, 2021 19:50

zoecarver mentioned this pull request Jan 17, 2021

[cxx-interop] Support forward declared records inside other records. #34864

Merged

gribozavr reviewed Jan 18, 2021

View reviewed changes

zoecarver mentioned this pull request Jan 22, 2021

[cxx-interop] Correctly import static function templates. #35548

Merged

zoecarver force-pushed the cxx/central-cxx-module branch from 929ca8e to 76de0a8 Compare January 22, 2021 04:25

zoecarver commented Jan 22, 2021

View reviewed changes

test/Interop/Cxx/namespace/free-functions.swift Outdated Show resolved Hide resolved

zoecarver force-pushed the cxx/central-cxx-module branch from 76de0a8 to 45fc817 Compare January 22, 2021 04:36

zoecarver force-pushed the cxx/central-cxx-module branch from 45fc817 to 884efc0 Compare January 25, 2021 22:03

zoecarver force-pushed the cxx/central-cxx-module branch from 884efc0 to 4c3b4d5 Compare January 25, 2021 22:46

zoecarver force-pushed the cxx/central-cxx-module branch from 4c3b4d5 to 97f844a Compare January 26, 2021 03:52

zoecarver force-pushed the cxx/central-cxx-module branch from 97f844a to d5707c8 Compare January 26, 2021 06:11

zoecarver force-pushed the cxx/central-cxx-module branch from d5707c8 to ba695b6 Compare January 27, 2021 04:19

zoecarver force-pushed the cxx/central-cxx-module branch from ba695b6 to 7a89d9e Compare February 3, 2021 22:27

zoecarver commented Feb 3, 2021

View reviewed changes

gribozavr approved these changes Feb 13, 2021

View reviewed changes

zoecarver force-pushed the cxx/central-cxx-module branch 3 times, most recently from 1ad2c83 to 4c5cbbc Compare February 15, 2021 00:41

zoecarver force-pushed the cxx/central-cxx-module branch from 4c5cbbc to bd96959 Compare February 15, 2021 00:54

zoecarver merged commit ea9ce7e into swiftlang:main Feb 15, 2021

		// This will be re-set as the enum after we return from
		// "VisitNamespaceDecl."

	inline void ChildWithFunctions::forwardDeclared(){};
	inline void ChildWithFunctions::forwardDeclared() {}

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

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

Uh oh!

Conversation

zoecarver commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zoecarver commented Dec 22, 2020

Uh oh!

zoecarver commented Dec 22, 2020

Uh oh!

zoecarver commented Dec 22, 2020

Uh oh!

zoecarver commented Dec 22, 2020

Uh oh!

zoecarver commented Dec 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoecarver commented Jan 17, 2021

Uh oh!

zoecarver commented Jan 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zoecarver commented Jan 22, 2021

Uh oh!

zoecarver commented Jan 25, 2021

Uh oh!

zoecarver commented Jan 25, 2021

Uh oh!

zoecarver commented Jan 26, 2021

Uh oh!

zoecarver commented Jan 26, 2021

Uh oh!

zoecarver commented Jan 27, 2021

Uh oh!

zoecarver commented Jan 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Dec 14, 2020 •

edited

Loading