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:13:00 PDT 2018
(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/85e38f8f/attachment-0001.html>
More information about the cfe-commits
mailing list