<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Mar 20, 2015, at 8:35 AM, Nico Weber <<a href="mailto:thakis@chromium.org" class="">thakis@chromium.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote">On Fri, Mar 20, 2015 at 8:10 AM, Ted Kremenek <span dir="ltr" class=""><<a href="mailto:kremenek@apple.com" target="_blank" class="">kremenek@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="">Hi Nico,<div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">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".</div><div class=""><br class=""></div><div class="">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:</div><div class=""><br class=""></div><div class="">(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.</div><div class=""><br class=""></div><div class="">(2) You silence that warning by adding the redeclaration.  Then all future uses of that method that you introduce won't get a warning.</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">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?</div></div></blockquote><div class=""><br class=""></div><div class="">Hi Ted,</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">(Also note that this warning is off by default and not in -Wall.)</div><div class=""><br class=""></div><div class="">Nico</div><div class=""> </div></div></div></div></div></blockquote><br class=""></div><div>Hi Nico,</div><div><br class=""></div><div>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.  People often get the actual checking wrong, and they may also use an unavailable API multiple times.</div><div><br class=""></div><div>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:</div><div><br class=""></div><div>(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.</div><div><br class=""></div><div>(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.</div><div><br class=""></div><div>(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.</div><div><br class=""></div><div>(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.</div><div><br class=""></div><div>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.</div><div><br class=""></div><div>Did you consider an approach like this?</div><div><br class=""></div><div>Ted</div><div><br class=""></div><div><br class=""></div><div><br class=""></div></body></html>