Unavailable method checker

[Tarka T]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131106/e64ee5b5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unavailable-0611.patch
Type: application/octet-stream
Size: 21427 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131106/e64ee5b5/attachment.obj>