This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dcoughlin on Nov 6 2017, 6:32 PM.

Details

Summary

The ObjCGenerics checker warns on a cast when there is no subtyping relationship
between the tracked type of the value and the destination type of the cast. It
does this even if the cast was explicitly written. This means the user can't
write an explicit cast to silence the diagnostic.

This patch treats explicit casts involving generic types as an indication from the programmer
that the Objective-C type system is not rich enough to express the needed invariant. On
explicit casts, the checker now removes any existing information inferred about the type
arguments. Further, it no longer assumes the casted-to specialized type because the
invariant the programmer specifies in the cast may only hold at a particular program point
and not later ones. This prevents a suppressing cast from requiring a cascade of casts down the
line.

rdar://problem/33603303

Diff Detail

Repository
rL LLVM

Event Timeline

dcoughlin created this revision.Nov 6 2017, 6:32 PM

@xazax.hun Would you be willing to take a look?

lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
587 ↗(On Diff #121830)

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?

test/Analysis/generics.m
7143 ↗(On Diff #121830)

could we remove dict with locations from the test output entirely, and just check that we get those warnings, in order?

xazax.hun edited edge metadata.Nov 7 2017, 4:58 AM

@xazax.hun Would you be willing to take a look?

Sure, I basically agree with George.

lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
587 ↗(On Diff #121830)

I agree, line 569 supposed to handle this case and also update the state accordingly.

dcoughlin added inline comments.Nov 7 2017, 2:31 PM
lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
587 ↗(On Diff #121830)

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.

lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
587 ↗(On Diff #121830)

Sounds reasonable, though actual examples would be even more helpful.

xazax.hun added inline comments.Nov 8 2017, 10:37 PM
lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
587 ↗(On Diff #121830)

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.

dcoughlin updated this revision to Diff 122550.Nov 10 2017, 4:44 PM
dcoughlin edited the summary of this revision. (Show Details)

Update to address Gabor's feedback and adopt "option 3". This changes the casting logic to always forget the specialized type arguments on an explicit cast regardless of whether it is an upcast/downcast or a cross cast.

This revision is now accepted and ready to land.Nov 13 2017, 3:04 AM
This revision was automatically updated to reflect the committed changes.