From f1822ec431cded5e50b69152346794d79976a8b2 Mon Sep 17 00:00:00 2001 From: Ilya Biryukov Date: Mon, 3 Dec 2018 13:29:17 +0000 Subject: [PATCH] [CodeComplete] Cleanup access checking in code completion Summary: Also fixes a crash (see the added 'accessibility-crash.cpp' test). Reviewers: ioeric, kadircet Reviewed By: kadircet Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D55124 llvm-svn: 348135 --- clang/include/clang/Sema/Sema.h | 5 +- clang/lib/Parse/ParseExprCXX.cpp | 27 +++++---- clang/lib/Sema/CodeCompleteConsumer.cpp | 3 + clang/lib/Sema/SemaAccess.cpp | 29 ++++++--- clang/lib/Sema/SemaCodeComplete.cpp | 71 ++++++++++++---------- clang/test/CodeCompletion/accessibility-crash.cpp | 23 +++++++ clang/test/CodeCompletion/accessibility.cpp | 73 +++++++++++++++++++++++ 7 files changed, 176 insertions(+), 55 deletions(-) create mode 100644 clang/test/CodeCompletion/accessibility-crash.cpp create mode 100644 clang/test/CodeCompletion/accessibility.cpp diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index f9e9cde..4712a11 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -6065,7 +6065,8 @@ public: 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); @@ -10340,7 +10341,7 @@ public: 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); diff --git a/clang/lib/Parse/ParseExprCXX.cpp b/clang/lib/Parse/ParseExprCXX.cpp index 43896e3..359bcf9 100644 --- a/clang/lib/Parse/ParseExprCXX.cpp +++ b/clang/lib/Parse/ParseExprCXX.cpp @@ -235,22 +235,11 @@ bool Parser::ParseOptionalCXXScopeSpecifier(CXXScopeSpec &SS, 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 @@ -259,6 +248,18 @@ bool Parser::ParseOptionalCXXScopeSpecifier(CXXScopeSpec &SS, 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: diff --git a/clang/lib/Sema/CodeCompleteConsumer.cpp b/clang/lib/Sema/CodeCompleteConsumer.cpp index f6a959c..40db849 100644 --- a/clang/lib/Sema/CodeCompleteConsumer.cpp +++ b/clang/lib/Sema/CodeCompleteConsumer.cpp @@ -555,6 +555,9 @@ void PrintingCodeCompleteConsumer::ProcessCodeCompleteResults( 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, ",") << ")"; } diff --git a/clang/lib/Sema/SemaAccess.cpp b/clang/lib/Sema/SemaAccess.cpp index cf33231..0432e70 100644 --- a/clang/lib/Sema/SemaAccess.cpp +++ b/clang/lib/Sema/SemaAccess.cpp @@ -1877,22 +1877,33 @@ void Sema::CheckLookupAccess(const LookupResult &R) { /// 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(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; } diff --git a/clang/lib/Sema/SemaCodeComplete.cpp b/clang/lib/Sema/SemaCodeComplete.cpp index aeb7aee..cb66a01 100644 --- a/clang/lib/Sema/SemaCodeComplete.cpp +++ b/clang/lib/Sema/SemaCodeComplete.cpp @@ -1278,38 +1278,53 @@ bool ResultBuilder::IsObjCIvar(const NamedDecl *ND) const { } 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 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 FixIts = std::vector(), - RecordDecl *MemberCompletionRecord = nullptr) - : Results(Results), CurContext(CurContext), FixIts(std::move(FixIts)), - MemberCompletionRecord(MemberCompletionRecord) {} + ResultBuilder &Results, DeclContext *InitialLookupCtx, + QualType BaseType = QualType(), + std::vector FixIts = std::vector()) + : Results(Results), InitialLookupCtx(InitialLookupCtx), + FixIts(std::move(FixIts)) { + NamingClass = llvm::dyn_cast(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(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 { @@ -3699,20 +3714,15 @@ void Sema::CodeCompleteOrdinaryName(Scope *S, } // 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(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()); @@ -4152,8 +4162,7 @@ AddRecordMembersCompletionResults(Sema &SemaRef, ResultBuilder &Results, std::vector 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, @@ -4283,7 +4292,7 @@ void Sema::CodeCompleteMemberReferenceExpr(Scope *S, Expr *Base, // 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(), @@ -4856,7 +4865,7 @@ void Sema::CodeCompleteAssignmentRHS(Scope *S, Expr *LHS) { } void Sema::CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS, - bool EnteringContext) { + bool EnteringContext, QualType BaseType) { if (SS.isEmpty() || !CodeCompleter) return; @@ -4903,7 +4912,7 @@ void Sema::CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS, if (CodeCompleter->includeNamespaceLevelDecls() || (!Ctx->isNamespace() && !Ctx->isTranslationUnit())) { - CodeCompletionDeclConsumer Consumer(Results, CurContext); + CodeCompletionDeclConsumer Consumer(Results, Ctx, BaseType); LookupVisibleDecls(Ctx, LookupOrdinaryName, Consumer, /*IncludeGlobalScope=*/true, /*IncludeDependentBases=*/true, diff --git a/clang/test/CodeCompletion/accessibility-crash.cpp b/clang/test/CodeCompletion/accessibility-crash.cpp new file mode 100644 index 0000000..b54f7ce --- /dev/null +++ b/clang/test/CodeCompletion/accessibility-crash.cpp @@ -0,0 +1,23 @@ +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) + }; + } +}; + + diff --git a/clang/test/CodeCompletion/accessibility.cpp b/clang/test/CodeCompletion/accessibility.cpp new file mode 100644 index 0000000..754d822 --- /dev/null +++ b/clang/test/CodeCompletion/accessibility.cpp @@ -0,0 +1,73 @@ +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 + } +}; -- 2.7.4