Re: r350768 - [ObjC] Allow the use of implemented unavailable methods from within
For the archives, here's said patch: https://reviews.llvm.org/D56816 (Thanks!) On Wed, Jan 16, 2019 at 5:26 PM Alex L wrote: > Hi, > > We are planning to fix this issue by not checking if the method is > defined. I will post a patch this week. > > Cheers, > Alex > > On Fri, 11 Jan 2019 at 10:26, Alex L wrote: > >> Thanks, we might have similar cases in our code base as well. We'll see >> if we can fix that too. >> >> On Fri, 11 Jan 2019 at 06:13, Nico Weber wrote: >> >>> Here's some user feedback on this new feature. >>> >>> It looks like the warning is only suppressed if `init` has a definition >>> in the @interface block. In the 4 cases where we saw this warning fire >>> after r349841, it still fires after this change because in all 4 cases a >>> class marked init as unavailable in the @interface but didn't add a >>> definition of it in the classes @implementation (instead relying on calling >>> the superclass's (NSObject) init). >>> >>> It's pretty easy to just add >>> >>> - (instancetype)init { return [super init]; } >>> >>> to these 4 classes, but: >>> a) it doesn't Just Work >>> b) the diagnostic doesn't make it clear that adding a definition of init >>> will suppress the warning. >>> >>> Up to you to decide what (if anything) to do with this feedback :-) >>> >>> (https://bugs.chromium.org/p/chromium/issues/detail?id=917351#c17 has a >>> full reduced code snippet.) >>> >>> On Wed, Jan 9, 2019 at 5:35 PM Alex Lorenz via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> Author: arphaman Date: Wed Jan 9 14:31:37 2019 New Revision: 350768 URL: http://llvm.org/viewvc/llvm-project?rev=350768=rev Log: [ObjC] Allow the use of implemented unavailable methods from within the @implementation context In Objective-C, it's common for some frameworks to mark some methods like init as unavailable in the @interface to prohibit their usage. However, these frameworks then often implemented said method and refer to it in another method that acts as a factory for that object. The recent change to how messages to self are type checked in clang (r349841) introduced a regression which started to prohibit this pattern with an X is unavailable error. This commit addresses the aforementioned regression. rdar://47134898 Differential Revision: https://reviews.llvm.org/D56469 Added: cfe/trunk/test/SemaObjC/call-unavailable-init-in-self.m Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=350768=350767=350768=diff == --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original) +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Jan 9 14:31:37 2019 @@ -7269,9 +7269,10 @@ ShouldDiagnoseAvailabilityOfDecl(Sema /// whether we should emit a diagnostic for \c K and \c DeclVersion in /// the context of \c Ctx. For example, we should emit an unavailable diagnostic /// in a deprecated context, but not the other way around. -static bool ShouldDiagnoseAvailabilityInContext(Sema , AvailabilityResult K, -VersionTuple DeclVersion, -Decl *Ctx) { +static bool +ShouldDiagnoseAvailabilityInContext(Sema , AvailabilityResult K, +VersionTuple DeclVersion, Decl *Ctx, +const NamedDecl *OffendingDecl) { assert(K != AR_Available && "Expected an unavailable declaration here!"); // Checks if we should emit the availability diagnostic in the context of C. @@ -7280,9 +7281,22 @@ static bool ShouldDiagnoseAvailabilityIn if (const AvailabilityAttr *AA = getAttrForPlatform(S.Context, C)) if (AA->getIntroduced() >= DeclVersion) return true; -} else if (K == AR_Deprecated) +} else if (K == AR_Deprecated) { if (C->isDeprecated()) return true; +} else if (K == AR_Unavailable) { + // It is perfectly fine to refer to an 'unavailable' Objective-C method + // when it's actually defined and is referenced from within the + // @implementation itself. In this context, we interpret unavailable as a + // form of access control. + if (const auto *MD = dyn_cast(OffendingDecl)) { +if (const auto *Impl = dyn_cast(C)) { + if (MD->getClassInterface() == Impl->getClassInterface() && + MD->isDefined()) +return true; +} + } +} if (C->isUnavailable())
Re: r350768 - [ObjC] Allow the use of implemented unavailable methods from within
Hi, We are planning to fix this issue by not checking if the method is defined. I will post a patch this week. Cheers, Alex On Fri, 11 Jan 2019 at 10:26, Alex L wrote: > Thanks, we might have similar cases in our code base as well. We'll see if > we can fix that too. > > On Fri, 11 Jan 2019 at 06:13, Nico Weber wrote: > >> Here's some user feedback on this new feature. >> >> It looks like the warning is only suppressed if `init` has a definition >> in the @interface block. In the 4 cases where we saw this warning fire >> after r349841, it still fires after this change because in all 4 cases a >> class marked init as unavailable in the @interface but didn't add a >> definition of it in the classes @implementation (instead relying on calling >> the superclass's (NSObject) init). >> >> It's pretty easy to just add >> >> - (instancetype)init { return [super init]; } >> >> to these 4 classes, but: >> a) it doesn't Just Work >> b) the diagnostic doesn't make it clear that adding a definition of init >> will suppress the warning. >> >> Up to you to decide what (if anything) to do with this feedback :-) >> >> (https://bugs.chromium.org/p/chromium/issues/detail?id=917351#c17 has a >> full reduced code snippet.) >> >> On Wed, Jan 9, 2019 at 5:35 PM Alex Lorenz via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Author: arphaman >>> Date: Wed Jan 9 14:31:37 2019 >>> New Revision: 350768 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=350768=rev >>> Log: >>> [ObjC] Allow the use of implemented unavailable methods from within >>> the @implementation context >>> >>> In Objective-C, it's common for some frameworks to mark some methods >>> like init >>> as unavailable in the @interface to prohibit their usage. However, these >>> frameworks then often implemented said method and refer to it in another >>> method >>> that acts as a factory for that object. The recent change to how >>> messages to >>> self are type checked in clang (r349841) introduced a regression which >>> started >>> to prohibit this pattern with an X is unavailable error. This commit >>> addresses >>> the aforementioned regression. >>> >>> rdar://47134898 >>> >>> Differential Revision: https://reviews.llvm.org/D56469 >>> >>> Added: >>> cfe/trunk/test/SemaObjC/call-unavailable-init-in-self.m >>> Modified: >>> cfe/trunk/lib/Sema/SemaDeclAttr.cpp >>> >>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=350768=350767=350768=diff >>> >>> == >>> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original) >>> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Jan 9 14:31:37 2019 >>> @@ -7269,9 +7269,10 @@ ShouldDiagnoseAvailabilityOfDecl(Sema >>> /// whether we should emit a diagnostic for \c K and \c DeclVersion in >>> /// the context of \c Ctx. For example, we should emit an unavailable >>> diagnostic >>> /// in a deprecated context, but not the other way around. >>> -static bool ShouldDiagnoseAvailabilityInContext(Sema , >>> AvailabilityResult K, >>> -VersionTuple >>> DeclVersion, >>> -Decl *Ctx) { >>> +static bool >>> +ShouldDiagnoseAvailabilityInContext(Sema , AvailabilityResult K, >>> +VersionTuple DeclVersion, Decl *Ctx, >>> +const NamedDecl *OffendingDecl) { >>>assert(K != AR_Available && "Expected an unavailable declaration >>> here!"); >>> >>>// Checks if we should emit the availability diagnostic in the >>> context of C. >>> @@ -7280,9 +7281,22 @@ static bool ShouldDiagnoseAvailabilityIn >>>if (const AvailabilityAttr *AA = getAttrForPlatform(S.Context, C)) >>> if (AA->getIntroduced() >= DeclVersion) >>>return true; >>> -} else if (K == AR_Deprecated) >>> +} else if (K == AR_Deprecated) { >>>if (C->isDeprecated()) >>> return true; >>> +} else if (K == AR_Unavailable) { >>> + // It is perfectly fine to refer to an 'unavailable' Objective-C >>> method >>> + // when it's actually defined and is referenced from within the >>> + // @implementation itself. In this context, we interpret >>> unavailable as a >>> + // form of access control. >>> + if (const auto *MD = dyn_cast(OffendingDecl)) { >>> +if (const auto *Impl = dyn_cast(C)) { >>> + if (MD->getClassInterface() == Impl->getClassInterface() && >>> + MD->isDefined()) >>> +return true; >>> +} >>> + } >>> +} >>> >>> if (C->isUnavailable()) >>>return true; >>> @@ -7471,7 +7485,8 @@ static void DoEmitAvailabilityWarning(Se >>>if (const AvailabilityAttr *AA = getAttrForPlatform(S.Context, >>> OffendingDecl)) >>> DeclVersion = AA->getIntroduced(); >>> >>> - if
Re: r350768 - [ObjC] Allow the use of implemented unavailable methods from within
Thanks, we might have similar cases in our code base as well. We'll see if we can fix that too. On Fri, 11 Jan 2019 at 06:13, Nico Weber wrote: > Here's some user feedback on this new feature. > > It looks like the warning is only suppressed if `init` has a definition in > the @interface block. In the 4 cases where we saw this warning fire after > r349841, it still fires after this change because in all 4 cases a class > marked init as unavailable in the @interface but didn't add a definition of > it in the classes @implementation (instead relying on calling the > superclass's (NSObject) init). > > It's pretty easy to just add > > - (instancetype)init { return [super init]; } > > to these 4 classes, but: > a) it doesn't Just Work > b) the diagnostic doesn't make it clear that adding a definition of init > will suppress the warning. > > Up to you to decide what (if anything) to do with this feedback :-) > > (https://bugs.chromium.org/p/chromium/issues/detail?id=917351#c17 has a > full reduced code snippet.) > > On Wed, Jan 9, 2019 at 5:35 PM Alex Lorenz via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: arphaman >> Date: Wed Jan 9 14:31:37 2019 >> New Revision: 350768 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=350768=rev >> Log: >> [ObjC] Allow the use of implemented unavailable methods from within >> the @implementation context >> >> In Objective-C, it's common for some frameworks to mark some methods like >> init >> as unavailable in the @interface to prohibit their usage. However, these >> frameworks then often implemented said method and refer to it in another >> method >> that acts as a factory for that object. The recent change to how messages >> to >> self are type checked in clang (r349841) introduced a regression which >> started >> to prohibit this pattern with an X is unavailable error. This commit >> addresses >> the aforementioned regression. >> >> rdar://47134898 >> >> Differential Revision: https://reviews.llvm.org/D56469 >> >> Added: >> cfe/trunk/test/SemaObjC/call-unavailable-init-in-self.m >> Modified: >> cfe/trunk/lib/Sema/SemaDeclAttr.cpp >> >> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=350768=350767=350768=diff >> >> == >> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Jan 9 14:31:37 2019 >> @@ -7269,9 +7269,10 @@ ShouldDiagnoseAvailabilityOfDecl(Sema >> /// whether we should emit a diagnostic for \c K and \c DeclVersion in >> /// the context of \c Ctx. For example, we should emit an unavailable >> diagnostic >> /// in a deprecated context, but not the other way around. >> -static bool ShouldDiagnoseAvailabilityInContext(Sema , >> AvailabilityResult K, >> -VersionTuple DeclVersion, >> -Decl *Ctx) { >> +static bool >> +ShouldDiagnoseAvailabilityInContext(Sema , AvailabilityResult K, >> +VersionTuple DeclVersion, Decl *Ctx, >> +const NamedDecl *OffendingDecl) { >>assert(K != AR_Available && "Expected an unavailable declaration >> here!"); >> >>// Checks if we should emit the availability diagnostic in the context >> of C. >> @@ -7280,9 +7281,22 @@ static bool ShouldDiagnoseAvailabilityIn >>if (const AvailabilityAttr *AA = getAttrForPlatform(S.Context, C)) >> if (AA->getIntroduced() >= DeclVersion) >>return true; >> -} else if (K == AR_Deprecated) >> +} else if (K == AR_Deprecated) { >>if (C->isDeprecated()) >> return true; >> +} else if (K == AR_Unavailable) { >> + // It is perfectly fine to refer to an 'unavailable' Objective-C >> method >> + // when it's actually defined and is referenced from within the >> + // @implementation itself. In this context, we interpret >> unavailable as a >> + // form of access control. >> + if (const auto *MD = dyn_cast(OffendingDecl)) { >> +if (const auto *Impl = dyn_cast(C)) { >> + if (MD->getClassInterface() == Impl->getClassInterface() && >> + MD->isDefined()) >> +return true; >> +} >> + } >> +} >> >> if (C->isUnavailable()) >>return true; >> @@ -7471,7 +7485,8 @@ static void DoEmitAvailabilityWarning(Se >>if (const AvailabilityAttr *AA = getAttrForPlatform(S.Context, >> OffendingDecl)) >> DeclVersion = AA->getIntroduced(); >> >> - if (!ShouldDiagnoseAvailabilityInContext(S, K, DeclVersion, Ctx)) >> + if (!ShouldDiagnoseAvailabilityInContext(S, K, DeclVersion, Ctx, >> + OffendingDecl)) >> return; >> >>SourceLocation Loc = Locs.front(); >> @@ -7955,7 +7970,8 @@ void DiagnoseUnguardedAvailability::Diag >> >>
Re: r350768 - [ObjC] Allow the use of implemented unavailable methods from within
Here's some user feedback on this new feature. It looks like the warning is only suppressed if `init` has a definition in the @interface block. In the 4 cases where we saw this warning fire after r349841, it still fires after this change because in all 4 cases a class marked init as unavailable in the @interface but didn't add a definition of it in the classes @implementation (instead relying on calling the superclass's (NSObject) init). It's pretty easy to just add - (instancetype)init { return [super init]; } to these 4 classes, but: a) it doesn't Just Work b) the diagnostic doesn't make it clear that adding a definition of init will suppress the warning. Up to you to decide what (if anything) to do with this feedback :-) (https://bugs.chromium.org/p/chromium/issues/detail?id=917351#c17 has a full reduced code snippet.) On Wed, Jan 9, 2019 at 5:35 PM Alex Lorenz via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: arphaman > Date: Wed Jan 9 14:31:37 2019 > New Revision: 350768 > > URL: http://llvm.org/viewvc/llvm-project?rev=350768=rev > Log: > [ObjC] Allow the use of implemented unavailable methods from within > the @implementation context > > In Objective-C, it's common for some frameworks to mark some methods like > init > as unavailable in the @interface to prohibit their usage. However, these > frameworks then often implemented said method and refer to it in another > method > that acts as a factory for that object. The recent change to how messages > to > self are type checked in clang (r349841) introduced a regression which > started > to prohibit this pattern with an X is unavailable error. This commit > addresses > the aforementioned regression. > > rdar://47134898 > > Differential Revision: https://reviews.llvm.org/D56469 > > Added: > cfe/trunk/test/SemaObjC/call-unavailable-init-in-self.m > Modified: > cfe/trunk/lib/Sema/SemaDeclAttr.cpp > > Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=350768=350767=350768=diff > > == > --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original) > +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Jan 9 14:31:37 2019 > @@ -7269,9 +7269,10 @@ ShouldDiagnoseAvailabilityOfDecl(Sema > /// whether we should emit a diagnostic for \c K and \c DeclVersion in > /// the context of \c Ctx. For example, we should emit an unavailable > diagnostic > /// in a deprecated context, but not the other way around. > -static bool ShouldDiagnoseAvailabilityInContext(Sema , > AvailabilityResult K, > -VersionTuple DeclVersion, > -Decl *Ctx) { > +static bool > +ShouldDiagnoseAvailabilityInContext(Sema , AvailabilityResult K, > +VersionTuple DeclVersion, Decl *Ctx, > +const NamedDecl *OffendingDecl) { >assert(K != AR_Available && "Expected an unavailable declaration > here!"); > >// Checks if we should emit the availability diagnostic in the context > of C. > @@ -7280,9 +7281,22 @@ static bool ShouldDiagnoseAvailabilityIn >if (const AvailabilityAttr *AA = getAttrForPlatform(S.Context, C)) > if (AA->getIntroduced() >= DeclVersion) >return true; > -} else if (K == AR_Deprecated) > +} else if (K == AR_Deprecated) { >if (C->isDeprecated()) > return true; > +} else if (K == AR_Unavailable) { > + // It is perfectly fine to refer to an 'unavailable' Objective-C > method > + // when it's actually defined and is referenced from within the > + // @implementation itself. In this context, we interpret > unavailable as a > + // form of access control. > + if (const auto *MD = dyn_cast(OffendingDecl)) { > +if (const auto *Impl = dyn_cast(C)) { > + if (MD->getClassInterface() == Impl->getClassInterface() && > + MD->isDefined()) > +return true; > +} > + } > +} > > if (C->isUnavailable()) >return true; > @@ -7471,7 +7485,8 @@ static void DoEmitAvailabilityWarning(Se >if (const AvailabilityAttr *AA = getAttrForPlatform(S.Context, > OffendingDecl)) > DeclVersion = AA->getIntroduced(); > > - if (!ShouldDiagnoseAvailabilityInContext(S, K, DeclVersion, Ctx)) > + if (!ShouldDiagnoseAvailabilityInContext(S, K, DeclVersion, Ctx, > + OffendingDecl)) > return; > >SourceLocation Loc = Locs.front(); > @@ -7955,7 +7970,8 @@ void DiagnoseUnguardedAvailability::Diag > > // If the context of this function is less available than D, we > should not > // emit a diagnostic. > -if (!ShouldDiagnoseAvailabilityInContext(SemaRef, Result, Introduced, > Ctx)) > +if (!ShouldDiagnoseAvailabilityInContext(SemaRef, Result, Introduced, > Ctx, > +