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

Repository
rL LLVM

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

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

675 ↗(On Diff #34844)

Could you use StaticResultType here?

795 ↗(On Diff #34844)

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

803 ↗(On Diff #34844)

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

865 ↗(On Diff #34844)

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

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

795 ↗(On Diff #34844)

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.

803 ↗(On Diff #34844)

Added a todo for the missing case.

zaks.anna added inline comments.Sep 15 2015, 5:48 PM
lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
680 ↗(On Diff #34858)

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

You could do
QualType ResultType = StaticResultType.substObjCTypeArgs(

C, TypeArgs, ObjCSubstitutionContext::Result);
688 ↗(On Diff #34858)

Should the method be renamed to reflect this new behavior?

807 ↗(On Diff #34858)

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

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.