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

[Ted Kremenek]

> On Mar 20, 2015, at 11:38 AM, Nico Weber <thakis at chromium.org> wrote:
> 
> On Fri, Mar 20, 2015 at 11:28 AM, Ted Kremenek <kremenek at apple.com <mailto:kremenek at apple.com>> wrote:
> 
>> On Mar 20, 2015, at 11:07 AM, Nico Weber <thakis at chromium.org <mailto:thakis at chromium.org>> wrote:
>> 
>> On Fri, Mar 20, 2015 at 10:42 AM, Ted Kremenek <kremenek at apple.com <mailto:kremenek at apple.com>> wrote:
>> 
>>> On Mar 20, 2015, at 8:35 AM, Nico Weber <thakis at chromium.org <mailto:thakis at chromium.org>> wrote:
>>> 
>>> On Fri, Mar 20, 2015 at 8:10 AM, Ted Kremenek <kremenek at apple.com <mailto: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 seems reasonable, but to be clear I'd have it bracketed with two pragmas.  Everything within that bracket would follow that assumption.

For example:

  #pragma clang assume_availability(macosx, 10.8) begin
  ...
  #pragma clang assume_availability(macosx, 10.8) end

Some other considerations:

- the name should follow exactly what is spelled in the availability attribute for the platform name.
- what about code that runs on different platforms?  Straw man:

  #pragma clang assume_availability(macosx, 10.8) begin
  #pragma clang assume_availability(ios, 8.0) begin
  ...
  #pragma clang assume_availability(ios, 8.0) end
  #pragma clang assume_availability(macosx, 10.8) end

or something more condense?

I assume if you are running with this idea that it appeals to you.

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

Sounds good.

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

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