Unavailable method checker

[Jordan Rose]

On Oct 18, 2013, at 3:05 , Richard <tarka.t.otter at gmail.com> wrote:

> On 14 Oct 2013, at 18:29, Jordan Rose <jordan_rose at apple.com> wrote:
> 
>> Ah...yes, you were right the first time. It's not that the true and false state are the same, per se...it's that the two sinks have the same state that causes the analyzer to think they're the same. There are a few ways around this:
>> 
>> 1. Ignore cases where the state doesn't change. DereferenceChecker doesn't do this explicitly, but it does skip non-Locs, which will mostly have the same effect.
>> 
>> 2. Add an explicit tag. A SimpleProgramPointTag can be declared as a static variable to provide a tag that's different from the default checker tag.
>> 
>> Either one of these would work. #1 saves a bit of work when we have no information, but it's not impossible that we'll eventually end up having a case where an unconstrained UnknownVal should be considered a problem.
>> 
>> Jordan
>> 
> 
> I went with the Tag solution, nice and simple. So, attached is the current patch with passing test, any further suggestions?
> 
> Ta,
> Richard

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.

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.


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


+        const ObjCSelectorExpr *SelExpr = cast<ObjCSelectorExpr>(ME->getArg(0));

This could fail if the user passes a SEL variable.


+    // We are only checking for SymInt and Symbol comparisons
+    else
+      break;

I'm not sure you want to break...perhaps just ignore these?


Thanks for working on this; sorry for drawing it out even more!
Jordan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131022/2020b0ec/attachment.html>