Thanks a lot for looking into this! You are right, the change in SemaAccess seemed to have done the job to fix the protected member bug in specific. I think I made the change in SemaCodeComplete before SemaAccess and didn't realized the previous one was unnecessary to fix the tests. I think the accessing context/naming class is still wrong though. Basically, we want the naming class to be the derived class instead of the base class if the access is from a derived class. Without this, accessibility check is relaxed, but it probably doesn't have very negative impact on code completion experience, so I think it's okay to revert the change in SemaCodeComplete. Thanks again!
On Mon, Jul 30, 2018 at 5:22 PM Ilya Biryukov <ibiryu...@google.com> wrote: > Prtially reverted the commit in r338255 to fix a very frequent crash. > The code seemed obviously wrong there (calling accessibility checking, > passing a possibly unrelated class) and the tests didn't break after > reverting it. > > Let me know if I missed anything. > > On Thu, Jul 19, 2018 at 3:37 PM Eric Liu via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: ioeric >> Date: Thu Jul 19 06:32:00 2018 >> New Revision: 337453 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=337453&view=rev >> Log: >> [CodeComplete] Fix accessibilty of protected members from base class. >> >> Summary: >> Currently, protected members from base classes are marked as >> inaccessible when completing in derived class. This patch fixes the >> problem by >> setting the naming class correctly when looking up results in base class >> according to [11.2.p5]. >> >> Reviewers: aaron.ballman, sammccall, rsmith >> >> Reviewed By: aaron.ballman >> >> Subscribers: cfe-commits >> >> Differential Revision: https://reviews.llvm.org/D49421 >> >> Modified: >> cfe/trunk/lib/Sema/SemaAccess.cpp >> cfe/trunk/lib/Sema/SemaCodeComplete.cpp >> cfe/trunk/test/Index/complete-access-checks.cpp >> >> Modified: cfe/trunk/lib/Sema/SemaAccess.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAccess.cpp?rev=337453&r1=337452&r2=337453&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaAccess.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaAccess.cpp Thu Jul 19 06:32:00 2018 >> @@ -11,6 +11,7 @@ >> // >> >> >> //===----------------------------------------------------------------------===// >> >> +#include "clang/Basic/Specifiers.h" >> #include "clang/Sema/SemaInternal.h" >> #include "clang/AST/ASTContext.h" >> #include "clang/AST/CXXInheritance.h" >> @@ -1856,29 +1857,31 @@ void Sema::CheckLookupAccess(const Looku >> } >> } >> >> -/// Checks access to Decl from the given class. The check will take >> access >> +/// Checks access to Target from the given class. The check will take >> access >> /// specifiers into account, but no member access expressions and such. >> /// >> -/// \param Decl the declaration to check if it can be accessed >> +/// \param Target the declaration to check if it can be accessed >> /// \param Ctx the class/context from which to start the search >> -/// \return true if the Decl is accessible from the Class, false >> otherwise. >> -bool Sema::IsSimplyAccessible(NamedDecl *Decl, DeclContext *Ctx) { >> +/// \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 (!Decl->isCXXClassMember()) >> + if (!Target->isCXXClassMember()) >> return true; >> >> + 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(Decl, Decl->getAccess()), >> - qType); >> - if (Entity.getAccess() == AS_public) >> - return true; >> - >> + DeclAccessPair::make(Target, AS_none), qType); >> EffectiveContext EC(CurContext); >> return ::IsAccessible(*this, EC, Entity) != ::AR_inaccessible; >> } >> - >> - if (ObjCIvarDecl *Ivar = dyn_cast<ObjCIvarDecl>(Decl)) { >> + >> + if (ObjCIvarDecl *Ivar = dyn_cast<ObjCIvarDecl>(Target)) { >> // @public and @package ivars are always accessible. >> if (Ivar->getCanonicalAccessControl() == ObjCIvarDecl::Public || >> Ivar->getCanonicalAccessControl() == ObjCIvarDecl::Package) >> >> Modified: cfe/trunk/lib/Sema/SemaCodeComplete.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCodeComplete.cpp?rev=337453&r1=337452&r2=337453&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaCodeComplete.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp Thu Jul 19 06:32:00 2018 >> @@ -1303,8 +1303,33 @@ namespace { >> void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx, >> bool InBaseClass) override { >> bool Accessible = true; >> - if (Ctx) >> - Accessible = Results.getSema().IsSimplyAccessible(ND, Ctx); >> + if (Ctx) { >> + DeclContext *AccessingCtx = Ctx; >> + // If ND comes from a base class, set the naming class back to >> the >> + // derived class if the search starts from the derived class >> (i.e. >> + // InBaseClass is true). >> + // >> + // Example: >> + // class B { protected: int X; } >> + // class D : public B { void f(); } >> + // void D::f() { this->^; } >> + // The completion after "this->" will have `InBaseClass` set to >> true and >> + // `Ctx` set to "B", when looking up in `B`. We need to set the >> actual >> + // accessing context (i.e. naming class) to "D" so that access >> can be >> + // calculated correctly. >> + if (InBaseClass && isa<CXXRecordDecl>(Ctx)) { >> + CXXRecordDecl *RC = nullptr; >> + // Get the enclosing record. >> + for (DeclContext *DC = CurContext; !DC->isFileContext(); >> + DC = DC->getParent()) { >> + if ((RC = dyn_cast<CXXRecordDecl>(DC))) >> + break; >> + } >> + if (RC) >> + AccessingCtx = RC; >> + } >> + Accessible = Results.getSema().IsSimplyAccessible(ND, >> AccessingCtx); >> + } >> >> ResultBuilder::Result Result(ND, Results.getBasePriority(ND), >> nullptr, >> false, Accessible, FixIts); >> >> Modified: cfe/trunk/test/Index/complete-access-checks.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/complete-access-checks.cpp?rev=337453&r1=337452&r2=337453&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/Index/complete-access-checks.cpp (original) >> +++ cfe/trunk/test/Index/complete-access-checks.cpp Thu Jul 19 06:32:00 >> 2018 >> @@ -36,10 +36,10 @@ void Y::doSomething() { >> >> // CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{TypedText >> doSomething}{LeftParen (}{RightParen )} (34) >> // CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative >> X::}{TypedText func1}{LeftParen (}{RightParen )} (36) >> -// CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative >> X::}{TypedText func2}{LeftParen (}{RightParen )} (36) (inaccessible) >> +// CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative >> X::}{TypedText func2}{LeftParen (}{RightParen )} (36) >> // CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative >> X::}{TypedText func3}{LeftParen (}{RightParen )} (36) (inaccessible) >> // CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative >> X::}{TypedText member1} (37) >> -// CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative >> X::}{TypedText member2} (37) (inaccessible) >> +// CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative >> X::}{TypedText member2} (37) >> // CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative >> X::}{TypedText member3} (37) (inaccessible) >> // CHECK-SUPER-ACCESS: CXXMethod:{ResultType Y &}{TypedText >> operator=}{LeftParen (}{Placeholder const Y &}{RightParen )} (79) >> // CHECK-SUPER-ACCESS: CXXMethod:{ResultType X &}{Text X::}{TypedText >> operator=}{LeftParen (}{Placeholder const X &}{RightParen )} (81) >> @@ -87,3 +87,26 @@ void f(P x, Q y) { >> // CHECK-USING-ACCESSIBLE: ClassDecl:{TypedText Q}{Text ::} (75) >> // CHECK-USING-ACCESSIBLE: CXXDestructor:{ResultType void}{Informative >> P::}{TypedText ~P}{LeftParen (}{RightParen )} (81) >> // CHECK-USING-ACCESSIBLE: CXXDestructor:{ResultType void}{TypedText >> ~Q}{LeftParen (}{RightParen )} (79) >> + >> +class B { >> +protected: >> + int member; >> +}; >> + >> +class C : private B {}; >> + >> + >> +class D : public C { >> + public: >> + void f(::B *b); >> +}; >> + >> +void D::f(::B *that) { >> + // RUN: c-index-test -code-completion-at=%s:106:9 %s | FileCheck >> -check-prefix=CHECK-PRIVATE-SUPER-THIS %s >> + this->; >> +// CHECK-PRIVATE-SUPER-THIS: FieldDecl:{ResultType int}{Informative >> B::}{TypedText member} (37) (inaccessible) >> + >> + // RUN: c-index-test -code-completion-at=%s:110:9 %s | FileCheck >> -check-prefix=CHECK-PRIVATE-SUPER-THAT %s >> + that->; >> +// CHECK-PRIVATE-SUPER-THAT: FieldDecl:{ResultType int}{TypedText >> member} (35) (inaccessible) >> +} >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > > > -- > Regards, > Ilya Biryukov >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits