[CodeComplete] Fix a crash in access checks of inner classes

[ilya-biryukov]

Summary

The crash was introduced in r348135.

[kadircet]

lib/Sema/SemaCodeComplete.cpp

1348

I suppose Cls should be NamingClass

[ilya-biryukov]

• Actually use the computed NamingClass

[kadircet]

I believe also we need another test case where Cls and NamingClass are different.

[ilya-biryukov]

• Make sure we still run ObjC access checks.
• Add a test with a different NamingClass and BaseType.
• Fix IsSimplyAccessible to work in this case.

[ilya-biryukov]

• Reformat

[ilya-biryukov]

• Add a newline to the end of the test file

[ilya-biryukov]

In D55260#1318280, @kadircet wrote:

I believe also we need another test case where Cls and NamingClass are different.

Done. And thanks for finding this, I totally missed that we don't test this.

[kadircet]

LGTM, thanks!

lib/Sema/SemaAccess.cpp

 ///          BaseType is null, NamingClass is the parent class of 'target'.
 /// \return true if the Target is accessible from the Class, false otherwise.
 bool Sema::IsSimplyAccessible(NamedDecl *Target, CXXRecordDecl *NamingClass,
                               QualType BaseType) {
   // Perform the C++ accessibility checks first.
   if (Target->isCXXClassMember() && NamingClass) {
     if (!getLangOpts().CPlusPlus)
       return false;
-    if (Target->getAccess() == AS_public)
-      return true;
     // The unprivileged access is AS_none as we don't know how the member was
     // accessed, which is described by the access in DeclAccessPair.
     // `IsAccessible` will examine the actual access of Target (i.e.
     // Decl->getAccess()) when calculating the access.
     AccessTarget Entity(Context, AccessedEntity::Member, NamingClass,
                         DeclAccessPair::make(Target, AS_none), BaseType);
     EffectiveContext EC(CurContext);
     return ::IsAccessible(*this, EC, Entity) != ::AR_inaccessible;

lib/Sema/SemaCodeComplete.cpp

 //===---------------- SemaCodeComplete.cpp - Code Completion ----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the code-completion semantic actions.
 //
 //===----------------------------------------------------------------------===//
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/QualTypeNames.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/MacroInfo.h"
if (BaseType.isNull()) {
           NamingClass = BaseType->getAsCXXRecordDecl();
       }
     }
     this->BaseType = BaseType;
   }
 
   void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx,
                  bool InBaseClass) override {
-    // Naming class to use for access check. In most cases it was provided
-    // explicitly (e.g. member access (lhs.foo) or qualified lookup (X::)), for
-    // unqualified lookup we fallback to the \p Ctx in which we found the
-    // member.
-    auto *NamingClass = this->NamingClass;
-    if (!NamingClass)
-      NamingClass = llvm::dyn_cast_or_null<CXXRecordDecl>(Ctx);
-    bool Accessible =
-        Results.getSema().IsSimplyAccessible(ND, NamingClass, BaseType);
     ResultBuilder::Result Result(ND, Results.getBasePriority(ND), nullptr,
-                                 false, Accessible, FixIts);
+                                 false, IsAccessible(ND, Ctx), FixIts);
     Results.AddResult(Result, InitialLookupCtx, Hiding, InBaseClass);
   }
 
   void EnteredContext(DeclContext *Ctx) override {
     Results.addVisitedContext(Ctx);
   }
+
+private:
+  bool IsAccessible(NamedDecl *ND, DeclContext *Ctx) {
+    // Naming class to use for access check. In most cases it was provided
+    // explicitly (e.g. member access (lhs.foo) or qualified lookup (X::)),
+    // for unqualified lookup we fallback to the \p Ctx in which we found the
+    // member.
+    auto *NamingClass = this->NamingClass;
+    QualType BaseType = this->BaseType;
+    if (auto *Cls = llvm::dyn_cast_or_null<CXXRecordDecl>(Ctx)) {
+      if (!NamingClass)
+        NamingClass = Cls;
+      // When we emulate implicit 'this->' in an unqualified lookup, we might
+      // end up with an invalid naming class. In that case, we avoid emulating
+      // 'this->' qualifier to satisfy preconditions of the access checking.
+      if (NamingClass->getCanonicalDecl() != Cls->getCanonicalDecl() &&
+          !NamingClass->isDerivedFrom(Cls)) {
+        NamingClass = Cls;
+        BaseType = QualType();
+      }
+    } else {
+      // The decl was found outside the C++ class, so only ObjC access checks
+      // apply. Those do not rely on NamingClass and BaseType, so we clear them
+      // out.
+      NamingClass = nullptr;
+      BaseType = QualType();
+    }
+    return Results.getSema().IsSimplyAccessible(ND, NamingClass, BaseType);
+  }
 };
 } // namespace
 
 /// Add type specifiers for the current language as keyword results.
 static void AddTypeSpecifierResults(const LangOptions &LangOpts,
                                     ResultBuilder &Results) {
   typedef CodeCompletionResult Result;
   Results.AddResult(Result("short", CCP_Type));

test/CodeCompletion/accessibility.cpp

int test() {
     // UNRELATED: pub : [#int#]pub
     //
     // RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:60:20 %s -o - \
     // RUN: | FileCheck -check-prefix=UNRELATED %s
     // RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:61:16 %s -o - \
     // RUN: | FileCheck -check-prefix=UNRELATED %s
   }
 };
+
+class Outer {
+ public:
+  static int pub;
+ protected:
+  static int prot;
+ private:
+  static int priv;
+
+  class Inner {
+    int test() {
+      Outer::pub = 10;
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:85:14 %s -o - \
+    // RUN: | FileCheck -check-prefix=OUTER %s
+    // OUTER: priv : [#int#]priv
+    // OUTER: prot : [#int#]prot
+    // OUTER: pub : [#int#]pub
+
+    // Also check the unqualified case.
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:85:1 %s -o - \
+    // RUN: | FileCheck -check-prefix=OUTER %s
+    }
+  };
+};
+
+class Base {
+public:
+  int pub;
+};
+
+class Accessible : public Base {
+};
+
+class Inaccessible : private Base {
+};
+
+class Test : public Accessible, public Inaccessible {
+  int test() {
+    this->Accessible::pub = 10;
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:112:23 %s -o - \
+    // RUN: | FileCheck -check-prefix=ACCESSIBLE %s
+    // ACCESSIBLE: pub (InBase)
+
+    this->Inaccessible::pub = 10;
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:117:25 %s -o - \
+    // RUN: | FileCheck -check-prefix=INACCESSIBLE %s
+    // INACCESSIBLE: pub (InBase,Inaccessible)
+  }
+};
 No newline at end of file