[clang][Sema] Add flag to LookupName to force C/ObjC codepath

[bulbazord]

Summary

Motivation: The intent here is for use in Swift.
When building a clang module for swift consumption, swift adds an
extension block to the module for name lookup purposes. Swift calls
this a SwiftLookupTable. One purpose that this serves is to handle
conflicting names between ObjC classes and ObjC protocols. They exist in
different namespaces in ObjC programs, but in Swift they would exist in
the same namespace. Swift handles this by appending a suffix to a
protocol name if it shares a name with a class. For example, if you have
an ObjC class named "Foo" and a protocol with the same name, the
protocol would be renamed to "FooProtocol" when imported into swift.

When constructing the previously mentioned SwiftLookupTable, we use
Sema::LookupName to look up name conflicts for the previous problem.
By this time, the Parser has long finished its job so the call to
LookupName gets nullptr for its Scope (TUScope will be nullptr
by this point). The C/ObjC path does not have this problem because it
only uses the Scope in specific scenarios. The C++ codepath uses the
Scope quite extensively and will fail early on if the Scope it gets is
null. In our very specific case of looking up ObjC classes with a
specific name, we want to force sema::LookupName to take the C/ObjC
codepath even if C++ or ObjC++ is enabled.

[plotfi]

@rjmccall would you be able to review this patch? What do you think of this approach? This change is to support C++-Interop on the Swift side.

[arphaman]

The change itself LGTM.

It would be really good if we can have a unit test that tests the new API when it lands, to ensure that this is tested in LLVM-project itself , in addition to Swift. Do you think it's possible to write a unit tests that constructs a compiler instance, parses a sample C++ file, and then you can use the Sema to try to invoke this LookupName API to perform the lookup to verify that it works as expected? I think it should be possible to construct such a unit test.

[bulbazord]

Added a test

[arphaman]

Great, thanks for adding the test case!

[bulbazord]

Gah, forgot to add the phabricator link to the commit message. I added the commit object to this diff, hopefully people can track it down the review if needed.

clang/include/clang/Sema/Sema.h

public:
   /// ambiguity and overloaded.
   NamedDecl *LookupSingleName(Scope *S, DeclarationName Name,
                               SourceLocation Loc,
                               LookupNameKind NameKind,
                               RedeclarationKind Redecl
                                 = NotForRedeclaration);
   bool LookupBuiltin(LookupResult &R);
   void LookupNecessaryTypesForBuiltin(Scope *S, unsigned ID);
-  bool LookupName(LookupResult &R, Scope *S,
-                  bool AllowBuiltinCreation = false);
+  bool LookupName(LookupResult &R, Scope *S, bool AllowBuiltinCreation = false,
+                  bool ForceNoCPlusPlus = false);
   bool LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx,
                            bool InUnqualifiedLookup = false);
   bool LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx,
                            CXXScopeSpec &SS);
   bool LookupParsedName(LookupResult &R, Scope *S, CXXScopeSpec *SS,
                         bool AllowBuiltinCreation = false,
                         bool EnteringContext = false);
   ObjCProtocolDecl *LookupProtocol(IdentifierInfo *II, SourceLocation IdLoc,

clang/lib/Sema/SemaLookup.cpp

 /// in the parent scopes.
 ///
 /// @param [in,out] R Specifies the lookup to perform (e.g., the name to
 /// look up and the lookup kind), and is updated with the results of lookup
 /// including zero or more declarations and possibly additional information
 /// used to diagnose ambiguities.
 ///
 /// @returns \c true if lookup succeeded and false otherwise.
-bool Sema::LookupName(LookupResult &R, Scope *S, bool AllowBuiltinCreation) {
+bool Sema::LookupName(LookupResult &R, Scope *S, bool AllowBuiltinCreation,
+                      bool ForceNoCPlusPlus) {
   DeclarationName Name = R.getLookupName();
   if (!Name) return false;
 
   LookupNameKind NameKind = R.getLookupKind();
 
-  if (!getLangOpts().CPlusPlus) {
+  if (!getLangOpts().CPlusPlus || ForceNoCPlusPlus) {
     // Unqualified name lookup in C/Objective-C is purely lexical, so
     // search in the declarations attached to the name.
     if (NameKind == Sema::LookupRedeclarationWithLinkage) {
       // Find the nearest non-transparent declaration scope.
       while (!(S->getFlags() & Scope::DeclScope) ||
              (S->getEntity() && S->getEntity()->isTransparentContext()))
         S = S->getParent();
     }

clang/unittests/Sema/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_unittest(SemaTests
   ExternalSemaSourceTest.cpp
   CodeCompleteTest.cpp
   GslOwnerPointerInference.cpp
+  SemaLookupTest.cpp
   )
 
 clang_target_link_libraries(SemaTests
   PRIVATE
   clangAST
   clangASTMatchers
   clangBasic
   clangFrontend

clang/unittests/Sema/SemaLookupTest.cpp

+#include "clang/AST/DeclarationName.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Parse/ParseAST.h"
+#include "clang/Sema/Lookup.h"
+#include "clang/Sema/Sema.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+using namespace clang::tooling;
+
+namespace {
+
+class LookupAction : public ASTFrontendAction {
+  std::unique_ptr<ASTConsumer>
+  CreateASTConsumer(CompilerInstance &CI, StringRef /*Unused*/) override {
+    return std::make_unique<clang::ASTConsumer>();
+  }
+
+  void ExecuteAction() override {
+    CompilerInstance &CI = getCompilerInstance();
+    ASSERT_FALSE(CI.hasSema());
+    CI.createSema(getTranslationUnitKind(), nullptr);
+    ASSERT_TRUE(CI.hasSema());
+    Sema &S = CI.getSema();
+    ParseAST(S);
+
+    ASTContext &Ctx = S.getASTContext();
+    auto Name = &Ctx.Idents.get("Foo");
+    LookupResult R_cpp(S, Name, SourceLocation(), Sema::LookupOrdinaryName);
+    S.LookupName(R_cpp, S.TUScope, /*AllowBuiltinCreation=*/false,
+                 /*ForceNoCPlusPlus=*/false);
+    // By this point, parsing is done and S.TUScope is nullptr
+    // CppLookupName will perform an early return with no results if the Scope
+    // we pass in is nullptr. We expect to find nothing.
+    ASSERT_TRUE(R_cpp.empty());
+
+    // On the other hand, the non-C++ path doesn't care if the Scope passed in
+    // is nullptr. We'll force the non-C++ path with a flag.
+    LookupResult R_nocpp(S, Name, SourceLocation(), Sema::LookupOrdinaryName);
+    S.LookupName(R_nocpp, S.TUScope, /*AllowBuiltinCreation=*/false,
+                 /*ForceNoCPlusPlus=*/true);
+    ASSERT_TRUE(!R_nocpp.empty());
+  }
+};
+
+TEST(SemaLookupTest, ForceNoCPlusPlusPath) {
+  const char *file_contents = R"objcxx(
+@protocol Foo
+@end
+@interface Foo <Foo>
+@end
+  )objcxx";
+  ASSERT_TRUE(runToolOnCodeWithArgs(std::make_unique<LookupAction>(),
+                                    file_contents, {"-x", "objective-c++"},
+                                    "test.mm"));
+}
+} // namespace