r337453 - [CodeComplete] Fix accessibilty of protected members from base class.
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 31 03:24:11 PDT 2018
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/20180731/dcdee68d/attachment-0001.html>
More information about the cfe-commits
mailing list