[interop] clang name lookup should find declarations in inline namesp… #40127

hyp · 2021-11-10T20:54:58Z

…aces

hyp · 2021-11-10T20:55:07Z

@swift-ci please test

lib/ClangImporter/ClangImporter.cpp

zoecarver

Some suggestions for tests to add:

Extending an inline namespace in Swift.
A type with the same name in two inline namespaces (this might be an ODR violation...).
A function inside of an inline namespace.
A test for std.inner and a test for std.__1.__2.inner.
If it's allowed, an inline namespace that is at the top level.
(Optional) a namespace inside of an inline namespace inside of a namespace.

zoecarver · 2021-11-10T23:18:00Z

test/Interop/Cxx/namespace/class-inline-namespace-irgen.swift

+// RUN: %target-swift-emit-ir -I %t/Inputs -enable-cxx-interop %t/test.swift | %FileCheck %t/test.swift
+
+
+//--- Inputs/module.modulemap


I love this new way to write tests :)

zoecarver · 2021-11-10T23:18:56Z

test/Interop/Cxx/namespace/class-inline-namespace-irgen.swift

+
+//--- Inputs/module.modulemap
+module std {
+  header "string.h"


string.h is a header that may already be in our search paths, so maybe another name is better.

zoecarver · 2021-11-10T23:20:10Z

test/Interop/Cxx/namespace/class-inline-namespace-irgen.swift

+template<class CharT>
+struct basic_string;
+
+typedef basic_string<char> string;
+
+template<class CharT>
+struct basic_string {
+  void *p;
+
+  const CharT *c_str();
+};


This doesn't seem like a really necessary part of the test. It's sort of complicated. It might be better to just use simple types/functions.

zoecarver · 2021-11-10T23:20:45Z

test/Interop/Cxx/namespace/class-inline-namespace-irgen.swift

+  const CharT *c_str();
+};
+
+struct other {


Generally we use the TitleCase format for types. I know this is a test... but might as well start off right.

zoecarver · 2021-11-10T23:21:08Z

test/Interop/Cxx/namespace/class-inline-namespace-irgen.swift

+
+}
+
+}


Can you mark these as the end of the namespaces they close?

test/Interop/Cxx/namespace/class-inline-namespace-irgen.swift

test/Interop/Cxx/namespace/inline-namespace-ambiguity-error.swift

swift-ci · 2021-11-10T23:52:54Z

Build failed
Swift Test OS X Platform
Git Sha - 595871fa1efab118c92b231c76f235a1ebbe1d12

swift-ci · 2021-11-10T23:54:48Z

Build failed
Swift Test Linux Platform
Git Sha - 595871fa1efab118c92b231c76f235a1ebbe1d12

hyp · 2021-11-11T20:20:41Z

@swift-ci please test

swift-ci · 2021-11-11T21:41:26Z

Build failed
Swift Test Linux Platform
Git Sha - 1ede3efde3b8a4f0db5bbdde935b26194067ed37

zoecarver · 2021-11-11T22:14:03Z

lib/ClangImporter/ClangImporter.cpp

-      });
+  llvm::copy_if(foundDecls, std::back_inserter(filteredDecls),
+                [clangDecl](SwiftLookupTable::SingleEntry decl) {
+                  auto foundClangDecl = decl.dyn_cast<clang::NamedDecl *>();


Did you run into problems with this? Macros can't be members so this should be fine.

yes, this broke the stdlib build.

Hmm, that's super interesting. Do you know how it broke the build or what decl it was? I might go investigate after this lands..

it was p_starttime

which I believe is a macro

That's super interesting... so it's a macro defined to a member of a struct. I guess this is the correct way to handle this...

I wish this could just be a ranges::filter(isa-named-decl) | ranges::filter(...)

zoecarver

Thank you for overhauling the test. They look great now!

lib/AST/NameLookup.cpp

test/Interop/Cxx/namespace/class-inline-namespace-irgen.swift

swift-ci · 2021-11-11T22:28:25Z

Build failed
Swift Test OS X Platform
Git Sha - 1ede3efde3b8a4f0db5bbdde935b26194067ed37

hyp · 2021-11-12T21:21:10Z

@swift-ci please test

swift-ci · 2021-11-12T23:17:21Z

Build failed
Swift Test Linux Platform
Git Sha - 34656fdab9408bb0a1e1489dedd7c30751517d5d

hyp · 2021-11-12T23:17:54Z

@swift-ci please test

zoecarver · 2021-11-12T23:25:37Z

lib/AST/NameLookup.cpp

+      // declarations due to inline namespaces. We still merge in the other
+      // entries found in the lookup table, to support finding members in
+      // namespace extensions.
+      if (!allFound.empty()) {


If you're going to duplicate the code anyway, might as well unconditionally take this path.

The code isn't duplicated, the regular path does something additional - it marks the name as markLazilyComplete in the table.

OK, so, if we know that there are no lazy members (i.e., all members are in the lookup table for a given name), then we don't have to emit a namespace lookup request? Makes sense.

zoecarver

Looks good. Thanks for the diligent updates, Alex!

swift-ci · 2021-11-13T00:38:49Z

Build failed
Swift Test Linux Platform
Git Sha - 71e9119c7fe495ccf184abfc90b115e01ead931c

hyp · 2021-11-13T01:35:43Z

@swift-ci please test

…aces

hyp · 2021-11-13T01:41:01Z

@swift-ci please test

hyp · 2021-11-13T03:00:15Z

@swift-ci please test source compatibility

zoecarver · 2021-11-13T19:42:30Z

@hyp I don't think a source compatibility check is really necessary here. It seems very unlikely this will break source compatibility as we have few projects that use Objective-C interop and none that use C++ interop.

I think the failures you're seeing for source compatibility are failing on main and will be resolved by #40143. Anyway, I think you're good to merge if you want.

hyp · 2021-11-13T20:04:44Z

@hyp I don't think a source compatibility check is really necessary here. It seems very unlikely this will break source compatibility as we have few projects that use Objective-C interop and none that use C++ interop.

I think the failures you're seeing for source compatibility are failing on main and will be resolved by #40143. Anyway, I think you're good to merge if you want.

I know, I saw that the same failures are in the baseline :) I was just curious to see what the source compatibility test would do.

zoecarver · 2021-11-13T21:03:20Z

I know I just approved this patch, but I also just had a thought: this will crash if someone tries to call a function in an inline namespace from the parent namespace, right? If so, that's really not a great failure mode on our part. Ideally we'd have a diagnostic, but it probably doesn't make sense to spend time adding that, as we want to fix this issue pretty soon. How hard would it be to filter out FuncDecls when filter on the inline parent? Maybe one of us can make a patch to do this next week (or maybe it's not needed depending on how quickly we plan to fix the underlying issue).

hyp · 2021-11-13T22:06:23Z

If so, that's really not a great failure mode on our part. Ideally we'd have a diagnostic, but it probably doesn't make sense to spen

It produces a diagnostic , unless I'm misunderstanding your scenario. One of the test cases covers it.

zoecarver · 2021-11-14T00:57:59Z

It produces a diagnostic , unless I'm misunderstanding your scenario. One of the test cases covers it.

Ah, no, you're right. It doesn't crash. I was mis remembering.

hyp requested review from guitard0g and zoecarver November 10, 2021 20:54

zoecarver added the c++ interop Feature: Interoperability with C++ label Nov 10, 2021

zoecarver reviewed Nov 10, 2021

View reviewed changes

lib/ClangImporter/ClangImporter.cpp Outdated Show resolved Hide resolved

zoecarver reviewed Nov 10, 2021

View reviewed changes

test/Interop/Cxx/namespace/class-inline-namespace-irgen.swift Outdated Show resolved Hide resolved

zoecarver reviewed Nov 10, 2021

View reviewed changes

test/Interop/Cxx/namespace/inline-namespace-ambiguity-error.swift Show resolved Hide resolved

hyp force-pushed the eng/cxx-inline-ns-lookup branch from 595871f to 1ede3ef Compare November 11, 2021 20:20

zoecarver reviewed Nov 11, 2021

View reviewed changes

lib/AST/NameLookup.cpp Outdated Show resolved Hide resolved

test/Interop/Cxx/namespace/class-inline-namespace-irgen.swift Outdated Show resolved Hide resolved

test/Interop/Cxx/namespace/class-inline-namespace-irgen.swift Outdated Show resolved Hide resolved

hyp force-pushed the eng/cxx-inline-ns-lookup branch from 1ede3ef to 34656fd Compare November 12, 2021 21:20

hyp force-pushed the eng/cxx-inline-ns-lookup branch from 34656fd to 71e9119 Compare November 12, 2021 23:17

zoecarver reviewed Nov 12, 2021

View reviewed changes

zoecarver approved these changes Nov 12, 2021

View reviewed changes

hyp force-pushed the eng/cxx-inline-ns-lookup branch from 71e9119 to d570826 Compare November 13, 2021 01:35

[interop] clang name lookup should find declarations in inline namesp…

1089959

…aces

hyp force-pushed the eng/cxx-inline-ns-lookup branch from d570826 to 1089959 Compare November 13, 2021 01:40

hyp merged commit 04f6062 into swiftlang:main Nov 13, 2021

		// RUN: %target-swift-emit-ir -I %t/Inputs -enable-cxx-interop %t/test.swift \| %FileCheck %t/test.swift


		//--- Inputs/module.modulemap

[interop] clang name lookup should find declarations in inline namesp… #40127

[interop] clang name lookup should find declarations in inline namesp… #40127

Uh oh!

Conversation

hyp commented Nov 10, 2021

Uh oh!

hyp commented Nov 10, 2021

Uh oh!

Uh oh!

zoecarver left a comment • edited by hyp Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

swift-ci commented Nov 10, 2021

Uh oh!

swift-ci commented Nov 10, 2021

Uh oh!

hyp commented Nov 11, 2021

Uh oh!

swift-ci commented Nov 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swift-ci commented Nov 11, 2021

Uh oh!

hyp commented Nov 12, 2021

Uh oh!

swift-ci commented Nov 12, 2021

Uh oh!

hyp commented Nov 12, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Nov 13, 2021

Uh oh!

hyp commented Nov 13, 2021

Uh oh!

hyp commented Nov 13, 2021

Uh oh!

hyp commented Nov 13, 2021

Uh oh!

zoecarver commented Nov 13, 2021

Uh oh!

zoecarver left a comment •

edited by hyp

Loading