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

[Nico Weber]

On Fri, Mar 20, 2015 at 11:28 AM, Ted Kremenek <kremenek at apple.com> wrote:

>
> On Mar 20, 2015, at 11:07 AM, Nico Weber <thakis at chromium.org> wrote:
>
> On Fri, Mar 20, 2015 at 10:42 AM, Ted Kremenek <kremenek at apple.com> wrote:
>
>>
>> On Mar 20, 2015, at 8:35 AM, Nico Weber <thakis at chromium.org> wrote:
>>
>> On Fri, Mar 20, 2015 at 8:10 AM, Ted Kremenek <kremenek at apple.com> wrote:
>>
>>> Hi Nico,
>>>
>>> I'm really sorry, but this is the first time I saw this.  When you first
>>> proposed the patch I was away from work for several weeks and wasn't
>>> reading email.  I then missed most of this.
>>>
>>> I honestly am very concerned about this approach.  The problem is
>>> certainly well-motivated, but it feels like a really partial solution to
>>> the problem that doesn't provide any real safety.  Looking back at the
>>> thread I see you and Doug discussed using dataflow analysis, which really
>>> seems like the right approach to me.  Even some basic lexical analysis to
>>> check if a guard existed before use probably would have provided some
>>> reasonable checking, and I disagree with Doug that the dataflow approach
>>> was "a heck of a lot more work".
>>>
>>> The thing I don't like about this approach is that as soon as you
>>> provide the redeclaration you lose all checking for uses of a method.  Here
>>> are my concerns:
>>>
>>> (1) You get a warning once for a method that is newer than your minimum
>>> deployment target regardless of whether or not you are using it safely.
>>>
>>> (2) You silence that warning by adding the redeclaration.  Then all
>>> future uses of that method that you introduce won't get a warning.
>>>
>>> I don't see how this provides any real checking at all.  Even if the
>>> first warning was valid in identify and unguarded case, you get no help
>>> from the compiler if you add the guard and do it incorrectly.
>>>
>>> My concern here is not that this problem isn't important to solve.  It's
>>> a very big problem.  I am concerned that this approach causes users to
>>> clutter their code with redeclarations and it provides them no much safety
>>> checking at all.  I know part of this was motivated by real issues you were
>>> seeing in Chrome, a large codebase that needs to run on multiple OS
>>> versions.  Do you think this will be a practical, and useful approach, to
>>> solving that problem in practice on a codebase of that size?
>>>
>>
>> Hi Ted,
>>
>> I agree that this is an imperfect solution. However, it's identical to
>> our current approach of just building against an old SDK (10.6). This is
>> what we currently do, and having to declare methods before using them does
>> help in practice. However, the OS suppresses some bug fixes when linking
>> against an old SDK, so we want to switch to a model where we use the newest
>> SDK. This warning is supposed to give us the same level of safety as using
>> an old SDK, without getting the drawbacks of an old SDK.
>>
>> This isn't Chromium-specific either: I talked to the Firefox folks, and
>> they currently build against an old SDK for the same reasons we do.
>>
>> (Also note that this warning is off by default and not in -Wall.)
>>
>> Nico
>>
>>
>>
>> Hi Nico,
>>
>> My main concern about this warning, as is, is that it creates a workflow
>> that castrates itself immediately and adds lots of ancillary boilerplate to
>> the code whose purpose is mostly indiscernible to a reader.  It also
>> doesn't provide any real checking, and thus in my opinion creates a false
>> sense of security.
>>
>
> Again, this approach does help us in practice. I agree that it's not
> perfect, but it does work for us.
>
>
> I understand that it works for us, but it's not a great workflow in
> general.  It would be great if we had something that worked in general for
> OS X and iOS developers.  This feels, at least to me, very idiomatic to
> something that might be adopted in a particular codebase.
>
>
>
>>  People often get the actual checking wrong, and they may also use an
>> unavailable API multiple times.
>>
>> I do think that we can take what you have and make something much better,
>> and far more powerful, with not much effort.  Here's a counterproposal:
>>
>> (a) Have the availability warning, as we have now, but delay issuing it
>> until you can do a bit more analysis to validate if the use of the
>> unavailable API is safe or not.
>>
>> (b) For each use of an unavailable API, walk up the lexical structure and
>> look for 'if' guards for 'respondsToSelector'.  If there is a
>> 'respondsToSelector' check for the given potentially unavailable selector,
>> then suppress the warning.
>>
>> (c) As a refinement, you can possibly generalize the 'respondsToSelector'
>> check to imply that other unavailable selectors that have the same API
>> availability would also be guarded against.  This is potentially unsafe,
>> however, as sometimes an API was internal Apple API in a previous OS, but
>> this is an idiom that is widely followed.  Actually, 'respondsToSelector'
>> for just checking on potentially unavailable selector is unsafe for this
>> reason.  Regardless, I think the analysis would be pretty much the same.
>> You can just walk the lexical structure and look for availability guards
>> that imply a minimum OS availability.  This seems very trivial to do; this
>> is just an AST walk.  You can also batch it up and do it efficiently by
>> only lazily doing it if a potentially unavailable API is encountered.
>>
>> (d) As an escape hatch, provide a more localized way to suppress the
>> warning other than suppressing all cases of the warning by adding a
>> category.  The category approach is completely non-obvious anyway, and from
>> a readability perspective doesn't provide any insight that the warning is
>> being suppressed by this bit of what appears to be redundant code.
>>
>
> The motivation is that it is the same mechanism one would use when
> building against an old SDK.
>
> What do you think would be a better escape hatch syntax?
>
>
> I'm talking to process here, but we do have pragmas already for
> suppressing warnings.  Those can already be used to suppress warnings
> within blocks of code.  That idea could be extended to suppress the warning
> for specific selectors, or at least selectors with in an availability
> equivalence class (so to speak).
>
> Note that this approach could extend to not just selectors, but any API.
>

Brainstorming a bit:
#pragma assume_availability(mac, 10.8)
…actually, that all I've got :-P


>
>
>
>>
>> This counterproposal doesn't require dataflow analysis, and provides a
>> strong model where actual checking is done.  It also doesn't create a
>> workflow that castrates itself upon silencing the first warning for an
>> unavailable selector.
>>
>> Did you consider an approach like this?
>>
>
> Yes. It's not sufficient. Some of our classes do calls to partial methods
> unconditionally, and we only instantiate these classes after checking that
> the OS is new enough (else we build a dummy object) with the same interface.
>
>
> That's a very good point.
>
> The suggestion I made above (with pragmas) could solve that problem, and
> would allow you to be more declarative about the actual availability you
> expect some code to be executed.  I think that would communicate far more
> intent than just adding categories that redeclare methods, and it gives you
> the flexibility to decide at what granularity you want to suppress the
> warning.
>

Ok, for next steps:

1.) I'll try to come up with a patch that suppresses the current warning in
if (respondsToSelector) blocks for objc message sends, and in if
(CFWeakFun) for C functions.
2.) I'll wait a bit for folks to brainstorm how exactly the pragma will
look, and once that has settled use that as a signal instead of
redeclarations.

Sounds good?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150320/a8c65cdb/attachment.html>