Unavailable method checker

[Jordan Rose]

On Nov 6, 2013, at 7:18 , Tarka T <tarka.t.otter at gmail.com> wrote:

> On Tue, Nov 5, 2013 at 6:46 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> 
> On Oct 31, 2013, at 5:43 , Richard <tarka.t.otter at gmail.com> wrote:
> 
>> On 29 Oct 2013, at 17:11, Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>>> The logic all seems correct here, but I think we can still do better on the message. The class and protocol cases seem a little underinformative to me, especially since the analyzer doesn't have a note where the availability shows up. So instead of a version kind, how about just passing the decl that has the availability attribute? That way we can say "method", "function", "class 'Foo'", or "protocol 'Foo'".
>>> 
>>> Also, it would be nice to include the platform name with the version. This code is stolen from DeclBase.cpp, but I don't have a better place for it right now.
>>> 
>>>   StringRef TargetPlatform = Context.getTargetInfo().getPlatformName();
>>>   StringRef PrettyPlatformName
>>>     = AvailabilityAttr::getPrettyPlatformName(TargetPlatform);
>>>   if (PrettyPlatformName.empty())
>>>     PrettyPlatformName = TargetPlatform;
>>> 
>>> "Calling method introduced in OS X 10.9 (deployment target is 10.8)"
>>> 
>>> But other than this polish this is looking very good. Have you tried running it on existing projects?
>>> Jordan
>> 
>> OK, I have changed the message as suggested, works for you?
>> 
>> I have been running this on some small projects, but today gave it a test on all the large old projects I have here. Mostly it is working well (and caught many unsafe calls I was not aware of), but there were some issues which I have addressed in the latest patch:
>> 
>> * id types cause false positives
>> When calling methods on id types (in block enumerations and so on) the method decl match is often incorrect, and so the version check information often causes false positives. I fixed this by not checking any ObjCMethodCall without a receiver interface.
> 
> Ha, good catch.
> 
> 
>> * Array and Dictionary subscripting
>> When using subscripting in a project with a deployment target less than iOS6 (or 10.8?), every subscript will cause a version warning. However, as stated here:
>> 
>> https://developer.apple.com/library/ios/releasenotes/ObjectiveC/ObjCAvailabilityIndex/index.html
>> 
>> subscripting is handled back to iOS5 and 10.6. I am not sure of the best way to handle this, the current workaround was just to match the selectors by name and not version check when we have a subscript selector. Ideally the checker would make sure that the deployment target was high enough for the runtime to support subscripting, is there a way to get this information from some API?
> 
> ObjCMethodCall has an accessor that lets you know if it's using subscripting syntax. I think we can ignore it in that case.
> 
> 
>> * Methods that call [super sameMethod]
>> I had a few false positives where implementing eg
>> 
>> - (void)encodeRestorableStateWithCoder:(NSCoder *)coder
>> {
>>     [super encodeRestorableStateWithCoder:coder];
>> }
>> 
>> Currently I have left these as false positives, but perhaps there should be special casing to handle this situation too. What do you think about this, is it safe to ignore methods called on super that have been implemented on self, regardless of deployment target? 
> 
> The most pedantic thing to do here would be to require putting an availability attribute on your own method, but I don't think that will fly. Let's start conservative and say it's always okay to call super if the caller has the same selector as the callee. (In most other cases you'd just use self.)
> 
> 
> Spot comments:
> 
> +    const Decl *VD = VersionDecl == NULL ? D : VersionDecl;
> 
> Might as well just set VersionDecl to D initially. It's not used for anything else.
> 
> +    os << "'" << cast<NamedDecl>(D)->getName() << "'";
> 
>> 
> You should just be able to use "os << *D" here, which will work even if we start printing method names (which aren't simple identifiers). You can drop the cast by making the parameter always be NamedDecl.
> 
> +  default:
> +    break;
> 
> Please make this llvm_unreachable, since we don't expect any other kinds to come in here and won't handle them properly if they do.
> 
> Everything else looks good. Thanks for testing this on existing projects.
> Jordan
> 
> OK, I think the current patch covers everything then. Let me know if you see any more issues.
> 
> Regards,
> Richard

This looks good to me. I'd like to give Ted a chance to look at it too before committing, in case he has any comments. Unfortunately, we were tied up with the Developer Meeting on Wed and Thu, and he's on vacation today, so that won't be until next week. 

Jordan

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