PTAL: https://reviews.llvm.org/D52647
On Fri, Sep 28, 2018 at 2:13 PM Eric Liu <ioe...@google.com> wrote: > (Oops, forgot there was this thread. Pasting my response to r338255 here). > > > The code seemed obviously wrong there (calling accessibility checking, > passing a possibly unrelated class) and the tests didn't break after > reverting it. > It turns out that this was due to my lacking LIT test skill. LIT does > partial string match... so "something" would match both "something" and > "something (inaccessible)" XD > > 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. > Any luck on this? > > On Tue, Jul 31, 2018 at 12:24 PM Ilya Biryukov <ibiryu...@google.com> > wrote: > >> 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
