[CodeComplete] Cleanup access checking in code completion

[ilya-biryukov]

Summary

Also fixes a crash (see the added 'accessibility-crash.cpp' test).

[ilya-biryukov]

• Add missed field init
• Also fix access checks for member access with qualifiers

[ilya-biryukov]

• Fix a comment

[kadircet]

lib/Parse/ParseExprCXX.cpp

248

What about first checking for whether we are on code completion point and then assigning nullptr to objecttype?

Also it is not in the scope of this patch but I was wondering why we only trigger completion if we've seen a scope specifier, do you have any idea?

[ilya-biryukov]

• Do not introduce a new local var, reuse existing

[ilya-biryukov]

lib/Parse/ParseExprCXX.cpp

248

Done. This got me thinking that we probably won't pass the object type on more than 1 qualifiers, e.g. X().Foo::Bar::baz(). I'll test and address this in a separate patch, though.

Also it is not in the scope of this patch but I was wondering why we only trigger completion if we've seen a scope specifier, do you have any idea?

I haven't looked closely into the code, but I suspect other cases are handled elsewhere, e.g. the case with no qualifiers is handled when parsing member expressions, etc.

include/clang/Sema/Sema.h

AccessResult CheckAddressOfMemberAccess(Expr *OvlExpr,
                                           DeclAccessPair FoundDecl);
   AccessResult CheckBaseClassAccess(SourceLocation AccessLoc,
                                     QualType Base, QualType Derived,
                                     const CXXBasePath &Path,
                                     unsigned DiagID,
                                     bool ForceCheck = false,
                                     bool ForceUnprivileged = false);
   void CheckLookupAccess(const LookupResult &R);
-  bool IsSimplyAccessible(NamedDecl *decl, DeclContext *Ctx);
+  bool IsSimplyAccessible(NamedDecl *Decl, CXXRecordDecl *NamingClass,
+                          QualType BaseType);
   bool isSpecialMemberAccessibleForDeletion(CXXMethodDecl *decl,
                                             AccessSpecifier access,
                                             QualType objectType);
 
   void HandleDependentAccessCheck(const DependentDiagnostic &DD,
                          const MultiLevelTemplateArgumentList &TemplateArgs);
   void PerformDependentDiagnostics(const DeclContext *Pattern,
                         const MultiLevelTemplateArgumentList &TemplateArgs);
QualType ProduceCtorInitMemberSignatureHelp(Scope *S, Decl *ConstructorDecl,
                                               IdentifierInfo *II,
                                               SourceLocation OpenParLoc);
   void CodeCompleteInitializer(Scope *S, Decl *D);
   void CodeCompleteReturn(Scope *S);
   void CodeCompleteAfterIf(Scope *S);
   void CodeCompleteAssignmentRHS(Scope *S, Expr *LHS);
 
   void CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS,
-                               bool EnteringContext);
+                               bool EnteringContext, QualType BaseType);
   void CodeCompleteUsing(Scope *S);
   void CodeCompleteUsingDirective(Scope *S);
   void CodeCompleteNamespaceDecl(Scope *S);
   void CodeCompleteNamespaceAliasDecl(Scope *S);
   void CodeCompleteOperatorName(Scope *S);
   void CodeCompleteConstructorInitializer(
                                 Decl *Constructor,
                                 ArrayRef<CXXCtorInitializer *> Initializers);

lib/Parse/ParseExprCXX.cpp

if (HasScopeSpecifier) {
       //
       //       ::class-name-or-namespace-name::...
       //
       //   the class-name-or-namespace-name is looked up in global scope as a
       //   class-name or namespace-name.
       //
       // To implement this, we clear out the object type as soon as we've
       // seen a leading '::' or part of a nested-name-specifier.
+      ParsedType ObjectTypeForCompletion = ObjectType;
       ObjectType = nullptr;
 
       if (Tok.is(tok::code_completion)) {
         // Code completion for a nested-name-specifier, where the code
         // completion token follows the '::'.
-        Actions.CodeCompleteQualifiedId(getCurScope(), SS, EnteringContext);
+        Actions.CodeCompleteQualifiedId(getCurScope(), SS, EnteringContext,
+                                        ObjectTypeForCompletion.get());
         // Include code completion token into the range of the scope otherwise
         // when we try to annotate the scope tokens the dangling code completion
         // token will cause assertion in
         // Preprocessor::AnnotatePreviousCachedTokens.
         SS.setEndLoc(Tok.getLocation());
         cutOffParsing();
         return true;
       }

lib/Sema/CodeCompleteConsumer.cpp

for (unsigned I = 0; I != NumResults; ++I) {
     case CodeCompletionResult::RK_Declaration:
       OS << *Results[I].Declaration;
       {
         std::vector<std::string> Tags;
         if (Results[I].Hidden)
           Tags.push_back("Hidden");
         if (Results[I].InBaseClass)
           Tags.push_back("InBase");
+        if (Results[I].Availability ==
+            CXAvailabilityKind::CXAvailability_NotAccessible)
+          Tags.push_back("Inaccessible");
         if (!Tags.empty())
           OS << " (" << llvm::join(Tags, ",") << ")";
       }
       if (CodeCompletionString *CCS = Results[I].CreateCodeCompletionString(
               SemaRef, Context, getAllocator(), CCTUInfo,
               includeBriefComments())) {
         OS << " : " << CCS->getAsString();
         if (const char *BriefComment = CCS->getBriefComment())

lib/Sema/SemaAccess.cpp

for (LookupResult::iterator I = R.begin(), E = R.end(); I != E; ++I) {
     }
   }
 }
 
 /// Checks access to Target from the given class. The check will take access
 /// specifiers into account, but no member access expressions and such.
 ///
 /// \param Target the declaration to check if it can be accessed
-/// \param Ctx the class/context from which to start the search
+/// \param NamingClass the class in which the lookup was started.
+/// \param BaseType type of the left side of member access expression.
+///        \p BaseType and \p NamingClass are used for C++ access control.
+///        Depending on the lookup case, they should be set to the following:
+///        - lhs.target (member access without a qualifier):
+///          \p BaseType and \p NamingClass are both the type of 'lhs'.
+///        - lhs.X::target (member access with a qualifier):
+///          BaseType is the type of 'lhs', NamingClass is 'X'
+///        - X::target (qualified lookup without member access):
+///          BaseType is null, NamingClass is 'X'.
+///        - target (unqualified lookup).
+///          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, DeclContext *Ctx) {
-  if (CXXRecordDecl *Class = dyn_cast<CXXRecordDecl>(Ctx)) {
-    if (!Target->isCXXClassMember())
-      return true;
-
+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;
-    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(Target, AS_none), qType);
+    AccessTarget Entity(Context, AccessedEntity::Member, NamingClass,
+                        DeclAccessPair::make(Target, AS_none), BaseType);
     EffectiveContext EC(CurContext);
     return ::IsAccessible(*this, EC, Entity) != ::AR_inaccessible;
   }
 
   if (ObjCIvarDecl *Ivar = dyn_cast<ObjCIvarDecl>(Target)) {
     // @public and @package ivars are always accessible.
     if (Ivar->getCanonicalAccessControl() == ObjCIvarDecl::Public ||
         Ivar->getCanonicalAccessControl() == ObjCIvarDecl::Package)

lib/Sema/SemaCodeComplete.cpp

 
 /// Determines whether the given declaration is an Objective-C
 /// instance variable.
 bool ResultBuilder::IsObjCIvar(const NamedDecl *ND) const {
   return isa<ObjCIvarDecl>(ND);
 }
 
 namespace {
+
 /// Visible declaration consumer that adds a code-completion result
 /// for each visible declaration.
 class CodeCompletionDeclConsumer : public VisibleDeclConsumer {
   ResultBuilder &Results;
-  DeclContext *CurContext;
+  DeclContext *InitialLookupCtx;
+  // NamingClass and BaseType are used for access-checking. See
+  // Sema::IsSimplyAccessible for details.
+  CXXRecordDecl *NamingClass;
+  QualType BaseType;
   std::vector<FixItHint> FixIts;
-  // This is set to the record where the search starts, if this is a record
-  // member completion.
-  RecordDecl *MemberCompletionRecord = nullptr;
 
 public:
   CodeCompletionDeclConsumer(
-      ResultBuilder &Results, DeclContext *CurContext,
-      std::vector<FixItHint> FixIts = std::vector<FixItHint>(),
-      RecordDecl *MemberCompletionRecord = nullptr)
-      : Results(Results), CurContext(CurContext), FixIts(std::move(FixIts)),
-        MemberCompletionRecord(MemberCompletionRecord) {}
+      ResultBuilder &Results, DeclContext *InitialLookupCtx,
+      QualType BaseType = QualType(),
+      std::vector<FixItHint> FixIts = std::vector<FixItHint>())
+      : Results(Results), InitialLookupCtx(InitialLookupCtx),
+        FixIts(std::move(FixIts)) {
+    NamingClass = llvm::dyn_cast<CXXRecordDecl>(InitialLookupCtx);
+    // If BaseType was not provided explicitly, emulate implicit 'this->'.
+    if (BaseType.isNull()) {
+      auto ThisType = Results.getSema().getCurrentThisType();
+      if (!ThisType.isNull()) {
+        assert(ThisType->isPointerType());
+        BaseType = ThisType->getPointeeType();
+        if (!NamingClass)
+          NamingClass = BaseType->getAsCXXRecordDecl();
+      }
+    }
+    this->BaseType = BaseType;
+  }
 
   void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx,
                  bool InBaseClass) override {
-    bool Accessible = true;
-    if (Ctx) {
-      // Set the actual accessing context (i.e. naming class) to the record
-      // context where the search starts. When `InBaseClass` is true, `Ctx`
-      // will be the base class, which is not the actual naming class.
-      DeclContext *AccessingCtx =
-          MemberCompletionRecord ? MemberCompletionRecord : Ctx;
-      Accessible = Results.getSema().IsSimplyAccessible(ND, AccessingCtx);
-    }
+    // 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);
-    Results.AddResult(Result, CurContext, Hiding, InBaseClass);
+    Results.AddResult(Result, InitialLookupCtx, Hiding, InBaseClass);
   }
 
   void EnteredContext(DeclContext *Ctx) override {
     Results.addVisitedContext(Ctx);
   }
 };
 } // namespace
 
case PCC_Condition:
     break;
 
   case PCC_RecoveryInFunction:
     // Unfiltered
     break;
   }
 
   // If we are in a C++ non-static member function, check the qualifiers on
-  // the member function to filter/prioritize the results list and set the
-  // context to the record context so that accessibility check in base class
-  // works correctly.
-  RecordDecl *MemberCompletionRecord = nullptr;
+  // the member function to filter/prioritize the results list.
   if (CXXMethodDecl *CurMethod = dyn_cast<CXXMethodDecl>(CurContext)) {
     if (CurMethod->isInstance()) {
       Results.setObjectTypeQualifiers(
           Qualifiers::fromCVRMask(CurMethod->getTypeQualifiers()));
-      MemberCompletionRecord = CurMethod->getParent();
     }
   }
 
-  CodeCompletionDeclConsumer Consumer(Results, CurContext, /*FixIts=*/{},
-                                      MemberCompletionRecord);
+  CodeCompletionDeclConsumer Consumer(Results, CurContext);
   LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
                      CodeCompleter->includeGlobals(),
                      CodeCompleter->loadExternal());
 
   AddOrdinaryNameResults(CompletionContext, S, *this, Results);
   Results.ExitScope();
 
   switch (CompletionContext) {
AddRecordMembersCompletionResults(Sema &SemaRef, ResultBuilder &Results,
   // for the base object type.
   Results.setObjectTypeQualifiers(BaseType.getQualifiers());
 
   // Access to a C/C++ class, struct, or union.
   Results.allowNestedNameSpecifiers();
   std::vector<FixItHint> FixIts;
   if (AccessOpFixIt)
     FixIts.emplace_back(AccessOpFixIt.getValue());
-  CodeCompletionDeclConsumer Consumer(Results, SemaRef.CurContext,
-                                      std::move(FixIts), RD);
+  CodeCompletionDeclConsumer Consumer(Results, RD, BaseType, std::move(FixIts));
   SemaRef.LookupVisibleDecls(RD, Sema::LookupMemberName, Consumer,
                              SemaRef.CodeCompleter->includeGlobals(),
                              /*IncludeDependentBases=*/true,
                              SemaRef.CodeCompleter->loadExternal());
 
   if (SemaRef.getLangOpts().CPlusPlus) {
     if (!Results.empty()) {
       // The "template" keyword can follow "->" or "." in the grammar.
if (const RecordType *Record = BaseType->getAs<RecordType>()) {
       if (const ObjCObjectPointerType *ObjCPtr =
               BaseType->getAs<ObjCObjectPointerType>())
         Class = ObjCPtr->getInterfaceDecl();
       else
         Class = BaseType->getAs<ObjCObjectType>()->getInterface();
 
       // Add all ivars from this class and its superclasses.
       if (Class) {
-        CodeCompletionDeclConsumer Consumer(Results, CurContext);
+        CodeCompletionDeclConsumer Consumer(Results, Class, BaseType);
         Results.setFilter(&ResultBuilder::IsObjCIvar);
         LookupVisibleDecls(
             Class, LookupMemberName, Consumer, CodeCompleter->includeGlobals(),
             /*IncludeDependentBases=*/false, CodeCompleter->loadExternal());
       }
     }
 
     // FIXME: How do we cope with isa?

 void Sema::CodeCompleteAssignmentRHS(Scope *S, Expr *LHS) {
   if (LHS)
     CodeCompleteExpression(S, static_cast<Expr *>(LHS)->getType());
   else
     CodeCompleteOrdinaryName(S, PCC_Expression);
 }
 
 void Sema::CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS,
-                                   bool EnteringContext) {
+                                   bool EnteringContext, QualType BaseType) {
   if (SS.isEmpty() || !CodeCompleter)
     return;
 
   // We want to keep the scope specifier even if it's invalid (e.g. the scope
   // "a::b::" is not corresponding to any context/namespace in the AST), since
   // it can be useful for global code completion which have information about
   // contexts/symbols that are not in the AST.
   if (SS.isInvalid()) {
void Sema::CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS,
   // in a context that permits expressions. This is a general issue with
   // qualified-id completions.
   if (!EnteringContext)
     MaybeAddOverrideCalls(*this, Ctx, Results);
   Results.ExitScope();
 
   if (CodeCompleter->includeNamespaceLevelDecls() ||
       (!Ctx->isNamespace() && !Ctx->isTranslationUnit())) {
-    CodeCompletionDeclConsumer Consumer(Results, CurContext);
+    CodeCompletionDeclConsumer Consumer(Results, Ctx, BaseType);
     LookupVisibleDecls(Ctx, LookupOrdinaryName, Consumer,
                        /*IncludeGlobalScope=*/true,
                        /*IncludeDependentBases=*/true,
                        CodeCompleter->loadExternal());
   }
 
   auto CC = Results.getCompletionContext();
   CC.setCXXScopeSpecifier(SS);

test/CodeCompletion/accessibility-crash.cpp

+class X {
+public:
+ int pub;
+protected:
+ int prot;
+private:
+ int priv;
+};
+
+class Y : public X {
+  int test() {
+    []() {
+
+      // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:13:1 %s -o - \
+      // RUN: | FileCheck %s
+      // CHECK: priv (InBase,Inaccessible)
+      // CHECK: prot (InBase)
+      // CHECK: pub (InBase)
+    };
+  }
+};
+
+

test/CodeCompletion/accessibility.cpp

+class X {
+public:
+ int pub;
+protected:
+ int prot;
+private:
+ int priv;
+};
+
+class Unrelated {
+public:
+  static int pub;
+protected:
+  static int prot;
+private:
+  static int priv;
+};
+
+class Y : public X {
+  int test() {
+    this->pub = 10;
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:11 %s -o - \
+    // RUN: | FileCheck -check-prefix=THIS %s
+    // THIS: priv (InBase,Inaccessible)
+    // THIS: prot (InBase)
+    // THIS: pub (InBase)
+    //
+    // Also check implicit 'this->', i.e. complete at the start of the line.
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:1 %s -o - \
+    // RUN: | FileCheck -check-prefix=THIS %s
+
+    X().pub + 10;
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:32:9 %s -o - \
+    // RUN: | FileCheck -check-prefix=X-OBJ %s
+    // X-OBJ: priv (Inaccessible)
+    // X-OBJ: prot (Inaccessible)
+    // X-OBJ: pub : [#int#]pub
+    
+    Y().pub + 10;
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:39:9 %s -o - \
+    // RUN: | FileCheck -check-prefix=Y-OBJ %s
+    // Y-OBJ: priv (InBase,Inaccessible)
+    // Y-OBJ: prot (InBase)
+    // Y-OBJ: pub (InBase)
+
+    this->X::pub = 10;
+    X::pub = 10;
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:46:14 %s -o - \
+    // RUN: | FileCheck -check-prefix=THIS-BASE %s
+    //
+    // THIS-BASE: priv (Inaccessible)
+    // THIS-BASE: prot : [#int#]prot
+    // THIS-BASE: pub : [#int#]pub
+    //
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:47:8 %s -o - \
+    // RUN: | FileCheck -check-prefix=THIS-BASE %s
+    
+
+    this->Unrelated::pub = 10; // a check we don't crash in this cases.
+    Y().Unrelated::pub = 10; // a check we don't crash in this cases.
+    Unrelated::pub = 10;
+    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:59:22 %s -o - \
+    // RUN: | FileCheck -check-prefix=UNRELATED %s
+    // UNRELATED: priv (Inaccessible)
+    // UNRELATED: prot (Inaccessible)
+    // 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
+  }
+};