[CodeComplete] Fix accessibilty of protected members from base class.

[ioeric]

Summary

Currently, protected members from base classes are marked as
inaccessible when completing in derived class. This patch fixes the problem by
setting the naming class correctly when looking up results in base class
according to [11.2.p5].

[aaron.ballman]

Adding Richard to see if he agrees with the direction taken.

lib/Sema/SemaAccess.cpp

1874–1876

This seems to contradict the function-level comment that says the check seems to only take the access specifiers into account and not how the member is accessed.

lib/Sema/SemaCodeComplete.cpp

1307

Please do not use auto as the type is not spelled out in the initialization.

1316–1317

This makes it sound like the caller has messed something up, but I'm also pretty unfamiliar with the code completion logic. Why would InBaseClass be true for that completion point and why would the context be set to B?

1319

You can drop the llvm::

1322

Please don't use auto here either.

1324

You can drop the llvm::

[ioeric]

• addressed review comments.

[ioeric]

Thanks for the review!

lib/Sema/SemaAccess.cpp

1874–1876

not how the member is accessed.

It's not very clear to me what exactly this means. But I think we are still not taking how member was accessed into account. The access in DeclAccessPair describes how a member was accessed, and this should be None instead of the access specifier of the member as we don't have this information here.

I updated the wording in the comment to make this a bit clearer.

lib/Sema/SemaCodeComplete.cpp

1316–1317

It's a bit messy, but this is roughly how lookup inside a class, say D,works:

1. lookup inside the class (InBaseClass = false, Context=D).
2. Run the same lookup on all base classes (InBaseClass=true, Context=B). (https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaLookup.cpp#L3584)

So, InBaseClass would be true when a lookup starts from a derived class.

[aaron.ballman]

lib/Sema/SemaAccess.cpp

1874–1876

I'm still wondering about the removal of the public access check -- if the declaration has a public access specifier, that early return saves work. So Entity may still require passing AS_none but we might want to continue to check Decl->getAccess() == AS_public for the early return?

(Drive-by nit that's not yours: Decl is the name of a type, so if you wanted to rename the parameter to something that isn't a type name, I would not be sad, but you're not required to do it.)

lib/Sema/SemaCodeComplete.cpp

1316–1317

Ah, thank you for the explanation; that helps.

[ioeric]

• addressed review comments.
• Addressed review comments.

[ioeric]

lib/Sema/SemaAccess.cpp

1874–1876

Added Decl->getAccess() == AS_public back to preserve the behavior.

s/Decl/Target/

[aaron.ballman]

LGTM!

lib/Sema/SemaAccess.cpp

 //===---- SemaAccess.cpp - C++ Access Control -------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file provides Sema routines for C++ access control semantics.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/Basic/Specifiers.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DependentDiagnostic.h"
 #include "clang/AST/ExprCXX.h"
if (I.getAccess() != AS_public) {
                           R.getNamingClass(), I.getPair(),
                           R.getBaseObjectType());
       Entity.setDiag(diag::err_access);
       CheckAccess(*this, R.getNameLoc(), Entity);
     }
   }
 }
 
-/// Checks access to Decl from the given class. The check will take access
+/// Checks access to Target from the given class. The check will take access
 /// specifiers into account, but no member access expressions and such.
 ///
-/// \param Decl the declaration to check if it can be accessed
+/// \param Target the declaration to check if it can be accessed
 /// \param Ctx the class/context from which to start the search
-/// \return true if the Decl is accessible from the Class, false otherwise.
-bool Sema::IsSimplyAccessible(NamedDecl *Decl, DeclContext *Ctx) {
+/// \return true if the Target is accessible from the Class, false otherwise.
+bool Sema::IsSimplyAccessible(NamedDecl *Target, DeclContext *Ctx) {
   if (CXXRecordDecl *Class = dyn_cast<CXXRecordDecl>(Ctx)) {
-    if (!Decl->isCXXClassMember())
+    if (!Target->isCXXClassMember())
       return true;
 
+    if (Target->getAccess() == AS_public)
+      return true;
     QualType qType = Class->getTypeForDecl()->getCanonicalTypeInternal();
+    // 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, Class,
-                        DeclAccessPair::make(Decl, Decl->getAccess()),
-                        qType);
-    if (Entity.getAccess() == AS_public)
-      return true;
-
+                        DeclAccessPair::make(Target, AS_none), qType);
     EffectiveContext EC(CurContext);
     return ::IsAccessible(*this, EC, Entity) != ::AR_inaccessible;
   }
-  
-  if (ObjCIvarDecl *Ivar = dyn_cast<ObjCIvarDecl>(Decl)) {
+  if (ObjCIvarDecl *Ivar = dyn_cast<ObjCIvarDecl>(Target)) {
     // @public and @package ivars are always accessible.
     if (Ivar->getCanonicalAccessControl() == ObjCIvarDecl::Public ||
         Ivar->getCanonicalAccessControl() == ObjCIvarDecl::Package)
       return true;
 
     // If we are inside a class or category implementation, determine the
     // interface we're in.
     ObjCInterfaceDecl *ClassOfMethodDecl = nullptr;

lib/Sema/SemaCodeComplete.cpp

public:
     CodeCompletionDeclConsumer(
         ResultBuilder &Results, DeclContext *CurContext,
         std::vector<FixItHint> FixIts = std::vector<FixItHint>())
         : Results(Results), CurContext(CurContext), FixIts(std::move(FixIts)) {}
 
     void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx,
                    bool InBaseClass) override {
       bool Accessible = true;
-      if (Ctx)
-        Accessible = Results.getSema().IsSimplyAccessible(ND, Ctx);
+      if (Ctx) {
+        DeclContext *AccessingCtx = Ctx;
+        // If ND comes from a base class, set the naming class back to the
+        // derived class if the search starts from the derived class (i.e.
+        // InBaseClass is true).
+        //
+        // Example:
+        //   class B { protected: int X; }
+        //   class D : public B { void f(); }
+        //   void D::f() { this->^; }
+        // The completion after "this->" will have `InBaseClass` set to true and
+        // `Ctx` set to "B", when looking up in `B`. We need to set the actual
+        // accessing context (i.e. naming class) to "D" so that access can be
+        // calculated correctly.
+        if (InBaseClass && isa<CXXRecordDecl>(Ctx)) {
+          CXXRecordDecl *RC = nullptr;
+          // Get the enclosing record.
+          for (DeclContext *DC = CurContext; !DC->isFileContext();
+               DC = DC->getParent()) {
+            if ((RC = dyn_cast<CXXRecordDecl>(DC)))
+              break;
+          }
+          if (RC)
+            AccessingCtx = RC;
+        }
+        Accessible = Results.getSema().IsSimplyAccessible(ND, AccessingCtx);
+      }
 
       ResultBuilder::Result Result(ND, Results.getBasePriority(ND), nullptr,
                                    false, Accessible, FixIts);
       Results.AddResult(Result, CurContext, Hiding, InBaseClass);
     }
 
     void EnteredContext(DeclContext* Ctx) override {
       Results.addVisitedContext(Ctx);

test/Index/complete-access-checks.cpp

void Y::doSomething() {
 
   Z that;
   // RUN: c-index-test -code-completion-at=%s:34:8 %s | FileCheck -check-prefix=CHECK-ACCESS %s
   that.
 }
 
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{TypedText doSomething}{LeftParen (}{RightParen )} (34)
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative X::}{TypedText func1}{LeftParen (}{RightParen )} (36)
-// CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative X::}{TypedText func2}{LeftParen (}{RightParen )} (36) (inaccessible)
+// CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative X::}{TypedText func2}{LeftParen (}{RightParen )} (36)
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative X::}{TypedText func3}{LeftParen (}{RightParen )} (36) (inaccessible)
 // CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative X::}{TypedText member1} (37)
-// CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative X::}{TypedText member2} (37) (inaccessible)
+// CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative X::}{TypedText member2} (37)
 // CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative X::}{TypedText member3} (37) (inaccessible)
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType Y &}{TypedText operator=}{LeftParen (}{Placeholder const Y &}{RightParen )} (79)
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType X &}{Text X::}{TypedText operator=}{LeftParen (}{Placeholder const X &}{RightParen )} (81)
 // CHECK-SUPER-ACCESS: StructDecl:{TypedText X}{Text ::} (77)
 // CHECK-SUPER-ACCESS: StructDecl:{TypedText Y}{Text ::} (75)
 // CHECK-SUPER-ACCESS: CXXDestructor:{ResultType void}{Informative X::}{TypedText ~X}{LeftParen (}{RightParen )} (81)
 // CHECK-SUPER-ACCESS: CXXDestructor:{ResultType void}{TypedText ~Y}{LeftParen (}{RightParen )} (79)
 

 
 // CHECK-USING-ACCESSIBLE: FieldDecl:{ResultType int}{TypedText member} (35)
 // CHECK-USING-ACCESSIBLE: CXXMethod:{ResultType Q &}{TypedText operator=}{LeftParen (}{Placeholder const Q &}{RightParen )} (79)
 // CHECK-USING-ACCESSIBLE: CXXMethod:{ResultType P &}{Text P::}{TypedText operator=}{LeftParen (}{Placeholder const P &}{RightParen )} (81)
 // CHECK-USING-ACCESSIBLE: ClassDecl:{TypedText P}{Text ::} (77)
 // CHECK-USING-ACCESSIBLE: ClassDecl:{TypedText Q}{Text ::} (75)
 // CHECK-USING-ACCESSIBLE: CXXDestructor:{ResultType void}{Informative P::}{TypedText ~P}{LeftParen (}{RightParen )} (81)
 // CHECK-USING-ACCESSIBLE: CXXDestructor:{ResultType void}{TypedText ~Q}{LeftParen (}{RightParen )} (79)
+
+class B {
+protected:
+  int member;
+};
+
+class C : private B {};
+
+
+class D : public C {
+ public:
+  void f(::B *b);
+};
+
+void D::f(::B *that) {
+  // RUN: c-index-test -code-completion-at=%s:106:9 %s | FileCheck -check-prefix=CHECK-PRIVATE-SUPER-THIS %s
+  this->;
+// CHECK-PRIVATE-SUPER-THIS: FieldDecl:{ResultType int}{Informative B::}{TypedText member} (37) (inaccessible)
+
+  // RUN: c-index-test -code-completion-at=%s:110:9 %s | FileCheck -check-prefix=CHECK-PRIVATE-SUPER-THAT %s
+  that->;
+// CHECK-PRIVATE-SUPER-THAT: FieldDecl:{ResultType int}{TypedText member} (35) (inaccessible)
+}