Unavailable method checker

[Richard]

Hey Ted,

Took a while to find the time, attached is the latest patch. Notes below.

On 04 Dec 2013, at 00:24, Ted Kremenek <kremenek at apple.com> wrote:
> 
> To start off, this file is missing the standard LLVM boilerplate, which mentions the name of the file and what its purpose is.  In this case, a brief description of the purpose of the checker seems warranted.
> 

Comments and boilerplate added, methods renamed.

>>     VersionTuple deploymentTarget(ProgramStateRef State) const;
> 
> I don’t think this method is even needed.  More comments below.
> 

There are still a couple of places where this is needed. For example, checking if the introduced version of a function is later than the deployment target, or to output the deployment target in the bug report.

>>     VersionTuple getCheckedForVersion(ProgramStateRef State) const;
>>     ProgramStateRef setCheckedForVersion(ProgramStateRef State,
>>                                          VersionTuple V) const;
>>   };
>> }
>> 
>> REGISTER_TRAIT_WITH_PROGRAMSTATE(CheckedVersionMajor, unsigned)
>> REGISTER_TRAIT_WITH_PROGRAMSTATE(CheckedVersionMinor, unsigned)
>> REGISTER_TRAIT_WITH_PROGRAMSTATE(CheckedVersionSubminor, unsigned)
> 
> Having three maps is far less efficient than having one map for VersionTuple.  It’s not just the size of the individual pieces of data; its the metadata to represent the map itself.  All of these seemed to be changed together in lockstep, it is is far more efficient to both retrieve and set data from one map instead of 3.  To understand the efficiency, each map is represented by a functional AVL binary tree.  Having three trees requires balancing and querying three trees, as well as the memory associated with three trees.
> 

Yes, this was supposed to be temporary while I worked out how to stuff a VersionTuple into the program state. After spending a bit of time on this, I am still none the wiser. What is the easiest way to achieve this? To use a single element list of VersionTuple as the type, or is there some other way to extend the PartialProgramState to handle a VersionTuple?

>> 
>>   VersionTuple Introduced = versionIntroduced(D);
>> 
>>   // If we have an ObjC method with no introduced version, check the class's or
>>   // the protocol's introduced version.
>>   const NamedDecl *VersionDecl = cast<NamedDecl>(D);
>>   if (Introduced.empty()) {
>>     if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {
>>       const DeclContext *DC = MD->getDeclContext();
>>       if (const ObjCProtocolDecl *PD = dyn_cast<ObjCProtocolDecl>(DC)) {
>>         Introduced = versionIntroduced(PD);
>>         VersionDecl = PD;
>>       } else {
>>         const ObjCInterfaceDecl *ID = MD->getClassInterface();
>>         Introduced = versionIntroduced(ID);
>>         VersionDecl = ID;
>>       }
>>     }
>>   }
> 
> This looks like this can be hoisted directly into “versionIntroduced” (or rather, “getVersionIntroduced”).  That way getVersionIntroduced() computes the truth as a self-encapsulated information, regardless of whether it consults the map or has to compute some of the information lazily.  Also, it doesn’t look like all of the callers to getVersionIntroduced() compute this information consistently; hoisting it into getVersionIntroduced() would solve that problem.
> 

Done, although it is a little bit messy as we need both the version tuple and the decl from which it came, so the getVersionIntroduced function now returns a pair.

>> 
>>   if (TargetMinVersion >= Introduced)
>>     return;
>> 
>>   // We have a method being called that is available later than the minimum
>>   // deployment target, so we need to check for any availability checks.
>>   VersionTuple CheckedVersion = getCheckedForVersion(State);
>>   if (CheckedVersion >= Introduced)
>>     return;
>> 
>>   if (ExplodedNode *N = Ctx.addTransition(State)) {
>>     displayError(State, Introduced, VersionDecl, N, &Ctx.getBugReporter());
>>   }
> 
> This is good, but maybe we should only emit a warning once along a path.  I’m concerned about a cascade of warnings.  For example, say I had 10 function calls, one after the other, that were used unsafely, but would be safely guarded with a single check.  We really don’t need 10 warnings; we only need one.  If we stopped tracking version information along a path, we could perhaps only emit the warning once?  One way to do that would be to pump up the checked version to something really high and store that in the state.  A call to ‘addTransition()’ doesn’t stop the path; the analyzer will keep exploring a path.
> 

Is it not sufficient to just change this to a generateSink instead of addTransition?

>> void ento::registerUnavailableMethodChecker(CheckerManager &mgr) {
>>   mgr.registerChecker<UnavailableMethodChecker>();
> 
> As mentioned earlier, you can put some checking for deployment information right here.  If the context doesn’t support running the checker, just don’t run it at all.
> 

This is a nice idea, but how can we get hold of the context here? 

>> }
> 
> For the test cases, they look like a good start.  One thing I’d like to see is a test case that shows two unsafe calls one after the other.  For example, you currently have:
> 
>> - (void)doVersion30InstanceStuff {
>>   [super doVersion30InstanceStuff]; // no warning
>>   [self doVersion40InstanceStuff];  // expected-warning {{Calling method introduced in OS X 40.0 (deployment target is 10.5.0)}}
>> }
> 
> 
> Try adding a third call here, maybe just repeating the second line:
> 
>> - (void)doVersion30InstanceStuff_B {
>>   [super doVersion30InstanceStuff]; // no warning
>>   [self doVersion40InstanceStuff];  // expected-warning {{Calling method introduced in OS X 40.0 (deployment target is 10.5.0)}}
>>   [self doVersion40InstanceStuff];  // expected-warning {{Calling method introduced in OS X 40.0 (deployment target is 10.5.0)}}
>> }
> 
> 
> Right now I’d expect to see two warnings.  It would be great to get them down to one (just the first one).
> 

Done.

> Overall this is looking really great.  Thanks so much for working on this!
> 
> Cheers,
> Ted

Other than the state map and the checker registration, I think I have everything else covered. Let me know what you think.

Ta,
Richard.