Author: ibiryukov Date: Mon Dec 3 05:29:17 2018 New Revision: 348135 URL: http://llvm.org/viewvc/llvm-project?rev=348135&view=rev Log: [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 Added: cfe/trunk/test/CodeCompletion/accessibility-crash.cpp cfe/trunk/test/CodeCompletion/accessibility.cpp Modified: cfe/trunk/include/clang/Sema/Sema.h cfe/trunk/lib/Parse/ParseExprCXX.cpp cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp cfe/trunk/lib/Sema/SemaAccess.cpp cfe/trunk/lib/Sema/SemaCodeComplete.cpp Modified: cfe/trunk/include/clang/Sema/Sema.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=348135&r1=348134&r2=348135&view=diff ============================================================================== --- cfe/trunk/include/clang/Sema/Sema.h (original) +++ cfe/trunk/include/clang/Sema/Sema.h Mon Dec 3 05:29:17 2018 @@ -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); Modified: cfe/trunk/lib/Parse/ParseExprCXX.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseExprCXX.cpp?rev=348135&r1=348134&r2=348135&view=diff ============================================================================== --- cfe/trunk/lib/Parse/ParseExprCXX.cpp (original) +++ cfe/trunk/lib/Parse/ParseExprCXX.cpp Mon Dec 3 05:29:17 2018 @@ -235,22 +235,11 @@ bool Parser::ParseOptionalCXXScopeSpecif 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::ParseOptionalCXXScopeSpecif 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: Modified: cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp?rev=348135&r1=348134&r2=348135&view=diff ============================================================================== --- cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp (original) +++ cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp Mon Dec 3 05:29:17 2018 @@ -555,6 +555,9 @@ void PrintingCodeCompleteConsumer::Proce 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, ",") << ")"; } Modified: cfe/trunk/lib/Sema/SemaAccess.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAccess.cpp?rev=348135&r1=348134&r2=348135&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaAccess.cpp (original) +++ cfe/trunk/lib/Sema/SemaAccess.cpp Mon Dec 3 05:29:17 2018 @@ -1877,22 +1877,33 @@ void Sema::CheckLookupAccess(const Looku /// 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; } Modified: cfe/trunk/lib/Sema/SemaCodeComplete.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCodeComplete.cpp?rev=348135&r1=348134&r2=348135&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaCodeComplete.cpp (original) +++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp Mon Dec 3 05:29:17 2018 @@ -1278,38 +1278,53 @@ bool ResultBuilder::IsObjCIvar(const Nam } 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 { @@ -3699,20 +3714,15 @@ void Sema::CodeCompleteOrdinaryName(Scop } // 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()); @@ -4152,8 +4162,7 @@ AddRecordMembersCompletionResults(Sema & 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, @@ -4283,7 +4292,7 @@ void Sema::CodeCompleteMemberReferenceEx // 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(Sco } 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 if (CodeCompleter->includeNamespaceLevelDecls() || (!Ctx->isNamespace() && !Ctx->isTranslationUnit())) { - CodeCompletionDeclConsumer Consumer(Results, CurContext); + CodeCompletionDeclConsumer Consumer(Results, Ctx, BaseType); LookupVisibleDecls(Ctx, LookupOrdinaryName, Consumer, /*IncludeGlobalScope=*/true, /*IncludeDependentBases=*/true, Added: cfe/trunk/test/CodeCompletion/accessibility-crash.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeCompletion/accessibility-crash.cpp?rev=348135&view=auto ============================================================================== --- cfe/trunk/test/CodeCompletion/accessibility-crash.cpp (added) +++ cfe/trunk/test/CodeCompletion/accessibility-crash.cpp Mon Dec 3 05:29:17 2018 @@ -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) + }; + } +}; + + Added: cfe/trunk/test/CodeCompletion/accessibility.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeCompletion/accessibility.cpp?rev=348135&view=auto ============================================================================== --- cfe/trunk/test/CodeCompletion/accessibility.cpp (added) +++ cfe/trunk/test/CodeCompletion/accessibility.cpp Mon Dec 3 05:29:17 2018 @@ -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 + } +}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
