[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 ↗

(On Diff #176153)

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 ↗

(On Diff #176153)

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.

cfe/trunk/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);

cfe/trunk/lib/Parse/ParseExprCXX.cpp

if (!HasScopeSpecifier &&
     if (Actions.ActOnCXXNestedNameSpecifierDecltype(SS, DS, CCLoc))
       SS.SetInvalid(SourceRange(DeclLoc, CCLoc));
 
     HasScopeSpecifier = true;
   }
 
   while (true) {
     if (HasScopeSpecifier) {
-      // C++ [basic.lookup.classref]p5:
-      //   If the qualified-id has the form
-      //
-      //       ::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.
-      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,
+                                        ObjectType.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;
       }
+
+      // C++ [basic.lookup.classref]p5:
+      //   If the qualified-id has the form
+      //
+      //       ::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.
+      ObjectType = nullptr;
     }
 
     // nested-name-specifier:
     //   nested-name-specifier 'template'[opt] simple-template-id '::'
 
     // Parse the optional 'template' keyword, then make sure we have
     // 'identifier <' after it.
     if (Tok.is(tok::kw_template)) {

cfe/trunk/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())

cfe/trunk/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)

cfe/trunk/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);

cfe/trunk/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)
+    };
+  }
+};
+
+

cfe/trunk/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
+  }
+};