This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] ObjCGenerics: account for __kindof specifiers met along a chain of casts.
Changes PlannedPublic

Authored by NoQ on May 15 2017, 5:35 AM.

Details

Summary

__kindof type specifiers allow the developers to express their intent in discriminating between generic type arguments being matched exactly or allowing sub-types. When generics were introduced, it was already too late to add __kindof specifiers consistently across all the code. So for now in most code we believe it's all right not to have them, but we need to expect to have them at least in some places (which would normally be API declarations) and pick them up when they're present by loosening our dynamic type information from non-__kindof to __kindof, which would suppress some of the false "invalid cast" warnings.

In the test provided, the __kindof is only present on the API return value; direct cast from the object created within the callee function is an error, however casting indirectly through a __kindof type should be possible, because just one __kindof annotation at the API declaration is considered to be enough to express the intent of "it was __kindof from the start but we didn't bother adding it".

For now, we discard the original type information when we find __kindof specifiers in a chain of casts.

Diff Detail

Event Timeline

NoQ created this revision.May 15 2017, 5:35 AM
NoQ updated this revision to Diff 99150.May 16 2017, 8:26 AM

Cover more cases and be more conservative.

@Gábor: I didn't want to bother you with this, but i'm not entirely sure about how to deal with these false positives; since you're the original author of this check, if you see anything obviously wrong here please comment on me :)

In D33191#756174, @NoQ wrote:

@Gábor: I didn't want to bother you with this, but i'm not entirely sure about how to deal with these false positives; since you're the original author of this check, if you see anything obviously wrong here please comment on me :)

I do not see anything obviously wrong. Of course this approach is very conservative, but in order to do it less conservatively probably we would need to store the type information separately for every type argument. Converting to that approach would be a lot of work, and it might make the check more complex.

zaks.anna added inline comments.May 17 2017, 8:59 AM
lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
464

I am concerned that this might cause regressions as other logic in this function is not triggered when this condition fires. For example, this would only be safe if Current and StaticUpperBoound are the same type apart from __kindof modifier.

NoQ updated this revision to Diff 99443.May 18 2017, 9:16 AM

I think i found a relatively clean way of storing the kindof specifiers, which is within the most-specialized type object itself.

Like, if we see Foo<X, Y> being casted to Foo<__kindof X, Y> and in another place to Foo<X, __kindof Y>, then we'd store Foo<__kindof X, __kindof Y> in our map.

NoQ updated this revision to Diff 99444.May 18 2017, 9:23 AM

Added a bit more info in the code comments.

In D33191#758583, @NoQ wrote:

I think i found a relatively clean way of storing the kindof specifiers, which is within the most-specialized type object itself.

Like, if we see Foo<X, Y> being casted to Foo<__kindof X, Y> and in another place to Foo<X, __kindof Y>, then we'd store Foo<__kindof X, __kindof Y> in our map.

Great idea! I also think this approach is superior to the previous one.

NoQ planned changes to this revision.EditedMay 18 2017, 9:55 AM

Todo:

  • See if the extra __kindofs leak into diagnostic messages.
  • Test how this behaves in cases (2) and (4), add tests.
  • Add tests for the recursive __kindof specifier merge (i.e. properly merge the second __kindof in NSSet<__kindof NSArray<__kindof NSString>>).