r348135 - [CodeComplete] Cleanup access checking in code completion
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 3 05:29:18 PST 2018
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
+ }
+};
More information about the cfe-commits
mailing list