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