I actually tried to pass the correct derived scope to CodeCompletionDeclConsumer on construction, it was not a lot of code and I think this should fix the problem you're describing. I'll send a patch.
On Tue, Jul 31, 2018 at 11:33 AM Eric Liu <ioe...@google.com> wrote: > 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 >> > -- Regards, Ilya Biryukov
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
