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

Nico Weber thakis at chromium.org
Fri Mar 20 11:07:14 PDT 2015

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.

>  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?

> 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.

I agree that it's a nice addition to not emit this warning in places that
_are_ protected by respondsToSelector.

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

More information about the cfe-commits mailing list