This is an archive of the discontinued LLVM Phabricator instance.

[Static Analyzer] Generics Checker: When an ObjC method returns a specialized object, track it properly.
ClosedPublic

Authored by xazax.hun on Sep 15 2015, 3:24 PM.

Details

Summary

When the return type of a method of a generic class was itself a generic type, this type was not recorded for the returned symbol.
This patch address this issue (see the test for examples).

There is one odd thing: the return type of a method is computed twice. Once in the preObjCMessage callback and once in the postObjCMessage callback. The reason is that, it is better to report the type error in the pre callback, because this way the user gets cleaner diagnostic (shorter path). However it is only possible to record the type information in the post callback, since the symbol for the return type is not available yet in the pre callback.

Diff Detail

Event Timeline

xazax.hun updated this revision to Diff 34844.Sep 15 2015, 3:24 PM
xazax.hun retitled this revision from to [Static Analyzer] Generics Checker: When an ObjC method returns a specialized object, track it properly..
xazax.hun updated this object.
xazax.hun added a subscriber: cfe-commits.
zaks.anna added inline comments.Sep 15 2015, 3:57 PM
lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
677

If you swap this 'if' with the 'if' above, you do not need to check for IsDeclaredAsInstanceType twice.

680

Could you use StaticResultType here?

860

The path is longer if the call is inlined, correct? (Not much difference otherwise..)

884

It looks like it's only inferring the type information of the specializations and not for non-generic types..

946

Please, rename the map to make it more explicit that it tracks the most specialized type. (I think I've mentioned it before in another review..)

xazax.hun updated this revision to Diff 34858.Sep 15 2015, 5:39 PM
xazax.hun marked 5 inline comments as done.
  • Renamed TypeParamMap.
  • Move the return type checking to post call (the diagnostic presented to the user is not affected by this change).
  • Do not override the tracked type for returned values when the type information is already available.
  • Added some missing TODO comments.
  • Minor refactorings.
lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
680

In StaticResultType every occurrence of a Type Argument is replaced with id, which is not suitable for type checking.

860

In fact there is a heuristic that omits the stepping in into the function when the diagnostic is generated. I moved this check to the post call callback.

884

Added a todo for the missing case.

zaks.anna added inline comments.Sep 15 2015, 5:48 PM
lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
680

From above:
QualType StaticResultType = Method->getReturnType();

You could do
QualType ResultType = StaticResultType.substObjCTypeArgs(

C, TypeArgs, ObjCSubstitutionContext::Result);
688–689

Should the method be renamed to reflect this new behavior?

807

and to diagnose..

xazax.hun updated this revision to Diff 34903.Sep 16 2015, 10:12 AM
xazax.hun marked 3 inline comments as done.
  • Addressed the comments.
  • Minor refactoring.
lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
680

Oh, I see. Sorry for misunderstanding.

zaks.anna accepted this revision.Sep 16 2015, 3:29 PM
zaks.anna edited edge metadata.
This revision is now accepted and ready to land.Sep 16 2015, 3:29 PM
This revision was automatically updated to reflect the committed changes.