This is an archive of the discontinued LLVM Phabricator instance.

[Static Analyzer] Checker for Obj-C generics
ClosedPublic

Authored by xazax.hun on Jul 22 2015, 1:40 PM.

Details

Summary

This checker tries to infer type information based on the Obj-C lightweight generics annotation, and detects type errors that would cause runtime errors.
This type check is not done by the compiler because generic collections convert implicitly to the type erased variants due to backward compatibility.
This checker tries to preserver the type information flow sensitively and spot type errors.

Diff Detail

Event Timeline

xazax.hun updated this revision to Diff 30390.Jul 22 2015, 1:40 PM
xazax.hun retitled this revision from to [Static Analyzer] Checker for Obj-C generics.
xazax.hun updated this object.
xazax.hun added a subscriber: cfe-commits.
zaks.anna edited edge metadata.Jul 24 2015, 2:38 PM

Here is a partial review. Some comments below and others are inline.

Thanks!
Anna.

  • Add at least one test that tests for the full error message.
  • Add a test that tests path-sensitive output (the plist file) after testing that that looks correct in Xcode. For example, how is GenericsBugVisitor tested? That should be easy with $ awk '{print "// CHECK: "$0}' in.txt > out.txt
  • nit: It would be helpful if you could use descriptive names for each test.
lib/StaticAnalyzer/Checkers/Checkers.td
456

Please, provide better description of the checker.

lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp
12

"was" -> "were"

Please, add some documentation on what this checker is trying to catch and how it works.

93

"type with specified params" -> "specialized type"?

Also, maybe we should move this above the GenericsBugVisitor declaration for greater visibility?

174

"two" -> "to"

198

Could you add a comment on what this does?
Is there a precondition on the arguments (To and From)? We seem to be walking up the hierarchy of To.
Is To always guaranteed to have a super class? What ensures that?

225

Move this after the check?

230

Could you explain more why are we skipping kindoff?
Is there a test case for this?
Let's add more __kindof tests.

254

Comment does not seem to match the code below.

258

Should the Map be specialized in this example?

271

We ignore the cast on the else branch (mismatched type)?

281

Simplify the code by using early returns.

Also, try to add comments with intuition on what cases are covered.

298

This will not get caught by the check below?

327

Shouldn't we strip more than id casts?

test/Analysis/generics.m
46

I'd prefer if these were copied directly from the headers, without modifications. Ex:
@interface NSMutableArray<ObjectType> : NSArray<ObjectType>

  • (void)addObject:(ObjectType)anObject;

Add tests for checking element retrieval and tracking with ObjC subscript syntax for arrays and dictionaries.

Also see:
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp
335

Add comment explaining that this callback is used to infer the types. Specifically, if a class method is called on a symbol, we use it to infer the type of the symbol.

339

Move State down, where it's used.

347

Clarify what you are doing here.

415

Split funding a tighter method definition into a subroutine.

422

Should checking for id, class and kindof be a helper method?

426

What if ASTCtxt.canAssignObjCInterfaces is false here? Shouldn't we warn?
Will we warn elsewhere? Let's make sure we have a test case. And add a comment.

445

Should we use the type on which this is defined and not the tracked type?

454

Why are we not checking other parameter types? Will those bugs get caught by casts? I guess yes..

459

Why isObjCTypeParamDependent is not used here?

486

Let's not shadow TrackedType.

496

This would be simplified if you pull out the negation; you essentially, list the cases where we do not warn on parameters:

  1. arg can be assigned to (subtype of) param
  2. covariant parameter types and param is a subtype of arg
501

This just returns the previous state.

If you want to create a node, you need to tag it; see the tags on leak reports.

514

We should not use TrackedType in the case lookup failed.
Can the TrackedType be less specialized that the return type?
Maybe rename as IsDeclaredAsInstanceType?

533

What are we doing here?
I prefer not to have any AST pattern matching.

540

This returns predecessor..

xazax.hun updated this revision to Diff 32176.Aug 14 2015, 12:40 PM
xazax.hun edited edge metadata.
xazax.hun marked 29 inline comments as done.

Updated to the latest trunk.
Incorporated the suggestions.

Addressed the suggestions.

lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp
258

No, this case handles buggy implementations, when the type parameters are not forwarded. I reworded the comments to make this more clear.

298

You are right, I refactored the code.

327

We should. And we also should strip some sugar. I rewrote that function.

426

The tracked type might be the super class of the static type (for example when the super type is specialized but the subtype is not). We should not warn in that case. When the tracked type is not in subtyping relationship with the static type, there was a cast somewhere, and the checker issued the warning there. I already have testcase for these cases. I updated the names of the tests to reflect this.

445

The type arguments might be only available for the tracked type.

454

The analyzer does not visit template definitions, only the instantiations. I removed this redundant check.

496

The check there was overcomplicated. Passing a subtype of a parameter as argument should always be safe.

514

The lookup might fail when the tracked type is the super of the dynamic type (which might be the subtype of the static type). In some cases the tracked type might even end up be the super type of the static type (when that super type has more information about the type parameters.)

533

Right now in order to keep the state as low as possible, only generic types are tracked. If it is not a problem to have bigger state, a future improvement would be to track the dynamic type of every object, and utilize that info in this checker. Alternatively that could utilize the get/setDynamicTypeInfo API of ProgramState.

test/Analysis/generics.m
46

I added some additional methods for NSArray to do some additional testing, but I made the rest of the methods more similar to the ones in the actual header files.

This revision was automatically updated to reflect the committed changes.