[PATCH] D39711: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:587
   if (TrackedType &&
+      !isa<ExplicitCastExpr>(CE) &&
       !ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, *TrackedType) &&
----------------
george.karpenkov wrote:
> dcoughlin wrote:
> > xazax.hun wrote:
> > > george.karpenkov wrote:
> > > > Should it check that we are actually casting to the right type? Also it's a bit strange that isa<> check on line 569 did not catch this case, maybe that if- branch should be generalized instead?
> > > I agree, line 569 supposed to handle this case and also update the state accordingly.
> > For suppressive casts, I am worried about updating the state to reflect destination type.
> > 
> > Programmers use a suppressive cast when they know a better invariant about the contents of a collection -- at a particular point in time -- than the analysis infers. It is not a promise that the invariant will hold at all later program points. I'm particularly worried that adding one suppressive cast would require the programmer to add other, different suppressive casts later in the program. For this reason I don't think it make sense to update the specialized type args map with the destination type of the cast.
> > 
> > Gabor: what do you think about the alternative of always removing the inferred specialized type args information for an unspecialized symbol on an explicit cast? After an explicit cast we would essentially treat the value as unspecialized and so not warn about later uses until the analyzer infers more information. Essentially this would be an acknowledgement that an explicit cast means the programmer had an invariant in mind that couldn't be represented in the type system and so the analyzer should back off.
> Sounds reasonable, though actual examples would be even more helpful. 
Basically, if we encounter such a situation, we have 3 options:
* Not touch the state
* Believe the explicit cast
* Reset the state for that symbol, so act as if we know nothing about it

Right now we do not change the state explicitly in that case, only suppress the warning, so we might end up picking the first option. However, I think, in case we cannot trust the 2nd, we should pick the 3rd. So we avoid having inconsistent info in the state and reporting errors later on based on this data. What do you think?

The decision we make here should be reflected and documented around line 569. So the treatment of explicit casts are consistent across the checker. 


https://reviews.llvm.org/D39711