r337453 - [CodeComplete] Fix accessibilty of protected members from base class.
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 31 02:33:22 PDT 2018
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180731/b7a5985a/attachment-0001.html>
More information about the cfe-commits
mailing list