[patch/rfc] An opt-in warning for functions whose availability(introduced) is newer than the deployment target

[Nico Weber]

r232750, thanks!

On Tue, Mar 17, 2015 at 9:44 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Mar 2, 2015, at 2:25 PM, Nico Weber <thakis at chromium.org> wrote:
>
> One more tweak: Apparently for `@interface A @end @class A;`, a lookup for
> A finds the @interface decl, not the @class redecl.
>
>
> Yeah, I think this is a longstanding hack from when we starting tracking
> redeclaration chains for Objective-C class declarations (and @class started
> being an ObjCInterfaceDecl). We (ahem, I) didn’t hunt down all of the
> places where we were expecting name lookup to return the definition.
>
> So for ObjCInterfaceDecls, the explicit loop is necessary – this patch
> adds it back for that case. With this change, I can build most of chrome
> with this warning enabled (and some declaration tweaks in chrome). (Only
> "most of" because the build is still running – no known bugs).
>
>
> Okay. Everything else LGTM, I say “go for it!”.
>
> - Doug
>
> On Sun, Mar 1, 2015 at 7:52 PM, Nico Weber <thakis at chromium.org> wrote:
>
>> On Mon, Feb 2, 2015 at 10:26 PM, Douglas Gregor <dgregor at apple.com>
>> wrote:
>>
>>> Hi Nico,
>>>
>>> On Jan 8, 2015, at 6:14 PM, Nico Weber <thakis at chromium.org> wrote:
>>>
>>> Hi,
>>>
>>> the Mac OS X and iOS SDKs add new functions in new releases. Apple
>>> recommends using the newest SDK and setting the deployment target to
>>> whatever old OS version one wants to support, and only calling new
>>> functions after checking that they are available at runtime.
>>>
>>> In practice, we (Chromium) get this wrong. Others who support old OS X
>>> versions get this wrong too. Hence, we (Chromium) use a very old SDK and
>>> then manually declare new functions when we want to call them – this
>>> reduces the chance of us forgetting if they are available at runtime
>>> considerably, in practice. But using an old SDK has its problems –
>>> sometimes the frameworks check which SDK an app was linked against and only
>>> then activate bug fixes, and newer Xcodes don't ship include old SDKs.
>>>
>>>
>>> That’s an interesting approach to handling the availability problem; I
>>> hadn’t heard of it before, but I see the logic there.
>>>
>>> Ideally, we could use a new SDK but get a warning when we use a new API
>>> without a manual redeclaration – this protects us against new APIs the same
>>> way using an old SDK does without the drawbacks that this brings.
>>>
>>> The attached patch is a sketch how such a warning might work. How
>>> repulsive is this idea? Are there other approaches to this problem? If the
>>> basic idea is ok:
>>>
>>>
>>> This is a drastically different approach than I’d imagined. Whenever
>>> I’ve thought about this problem, I’ve always come back to some form of
>>> dataflow analysis that checks whether uses of “not-yet-introduced” API is
>>> used in a sane way: is it dominated by some check that implies the
>>> availability, e.g., a -respondsToSelector: check on a method with at least
>>> that availability, or checking whether “[NSFoo class]” is non-null when the
>>> class has availability. I suspect that’s the idea behind Deploymate (
>>> http://www.deploymateapp.com), although I’ve never used it, and it has
>>> the benefit that it should make idiomatic code (that does the right
>>> checking) just work.
>>>
>>> It’s also a heck of a lot more work to implement than the approach
>>> you’re using.
>>>
>>
>> Right :-)
>>
>>
>>>
>>> Any comments on the implementation?
>>>
>>>
>>> The implementation generally looks fine. One minor comment:
>>>
>>> +    case AR_NotYetIntroduced: {
>>> +      // don't do this for enums, they can't be redeclared.
>>> +      if (isa<EnumConstantDecl>(D) || isa<EnumDecl>(D))
>>> +        break;
>>> +      bool FoundRedecl = false;
>>> +      for (Decl *Redecl = D->getMostRecentDecl(); Redecl &&
>>> !FoundRedecl;
>>> +           Redecl = Redecl->getPreviousDecl()) {
>>> +        if (Redecl->getAttr<AvailabilityAttr>()->isInherited())
>>> +          FoundRedecl = true;
>>> +      }
>>> +      if (!FoundRedecl)
>>> +        S.EmitAvailabilityWarning(Sema::AD_Partial, D, Message, Loc,
>>> +                                  UnknownObjCClass, ObjCPDecl,
>>> +                                  ObjCPropertyAccess);
>>> +      break;
>>> +    }
>>>
>>> Generally speaking, name lookup will always find the most recent
>>> declaration, so you might be able to skip the D->getMostRecentDecl() bit
>>> entirely and just check that the availability attribute was inherited.
>>>
>>
>> That worked, done.
>>
>> I also had to add some explicit code for handling redeclarations in
>> @interfaces, as these aren't modeled as redeclarations in the AST. I also
>> added the property note used in the other availability warnings, and I
>> added lots of tests.
>>
>
> <clang-redecl.diff>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150319/fb8e36b2/attachment.html>