r337453 - [CodeComplete] Fix accessibilty of protected members from base class.
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 28 05:48:26 PDT 2018
PTAL: https://reviews.llvm.org/D52647
On Fri, Sep 28, 2018 at 2:13 PM Eric Liu <ioeric at 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 <ibiryukov at 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 <ioeric at 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 <ibiryukov at 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 at 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 at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>
>>>>
>>>> --
>>>> Regards,
>>>> Ilya Biryukov
>>>>
>>>
>>
>> --
>> Regards,
>> Ilya Biryukov
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180928/83982429/attachment-0001.html>
More information about the cfe-commits
mailing list