[patch/rfc] An opt-in warning for functions whose availability(introduced) is newer than the deployment target
thakis at chromium.org
Fri Mar 20 11:38:57 PDT 2015
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.)
>> 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
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
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-commits