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

Repository
rL LLVM

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 ↗(On Diff #30390)

Please, provide better description of the checker.

lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp
11 ↗(On Diff #30390)

"was" -> "were"

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

92 ↗(On Diff #30390)

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

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

173 ↗(On Diff #30390)

"two" -> "to"

197 ↗(On Diff #30390)

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?

224 ↗(On Diff #30390)

Move this after the check?

229 ↗(On Diff #30390)

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

253 ↗(On Diff #30390)

Comment does not seem to match the code below.

257 ↗(On Diff #30390)

Should the Map be specialized in this example?

270 ↗(On Diff #30390)

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

280 ↗(On Diff #30390)

Simplify the code by using early returns.

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

297 ↗(On Diff #30390)

This will not get caught by the check below?

326 ↗(On Diff #30390)

Shouldn't we strip more than id casts?

test/Analysis/generics.m
45 ↗(On Diff #30390)

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
334 ↗(On Diff #30390)

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.

338 ↗(On Diff #30390)

Move State down, where it's used.

346 ↗(On Diff #30390)

Clarify what you are doing here.

414 ↗(On Diff #30390)

Split funding a tighter method definition into a subroutine.

421 ↗(On Diff #30390)

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

425 ↗(On Diff #30390)

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.

444 ↗(On Diff #30390)

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

453 ↗(On Diff #30390)

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

458 ↗(On Diff #30390)

Why isObjCTypeParamDependent is not used here?

485 ↗(On Diff #30390)

Let's not shadow TrackedType.

495 ↗(On Diff #30390)

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
500 ↗(On Diff #30390)

This just returns the previous state.

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

513 ↗(On Diff #30390)

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

532 ↗(On Diff #30390)

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

539 ↗(On Diff #30390)

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
257 ↗(On Diff #30390)

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

297 ↗(On Diff #30390)

You are right, I refactored the code.

326 ↗(On Diff #30390)

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

425 ↗(On Diff #30390)

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.

444 ↗(On Diff #30390)

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

453 ↗(On Diff #30390)

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

495 ↗(On Diff #30390)

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

513 ↗(On Diff #30390)

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

532 ↗(On Diff #30390)

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
45 ↗(On Diff #30390)

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.
cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp