Unavailable method checker

[Richard]

On 22 Oct 2013, at 18:43, Jordan Rose <jordan_rose at apple.com> wrote:

> Looking good! I do still have a few reservations, though. The main one is this:
> 
> +    if (const SymIntExpr *SIExpr = dyn_cast<SymIntExpr>(*I)) {
> +      // A symbol is being compared to an int value. The Assumption value
> +      // is adjusted so that it is checking the symbol exists.
> +      BinaryOperatorKind op = SIExpr->getOpcode();
> +      bool rhs = SIExpr->getRHS() != 0;
> +      if ((op == BO_EQ && rhs == false) || (op == BO_NE && rhs == true))
> +        Assumption = !Assumption;
> +
> +      continue;
> +    }
> 
> I'm not actually sure which case this handles...is it after a weak function pointer has been converted to bool? Because it also matches this case:
> 
> NSAppKitVersionNumber > NSAppKitVersionNumber10_7
> 
> At the very least, you should be checking that the opcode is one of the operators you expect.
> 

Oops, yes, that was careless. This part of the code handles situations like:

if ([obj respondsToSelector:@selector(doVersion20Stuff)] == YES) {
    // do version 20 stuff
}

If the Assumption bool was not matched to the boolean operator, then the following case would also set version check status:

if ([obj respondsToSelector:@selector(doVersion20Stuff)] == NO) {
    // don't do version 20 stuff
}

> Besides that, here are a storm of little notes:
> 
> +    if (ExplodedNode *N = C.generateSink(StNull, 0, &Tag)) {
> +        ImplicitNullDerefEvent Event = { L, false, N, &C.getBugReporter() };
> +        dispatchEvent(Event);
> +    }
> 
> Extra indentation here.
> 
> 
> +  if (!(isa<SimpleCall>(&Call) || isa<ObjCMethodCall>(&Call)))
> 
> isa<> should work on references as well as pointers.
> 
> 
> +  // If we have an objc method, check for the classes introduced version.
> +  if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {
> +    VersionTuple ClassIntroduced;
> +    const DeclContext *DC = MD->getDeclContext();
> +    if (const ObjCProtocolDecl *PD = dyn_cast<ObjCProtocolDecl>(DC))
> +      ClassIntroduced = versionIntroduced(PD);
> +    else
> +      ClassIntroduced = versionIntroduced(MD->getClassInterface());
> +
> +    Introduced = std::max(Introduced, ClassIntroduced);
> +  }
> 
> I think you only need to do this if the method itself doesn't have an introduced version...it's probably safe to assume that headers are sane and won't have a method introduced before the class that contains it.
> 
> Also, typos/formatting: "ObjC" (or full "Objective-C"), and "class's".
> 
> Nice handling of protocols/categories/implementation/interface.
> 
> 
> +  // Walk through the symbols children looking for a version check.
> 
> Typo: "symbol's"
> 
> 
> +  // If this is a weak function, it is not OK to derefence if availability
> +  // has not been checked for.
> 
> Typo: "dereference"
> 
> 
> +  os << "The method is introduced in ";
> +  os << Introduced;
> +  os << ", later than the deployment target ";
> +  os << deploymentTarget(State);
> +
> +  BugReport *R = new BugReport(*BT, os.str(), N);
> +  BR->emitReport(R);
> 
> I'm not so happy with the message. For one thing, this applies to functions as well as methods, and may eventually apply to enumerations, their enumerators, and/or global variables. For another, it doesn't quite say what's wrong. How about "Calling method introduced in X.Y (deployment target is X.W)"?
> 
> It's also too bad that there's no highlight range here, but I guess the location is accurate enough.
> 
> 

Yes, I did originally have the highlight range, but I could not work out a way to get this from the ImplicitNullDerefEvent, so I removed it.

> +const Decl* UnavailableMethodChecker::weakDeclForSymbol(SymbolRef Sym) const {
> 
> Asterisk on the right, please. 
> 
> 
> +const Decl*
> +UnavailableMethodChecker::weakDeclForRegion(const MemRegion *R) const {
> 
> ...and here too.
> 
> 
> +    }
> +
> +    else if (const SymbolConjured *SConj = dyn_cast<SymbolConjured>(*I)) {
> 
> Please keep these on one line. If it looks too crowded, you can add a blank line before the close brace.
> 
> 
> +      // This is an objective C message, first get the class availability.
> 
> Formatting: "Objective-C"
> 
> 
> +      std::string SelStr = ME->getSelector().getAsString();
> +      if (SelStr == "respondsToSelector:" ||
> +          SelStr == "instancesRespondToSelector:") {
> 
> getAsString() is expensive because it constructs a new string on the heap. Better to check that it is a unary selector, then grab the first selector piece and check that.
> 

Not sure I get you here, isn't only 'class' a unary selector? I changed this to breaking early if there is more than 1 arg, and using the selector name for the first slot only.

> 
> +        const ObjCSelectorExpr *SelExpr = cast<ObjCSelectorExpr>(ME->getArg(0));
> 
> This could fail if the user passes a SEL variable.
> 

Well spotted. I have added a helper method to extract the ObjCSelectorExpr in the case of a SEL variable, it is very ugly code though, is there an easier way to do this? see selectorForArgument().

> 
> +    // We are only checking for SymInt and Symbol comparisons
> +    else
> +      break;
> 
> I'm not sure you want to break...perhaps just ignore these?
> 

I suppose I was simply trying to restrict the version checks as much as possible. I could not think of a valid case that would require walking further through the symbols children. I have removed it anyway, as I also cannot think of a case it helps.

> 
> Thanks for working on this; sorry for drawing it out even more!
> Jordan


So, latest patch attached, I hope I got all the aesthetic things sorted.
Richard



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131024/29511793/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unavailable-2410.patch
Type: application/octet-stream
Size: 18692 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131024/29511793/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131024/29511793/attachment-0001.html>