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

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Wed Dec  5 09:38:39 2018
New Revision: 348387

URL: http://llvm.org/viewvc/llvm-project?rev=348387&view=rev
Log:
[CodeComplete] Fix a crash in access checks of inner classes

Summary: The crash was introduced in r348135.

Reviewers: kadircet

Reviewed By: kadircet

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D55260

Modified:
    cfe/trunk/lib/Sema/SemaAccess.cpp
    cfe/trunk/lib/Sema/SemaCodeComplete.cpp
    cfe/trunk/test/CodeCompletion/accessibility.cpp

Modified: cfe/trunk/lib/Sema/SemaAccess.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAccess.cpp?rev=348387&r1=348386&r2=348387&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaAccess.cpp (original)
+++ cfe/trunk/lib/Sema/SemaAccess.cpp Wed Dec  5 09:38:39 2018
@@ -1896,8 +1896,6 @@ bool Sema::IsSimplyAccessible(NamedDecl
   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.

Modified: cfe/trunk/lib/Sema/SemaCodeComplete.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCodeComplete.cpp?rev=348387&r1=348386&r2=348387&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaCodeComplete.cpp (original)
+++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp Wed Dec  5 09:38:39 2018
@@ -11,6 +11,7 @@
 //
 //===----------------------------------------------------------------------===//
 #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"
@@ -1313,23 +1314,43 @@ public:
 
   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
 

Modified: cfe/trunk/test/CodeCompletion/accessibility.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeCompletion/accessibility.cpp?rev=348387&r1=348386&r2=348387&view=diff
==============================================================================
--- cfe/trunk/test/CodeCompletion/accessibility.cpp (original)
+++ cfe/trunk/test/CodeCompletion/accessibility.cpp Wed Dec  5 09:38:39 2018
@@ -71,3 +71,52 @@ class Y : public X {
     // 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)
+  }
+};