From 98397555a51b61fabfc992ebcb054072eb5720c2 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov <ibiryukov@google.com>
Date: Wed, 5 Dec 2018 17:38:39 +0000
Subject: [PATCH] [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

llvm-svn: 348387
---
 clang/lib/Sema/SemaAccess.cpp               |  2 --
 clang/lib/Sema/SemaCodeComplete.cpp         | 41 ++++++++++++++++++------
 clang/test/CodeCompletion/accessibility.cpp | 49 +++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+), 12 deletions(-)

diff --git a/clang/lib/Sema/SemaAccess.cpp b/clang/lib/Sema/SemaAccess.cpp
index 0432e70..6908458 100644
--- a/clang/lib/Sema/SemaAccess.cpp
+++ b/clang/lib/Sema/SemaAccess.cpp
@@ -1896,8 +1896,6 @@ bool Sema::IsSimplyAccessible(NamedDecl *Target, CXXRecordDecl *NamingClass,
   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.
diff --git a/clang/lib/Sema/SemaCodeComplete.cpp b/clang/lib/Sema/SemaCodeComplete.cpp
index cb66a01..7a8f31f0 100644
--- a/clang/lib/Sema/SemaCodeComplete.cpp
+++ b/clang/lib/Sema/SemaCodeComplete.cpp
@@ -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
 
diff --git a/clang/test/CodeCompletion/accessibility.cpp b/clang/test/CodeCompletion/accessibility.cpp
index 754d822..a050efc 100644
--- a/clang/test/CodeCompletion/accessibility.cpp
+++ b/clang/test/CodeCompletion/accessibility.cpp
@@ -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)
+  }
+};
-- 
2.7.4