[ClangImporter] Suffix ambiguous protocol names if C++ interop is enabled #41410

drodriguez · 2022-02-16T20:24:25Z

The Clang Importer when C++ interop is not enabled, disambigate an Obj-C
class and protocol that are named the same by appending Protocol to
the protocol. This was not happening when C++ interop was enabled, but
should also apply to Obj-C++ modules.

The fix is providing an starting scope for the search, which the C++
name lookup need to actually find the similarly named counterpart.

Includes a test to avoid this problem creeping in again, and locally it
did not break any other tests.

drodriguez · 2022-02-16T20:24:35Z

@swift-ci please smoke test

zoecarver

This seems reasonable. Thanks for fixing it!

Would you mind adding a module-interface test too? It would be good to make sure the imported names are correct (i.e., the "Protocol" suffix is actually added).

hyp · 2022-02-18T02:50:32Z

lib/ClangImporter/ImportName.cpp

    if (std::any_of(lookupResult.begin(), lookupResult.end(), conflicts))
      return true;
  }

  lookupResult.clear(clang::Sema::LookupTagName);
-  if (clangSema.LookupName(lookupResult, /*scope=*/nullptr)) {
+  if (clangSema.LookupName(lookupResult, /*scope=*/clangSema.TUScope)) {


The second lookup performs a LookupTagName, which I believe should not produce more lookup results in C++ (In C it can though). In that case , do we really need to adjust the scope here? Also, if this lookup isn't needed in C++, can we just wrap it in an if when clang's lang options have C++ on to remove the redundant lookup?

I can try both theories and I see if it changes any result.

I found the problem with the first, and I thought that keeping both with the same scope made sense, but it might not be so.

Neither changing back to nullptr, nor wrapping the lookup for a check of C++ changed the result of the test, or the results in the rest of the test suite, so it seems safe to assume that LookupTagName is not necessary. Thanks for the help!

drodriguez · 2022-02-18T06:27:31Z

@swift-ci please smoke test

drodriguez · 2022-02-18T08:29:18Z

Would you mind adding a module-interface test too? It would be good to make sure the imported names are correct (i.e., the "Protocol" suffix is actually added).

Good intuition. It doesn't seem that the protocol gets imported. I am still trying to figure out why. For some reason the name requests for the class name end up in the machinery that I would expect, but the request for the *Protocol stops in the SwiftLookupTable. I will keep looking.

zoecarver · 2022-03-08T01:17:09Z

@drodriguez is it okay to land this now? Maybe we can investigate after the fact?

drodriguez · 2022-03-08T01:31:41Z

@bulbazord was looking at little bit more into this problem and I think he was close to find out the reason. He might be able to determine if this is going to be good, or going to be discarded in the final solution.

drodriguez · 2022-03-08T03:54:55Z

lib/ClangImporter/ImportName.cpp

-    if (std::any_of(lookupResult.begin(), lookupResult.end(), conflicts))
-      return true;
+  // No need to lookup tags if we are using C++ mode.
+  if (clang::LangStandard::getLangStandardForKind(


The check is the opposite of what it should be.

Yep. Noted.

zoecarver · 2022-03-08T19:17:19Z

lib/ClangImporter/ImportName.cpp

-  if (clangSema.LookupName(lookupResult, /*scope=*/nullptr)) {
-    if (std::any_of(lookupResult.begin(), lookupResult.end(), conflicts))
-      return true;
+  // No need to lookup tags if we are using C++ mode.


You can also use ASTContext.LangOpts.EnableCXXInterop, either way.

bulbazord · 2022-03-08T21:27:33Z

I believe I have discovered the root cause of the bug(s). Effectively, the function being modified NameImporter::hasNamingConflict is used in a few scenarios. I will list the two scenarios relevant to the bug(s) occurring here.

Scenario 1: We are looking for the right clang::Decl to use when importing something from Objective-C. As the test for the patch demonstrates, you have conflicting names for an ObjC protocol and an ObjC class. Both will get looked at when we are trying to use one of them from swift code, so we need to disambiguate by appending the word “Protocol” to the end of the protocol name. If you take the patch as it is laid out here, you will be able to subclass the ObjC class successfully. Conforming to the protocol on the other hand is still problematic. I will explain why in Scenario 2.

Scenario 2: NameImporter::hasNamingConflict (and the function that calls it NameImporter::importNameImpl) are also a part of the code path that writes the SwiftLookupTable extension block into the module itself. NameImporter::importNameImpl will be called to check each decl to see if it needs a swift name different than what is imported (e.g. @protocol Foo -> protocol FooProtocol).

NameImporter::hasNamingConflict makes use of clang::Sema::LookupName which takes as argument a clang::Sema::Scope *. The right thing to pass here is indeed clangSema.TUScope and not nullptr, but unfortunately that is only works when clangSema.TUScope != nullptr. This only occurs when you have an active clang::Parser. Scenario 1 is called in a situation where we have an active ClangImporter object, which actively creates an accompanying clang::Parser to the clang::Sema being used (Refer to ClangImporter::create where you manually run the steps to create and initialize a clang::Parser). This means that in Scenario 1, clangSema.TUScope will not be null. Unfortunately with Scenario 2, by the time we have gotten to NameImporter::hasNamingConflict, the clang::Sema’s associated clang::Parser has finished the parse, and thus clangSema.TUScope will be nullptr.

To recap: clangSema.TUScope will be nullptr when populating the SwiftLookupTable during PCM creation time. This means that anything that should get a different name when importing into swift will not be visible (e.g. FooProtocol will not be visible when importing @protocol Foo from ObjC).

How do we fix this? I have 3 general routes in mind:
1.) We can change NameImporter::hasNamingConflict to not use clang::Sema::LookupName, but instead use some other method of finding conflicting names. Potentially we can walk the AST we already have access to?
2.) We can change SwiftLookupTableWriter::populateTable to create a clang::Parser to match the clang::Sema we have and re-parse the file the module was built from so we can continue to use clang::Sema::LookupName.
3.) We can change clang::Sema::LookupName to not fail when the scope passed in is nullptr when C++-interop is enabled.

I think option 3 is probably not the right thing to do. I experimented with option 2, but I didn’t get very far and I’m not familiar enough with clang to know how viable this option even is. I’d like to say 1 is the better option here, but I’m not sure. What do y’all think?

hyp · 2022-03-11T05:28:23Z

I think that NameImporter::hasNamingConflict can either use another API, or do a C-language based lookup using LookupName (e.g. by using a different set of LangOpts) most likely to identify any naming conflicts. Wdyt?

zoecarver · 2022-03-11T19:22:55Z

Here's what I think we should do. Basically, the failure mode that you're investigating is when clangSema.TUScope is nullptr or invalid, right? Before this patch, it was always nullptr, so it's not like this PR is introducing a regression. It seems to me like we have two different issues. I think this patch fixes an important bug, let's land it. We can investigate the other failure as a follow up PR with its own test, etc. WDYT?

zoecarver · 2022-03-11T19:23:03Z

@swift-ci please test.

zoecarver · 2022-03-11T19:23:28Z

lib/ClangImporter/ImportName.cpp

-    if (std::any_of(lookupResult.begin(), lookupResult.end(), conflicts))
-      return true;
+  // No need to lookup tags if we are using C++ mode.
+  if (clang::LangStandard::getLangStandardForKind(


Once this is fixed, my approval stands. Let's land this!

zoecarver · 2022-03-11T19:23:50Z

@swift-ci please test.

…bled The Clang Importer when C++ interop is not enabled, disambigate an Obj-C class and protocol that are named the same by appending `Protocol` to the protocol. This was not happening when C++ interop was enabled, but should also apply to Obj-C++ modules. The fix is providing an starting scope for the search, which the C++ name lookup need to actually find the similarly named counterpart. Includes a test to avoid this problem creeping in again, and locally it did not break any other tests.

drodriguez · 2022-03-12T01:20:05Z

@swift-ci please smoke test and merge

drodriguez · 2022-03-12T01:21:35Z

Something interesting I noticed while flipping the condition: it doesn't seem to matter in any of the tests of the suite. I was running those tests to check that nothing broke, and in both negated and not negated all tests pass.

drodriguez requested a review from zoecarver February 16, 2022 22:05

zoecarver approved these changes Feb 18, 2022

View reviewed changes

hyp reviewed Feb 18, 2022

View reviewed changes

drodriguez force-pushed the class-protocol-name-clash branch from 355c518 to 8c931c0 Compare February 18, 2022 06:27

zoecarver mentioned this pull request Mar 4, 2022

[cxx-interop] Fix looking up objc names when cxx-interop enabled #41675

Closed

zoecarver added the c++ interop Feature: Interoperability with C++ label Mar 8, 2022

zoecarver approved these changes Mar 8, 2022

View reviewed changes

drodriguez commented Mar 8, 2022

View reviewed changes

zoecarver reviewed Mar 8, 2022

View reviewed changes

zoecarver approved these changes Mar 11, 2022

View reviewed changes

drodriguez force-pushed the class-protocol-name-clash branch from 8c931c0 to 746080d Compare March 12, 2022 01:19

swift-ci merged commit cb56373 into swiftlang:main Mar 12, 2022

drodriguez deleted the class-protocol-name-clash branch March 14, 2022 19:18

[ClangImporter] Suffix ambiguous protocol names if C++ interop is enabled #41410

[ClangImporter] Suffix ambiguous protocol names if C++ interop is enabled #41410

Uh oh!

Conversation

drodriguez commented Feb 16, 2022

Uh oh!

drodriguez commented Feb 16, 2022

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

hyp Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

drodriguez Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

drodriguez Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

drodriguez commented Feb 18, 2022

Uh oh!

drodriguez commented Feb 18, 2022

Uh oh!

zoecarver commented Mar 8, 2022

Uh oh!

drodriguez commented Mar 8, 2022

Uh oh!

drodriguez Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

plotfi Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

zoecarver Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

bulbazord commented Mar 8, 2022

Uh oh!

hyp commented Mar 11, 2022

Uh oh!

zoecarver commented Mar 11, 2022

Uh oh!

zoecarver commented Mar 11, 2022

Uh oh!

zoecarver Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Mar 11, 2022

Uh oh!

drodriguez commented Mar 12, 2022

Uh oh!

drodriguez commented Mar 12, 2022

Uh oh!

Uh oh!