Skip to content

Commit f1822ec

Browse files
ilya-biryukovilya-biryukov
committedDec 3, 2018
[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
1 parent 6ef089d commit f1822ec

File tree

7 files changed

+176
-55
lines changed

7 files changed

+176
-55
lines changed
 

Diff for: ‎clang/include/clang/Sema/Sema.h

+3-2
Original file line numberDiff line numberDiff line change
@@ -6065,7 +6065,8 @@ class Sema {
60656065
bool ForceCheck = false,
60666066
bool ForceUnprivileged = false);
60676067
void CheckLookupAccess(const LookupResult &R);
6068-
bool IsSimplyAccessible(NamedDecl *decl, DeclContext *Ctx);
6068+
bool IsSimplyAccessible(NamedDecl *Decl, CXXRecordDecl *NamingClass,
6069+
QualType BaseType);
60696070
bool isSpecialMemberAccessibleForDeletion(CXXMethodDecl *decl,
60706071
AccessSpecifier access,
60716072
QualType objectType);
@@ -10340,7 +10341,7 @@ class Sema {
1034010341
void CodeCompleteAssignmentRHS(Scope *S, Expr *LHS);
1034110342

1034210343
void CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS,
10343-
bool EnteringContext);
10344+
bool EnteringContext, QualType BaseType);
1034410345
void CodeCompleteUsing(Scope *S);
1034510346
void CodeCompleteUsingDirective(Scope *S);
1034610347
void CodeCompleteNamespaceDecl(Scope *S);

Diff for: ‎clang/lib/Parse/ParseExprCXX.cpp

+14-13
Original file line numberDiff line numberDiff line change
@@ -235,22 +235,11 @@ bool Parser::ParseOptionalCXXScopeSpecifier(CXXScopeSpec &SS,
235235

236236
while (true) {
237237
if (HasScopeSpecifier) {
238-
// C++ [basic.lookup.classref]p5:
239-
// If the qualified-id has the form
240-
//
241-
// ::class-name-or-namespace-name::...
242-
//
243-
// the class-name-or-namespace-name is looked up in global scope as a
244-
// class-name or namespace-name.
245-
//
246-
// To implement this, we clear out the object type as soon as we've
247-
// seen a leading '::' or part of a nested-name-specifier.
248-
ObjectType = nullptr;
249-
250238
if (Tok.is(tok::code_completion)) {
251239
// Code completion for a nested-name-specifier, where the code
252240
// completion token follows the '::'.
253-
Actions.CodeCompleteQualifiedId(getCurScope(), SS, EnteringContext);
241+
Actions.CodeCompleteQualifiedId(getCurScope(), SS, EnteringContext,
242+
ObjectType.get());
254243
// Include code completion token into the range of the scope otherwise
255244
// when we try to annotate the scope tokens the dangling code completion
256245
// token will cause assertion in
@@ -259,6 +248,18 @@ bool Parser::ParseOptionalCXXScopeSpecifier(CXXScopeSpec &SS,
259248
cutOffParsing();
260249
return true;
261250
}
251+
252+
// C++ [basic.lookup.classref]p5:
253+
// If the qualified-id has the form
254+
//
255+
// ::class-name-or-namespace-name::...
256+
//
257+
// the class-name-or-namespace-name is looked up in global scope as a
258+
// class-name or namespace-name.
259+
//
260+
// To implement this, we clear out the object type as soon as we've
261+
// seen a leading '::' or part of a nested-name-specifier.
262+
ObjectType = nullptr;
262263
}
263264

264265
// nested-name-specifier:

Diff for: ‎clang/lib/Sema/CodeCompleteConsumer.cpp

+3
Original file line numberDiff line numberDiff line change
@@ -555,6 +555,9 @@ void PrintingCodeCompleteConsumer::ProcessCodeCompleteResults(
555555
Tags.push_back("Hidden");
556556
if (Results[I].InBaseClass)
557557
Tags.push_back("InBase");
558+
if (Results[I].Availability ==
559+
CXAvailabilityKind::CXAvailability_NotAccessible)
560+
Tags.push_back("Inaccessible");
558561
if (!Tags.empty())
559562
OS << " (" << llvm::join(Tags, ",") << ")";
560563
}

Diff for: ‎clang/lib/Sema/SemaAccess.cpp

+20-9
Original file line numberDiff line numberDiff line change
@@ -1877,22 +1877,33 @@ void Sema::CheckLookupAccess(const LookupResult &R) {
18771877
/// specifiers into account, but no member access expressions and such.
18781878
///
18791879
/// \param Target the declaration to check if it can be accessed
1880-
/// \param Ctx the class/context from which to start the search
1880+
/// \param NamingClass the class in which the lookup was started.
1881+
/// \param BaseType type of the left side of member access expression.
1882+
/// \p BaseType and \p NamingClass are used for C++ access control.
1883+
/// Depending on the lookup case, they should be set to the following:
1884+
/// - lhs.target (member access without a qualifier):
1885+
/// \p BaseType and \p NamingClass are both the type of 'lhs'.
1886+
/// - lhs.X::target (member access with a qualifier):
1887+
/// BaseType is the type of 'lhs', NamingClass is 'X'
1888+
/// - X::target (qualified lookup without member access):
1889+
/// BaseType is null, NamingClass is 'X'.
1890+
/// - target (unqualified lookup).
1891+
/// BaseType is null, NamingClass is the parent class of 'target'.
18811892
/// \return true if the Target is accessible from the Class, false otherwise.
1882-
bool Sema::IsSimplyAccessible(NamedDecl *Target, DeclContext *Ctx) {
1883-
if (CXXRecordDecl *Class = dyn_cast<CXXRecordDecl>(Ctx)) {
1884-
if (!Target->isCXXClassMember())
1885-
return true;
1886-
1893+
bool Sema::IsSimplyAccessible(NamedDecl *Target, CXXRecordDecl *NamingClass,
1894+
QualType BaseType) {
1895+
// Perform the C++ accessibility checks first.
1896+
if (Target->isCXXClassMember() && NamingClass) {
1897+
if (!getLangOpts().CPlusPlus)
1898+
return false;
18871899
if (Target->getAccess() == AS_public)
18881900
return true;
1889-
QualType qType = Class->getTypeForDecl()->getCanonicalTypeInternal();
18901901
// The unprivileged access is AS_none as we don't know how the member was
18911902
// accessed, which is described by the access in DeclAccessPair.
18921903
// `IsAccessible` will examine the actual access of Target (i.e.
18931904
// Decl->getAccess()) when calculating the access.
1894-
AccessTarget Entity(Context, AccessedEntity::Member, Class,
1895-
DeclAccessPair::make(Target, AS_none), qType);
1905+
AccessTarget Entity(Context, AccessedEntity::Member, NamingClass,
1906+
DeclAccessPair::make(Target, AS_none), BaseType);
18961907
EffectiveContext EC(CurContext);
18971908
return ::IsAccessible(*this, EC, Entity) != ::AR_inaccessible;
18981909
}

Diff for: ‎clang/lib/Sema/SemaCodeComplete.cpp

+40-31
Original file line numberDiff line numberDiff line change
@@ -1278,38 +1278,53 @@ bool ResultBuilder::IsObjCIvar(const NamedDecl *ND) const {
12781278
}
12791279

12801280
namespace {
1281+
12811282
/// Visible declaration consumer that adds a code-completion result
12821283
/// for each visible declaration.
12831284
class CodeCompletionDeclConsumer : public VisibleDeclConsumer {
12841285
ResultBuilder &Results;
1285-
DeclContext *CurContext;
1286+
DeclContext *InitialLookupCtx;
1287+
// NamingClass and BaseType are used for access-checking. See
1288+
// Sema::IsSimplyAccessible for details.
1289+
CXXRecordDecl *NamingClass;
1290+
QualType BaseType;
12861291
std::vector<FixItHint> FixIts;
1287-
// This is set to the record where the search starts, if this is a record
1288-
// member completion.
1289-
RecordDecl *MemberCompletionRecord = nullptr;
12901292

12911293
public:
12921294
CodeCompletionDeclConsumer(
1293-
ResultBuilder &Results, DeclContext *CurContext,
1294-
std::vector<FixItHint> FixIts = std::vector<FixItHint>(),
1295-
RecordDecl *MemberCompletionRecord = nullptr)
1296-
: Results(Results), CurContext(CurContext), FixIts(std::move(FixIts)),
1297-
MemberCompletionRecord(MemberCompletionRecord) {}
1295+
ResultBuilder &Results, DeclContext *InitialLookupCtx,
1296+
QualType BaseType = QualType(),
1297+
std::vector<FixItHint> FixIts = std::vector<FixItHint>())
1298+
: Results(Results), InitialLookupCtx(InitialLookupCtx),
1299+
FixIts(std::move(FixIts)) {
1300+
NamingClass = llvm::dyn_cast<CXXRecordDecl>(InitialLookupCtx);
1301+
// If BaseType was not provided explicitly, emulate implicit 'this->'.
1302+
if (BaseType.isNull()) {
1303+
auto ThisType = Results.getSema().getCurrentThisType();
1304+
if (!ThisType.isNull()) {
1305+
assert(ThisType->isPointerType());
1306+
BaseType = ThisType->getPointeeType();
1307+
if (!NamingClass)
1308+
NamingClass = BaseType->getAsCXXRecordDecl();
1309+
}
1310+
}
1311+
this->BaseType = BaseType;
1312+
}
12981313

12991314
void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx,
13001315
bool InBaseClass) override {
1301-
bool Accessible = true;
1302-
if (Ctx) {
1303-
// Set the actual accessing context (i.e. naming class) to the record
1304-
// context where the search starts. When `InBaseClass` is true, `Ctx`
1305-
// will be the base class, which is not the actual naming class.
1306-
DeclContext *AccessingCtx =
1307-
MemberCompletionRecord ? MemberCompletionRecord : Ctx;
1308-
Accessible = Results.getSema().IsSimplyAccessible(ND, AccessingCtx);
1309-
}
1316+
// Naming class to use for access check. In most cases it was provided
1317+
// explicitly (e.g. member access (lhs.foo) or qualified lookup (X::)), for
1318+
// unqualified lookup we fallback to the \p Ctx in which we found the
1319+
// member.
1320+
auto *NamingClass = this->NamingClass;
1321+
if (!NamingClass)
1322+
NamingClass = llvm::dyn_cast_or_null<CXXRecordDecl>(Ctx);
1323+
bool Accessible =
1324+
Results.getSema().IsSimplyAccessible(ND, NamingClass, BaseType);
13101325
ResultBuilder::Result Result(ND, Results.getBasePriority(ND), nullptr,
13111326
false, Accessible, FixIts);
1312-
Results.AddResult(Result, CurContext, Hiding, InBaseClass);
1327+
Results.AddResult(Result, InitialLookupCtx, Hiding, InBaseClass);
13131328
}
13141329

13151330
void EnteredContext(DeclContext *Ctx) override {
@@ -3699,20 +3714,15 @@ void Sema::CodeCompleteOrdinaryName(Scope *S,
36993714
}
37003715

37013716
// If we are in a C++ non-static member function, check the qualifiers on
3702-
// the member function to filter/prioritize the results list and set the
3703-
// context to the record context so that accessibility check in base class
3704-
// works correctly.
3705-
RecordDecl *MemberCompletionRecord = nullptr;
3717+
// the member function to filter/prioritize the results list.
37063718
if (CXXMethodDecl *CurMethod = dyn_cast<CXXMethodDecl>(CurContext)) {
37073719
if (CurMethod->isInstance()) {
37083720
Results.setObjectTypeQualifiers(
37093721
Qualifiers::fromCVRMask(CurMethod->getTypeQualifiers()));
3710-
MemberCompletionRecord = CurMethod->getParent();
37113722
}
37123723
}
37133724

3714-
CodeCompletionDeclConsumer Consumer(Results, CurContext, /*FixIts=*/{},
3715-
MemberCompletionRecord);
3725+
CodeCompletionDeclConsumer Consumer(Results, CurContext);
37163726
LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
37173727
CodeCompleter->includeGlobals(),
37183728
CodeCompleter->loadExternal());
@@ -4152,8 +4162,7 @@ AddRecordMembersCompletionResults(Sema &SemaRef, ResultBuilder &Results,
41524162
std::vector<FixItHint> FixIts;
41534163
if (AccessOpFixIt)
41544164
FixIts.emplace_back(AccessOpFixIt.getValue());
4155-
CodeCompletionDeclConsumer Consumer(Results, SemaRef.CurContext,
4156-
std::move(FixIts), RD);
4165+
CodeCompletionDeclConsumer Consumer(Results, RD, BaseType, std::move(FixIts));
41574166
SemaRef.LookupVisibleDecls(RD, Sema::LookupMemberName, Consumer,
41584167
SemaRef.CodeCompleter->includeGlobals(),
41594168
/*IncludeDependentBases=*/true,
@@ -4283,7 +4292,7 @@ void Sema::CodeCompleteMemberReferenceExpr(Scope *S, Expr *Base,
42834292

42844293
// Add all ivars from this class and its superclasses.
42854294
if (Class) {
4286-
CodeCompletionDeclConsumer Consumer(Results, CurContext);
4295+
CodeCompletionDeclConsumer Consumer(Results, Class, BaseType);
42874296
Results.setFilter(&ResultBuilder::IsObjCIvar);
42884297
LookupVisibleDecls(
42894298
Class, LookupMemberName, Consumer, CodeCompleter->includeGlobals(),
@@ -4856,7 +4865,7 @@ void Sema::CodeCompleteAssignmentRHS(Scope *S, Expr *LHS) {
48564865
}
48574866

48584867
void Sema::CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS,
4859-
bool EnteringContext) {
4868+
bool EnteringContext, QualType BaseType) {
48604869
if (SS.isEmpty() || !CodeCompleter)
48614870
return;
48624871

@@ -4903,7 +4912,7 @@ void Sema::CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS,
49034912

49044913
if (CodeCompleter->includeNamespaceLevelDecls() ||
49054914
(!Ctx->isNamespace() && !Ctx->isTranslationUnit())) {
4906-
CodeCompletionDeclConsumer Consumer(Results, CurContext);
4915+
CodeCompletionDeclConsumer Consumer(Results, Ctx, BaseType);
49074916
LookupVisibleDecls(Ctx, LookupOrdinaryName, Consumer,
49084917
/*IncludeGlobalScope=*/true,
49094918
/*IncludeDependentBases=*/true,

Diff for: ‎clang/test/CodeCompletion/accessibility-crash.cpp

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
class X {
2+
public:
3+
int pub;
4+
protected:
5+
int prot;
6+
private:
7+
int priv;
8+
};
9+
10+
class Y : public X {
11+
int test() {
12+
[]() {
13+
14+
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:13:1 %s -o - \
15+
// RUN: | FileCheck %s
16+
// CHECK: priv (InBase,Inaccessible)
17+
// CHECK: prot (InBase)
18+
// CHECK: pub (InBase)
19+
};
20+
}
21+
};
22+
23+

Diff for: ‎clang/test/CodeCompletion/accessibility.cpp

+73
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
class X {
2+
public:
3+
int pub;
4+
protected:
5+
int prot;
6+
private:
7+
int priv;
8+
};
9+
10+
class Unrelated {
11+
public:
12+
static int pub;
13+
protected:
14+
static int prot;
15+
private:
16+
static int priv;
17+
};
18+
19+
class Y : public X {
20+
int test() {
21+
this->pub = 10;
22+
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:11 %s -o - \
23+
// RUN: | FileCheck -check-prefix=THIS %s
24+
// THIS: priv (InBase,Inaccessible)
25+
// THIS: prot (InBase)
26+
// THIS: pub (InBase)
27+
//
28+
// Also check implicit 'this->', i.e. complete at the start of the line.
29+
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:1 %s -o - \
30+
// RUN: | FileCheck -check-prefix=THIS %s
31+
32+
X().pub + 10;
33+
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:32:9 %s -o - \
34+
// RUN: | FileCheck -check-prefix=X-OBJ %s
35+
// X-OBJ: priv (Inaccessible)
36+
// X-OBJ: prot (Inaccessible)
37+
// X-OBJ: pub : [#int#]pub
38+
39+
Y().pub + 10;
40+
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:39:9 %s -o - \
41+
// RUN: | FileCheck -check-prefix=Y-OBJ %s
42+
// Y-OBJ: priv (InBase,Inaccessible)
43+
// Y-OBJ: prot (InBase)
44+
// Y-OBJ: pub (InBase)
45+
46+
this->X::pub = 10;
47+
X::pub = 10;
48+
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:46:14 %s -o - \
49+
// RUN: | FileCheck -check-prefix=THIS-BASE %s
50+
//
51+
// THIS-BASE: priv (Inaccessible)
52+
// THIS-BASE: prot : [#int#]prot
53+
// THIS-BASE: pub : [#int#]pub
54+
//
55+
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:47:8 %s -o - \
56+
// RUN: | FileCheck -check-prefix=THIS-BASE %s
57+
58+
59+
this->Unrelated::pub = 10; // a check we don't crash in this cases.
60+
Y().Unrelated::pub = 10; // a check we don't crash in this cases.
61+
Unrelated::pub = 10;
62+
// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:59:22 %s -o - \
63+
// RUN: | FileCheck -check-prefix=UNRELATED %s
64+
// UNRELATED: priv (Inaccessible)
65+
// UNRELATED: prot (Inaccessible)
66+
// UNRELATED: pub : [#int#]pub
67+
//
68+
// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:60:20 %s -o - \
69+
// RUN: | FileCheck -check-prefix=UNRELATED %s
70+
// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:61:16 %s -o - \
71+
// RUN: | FileCheck -check-prefix=UNRELATED %s
72+
}
73+
};

0 commit comments

Comments
 (0)
Please sign in to comment.