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

2017-11-13 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

LGTM!


https://reviews.llvm.org/D39711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-11-08 Thread 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(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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-11-08 Thread George Karpenkov via Phabricator via cfe-commits 
george.karpenkov added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:587
   if (TrackedType &&
+  !isa(CE) &&
   !ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, *TrackedType) &&

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. 


https://reviews.llvm.org/D39711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-11-07 Thread Devin Coughlin via Phabricator via cfe-commits 
dcoughlin added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:587
   if (TrackedType &&
+  !isa(CE) &&
   !ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, *TrackedType) &&

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.


https://reviews.llvm.org/D39711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-11-07 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

In https://reviews.llvm.org/D39711#917433, @dcoughlin wrote:

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


Sure, I basically agree with George.




Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:587
   if (TrackedType &&
+  !isa(CE) &&
   !ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, *TrackedType) &&

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.


https://reviews.llvm.org/D39711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-11-06 Thread George Karpenkov via Phabricator via cfe-commits 
george.karpenkov added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:587
   if (TrackedType &&
+  !isa(CE) &&
   !ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, *TrackedType) &&

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?



Comment at: test/Analysis/generics.m:7143
 // CHECK-NEXT:   
-// CHECK-NEXT:line375
+// CHECK-NEXT:line380
 // CHECK-NEXT:col70

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


https://reviews.llvm.org/D39711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-11-06 Thread Devin Coughlin via Phabricator via cfe-commits 
dcoughlin added a comment.

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


https://reviews.llvm.org/D39711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-11-06 Thread Devin Coughlin via Phabricator via cfe-commits 
dcoughlin created this revision.
Herald added a subscriber: szepet.

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 changes diagnostic emission to allow an explicit cast to silence
the diagnostic.

rdar://problem/33603303


https://reviews.llvm.org/D39711

Files:
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  test/Analysis/generics.m

Index: test/Analysis/generics.m
===
--- test/Analysis/generics.m
+++ test/Analysis/generics.m
@@ -247,6 +247,11 @@
   withMutArrMutableString(b); // expected-warning {{Conversion}}
 }
 
+void trustExplicitCastsAfterInference(MutableArray *a) {
+  withMutArrString(a);
+  withMutArrMutableString((MutableArray *)a); // no-warning
+}
+
 NSArray *getStrings();
 void enforceDynamicRulesInsteadOfStatic(NSArray *a) {
   NSArray *b = a;
@@ -4616,25 +4621,25 @@
 // CHECK-NEXT: start
 // CHECK-NEXT:  
 // CHECK-NEXT:   
-// CHECK-NEXT:line259
+// CHECK-NEXT:line264
 // CHECK-NEXT:col3
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
 // CHECK-NEXT:   
-// CHECK-NEXT:line259
+// CHECK-NEXT:line264
 // CHECK-NEXT:col9
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
 // CHECK-NEXT:  
 // CHECK-NEXT: end
 // CHECK-NEXT:  
 // CHECK-NEXT:   
-// CHECK-NEXT:line260
+// CHECK-NEXT:line265
 // CHECK-NEXT:col3
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
 // CHECK-NEXT:   
-// CHECK-NEXT:line260
+// CHECK-NEXT:line265
 // CHECK-NEXT:col10
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
@@ -4650,25 +4655,25 @@
 // CHECK-NEXT: start
 // CHECK-NEXT:  
 // CHECK-NEXT:   
-// CHECK-NEXT:line260
+// CHECK-NEXT:line265
 // CHECK-NEXT:col3
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
 // CHECK-NEXT:   
-// CHECK-NEXT:line260
+// CHECK-NEXT:line265
 // CHECK-NEXT:col10
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
 // CHECK-NEXT:  
 // CHECK-NEXT: end
 // CHECK-NEXT:  
 // CHECK-NEXT:   
-// CHECK-NEXT:line260
+// CHECK-NEXT:line265
 // CHECK-NEXT:col19
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
 // CHECK-NEXT:   
-// CHECK-NEXT:line260
+// CHECK-NEXT:line265
 // CHECK-NEXT:col19
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
@@ -4680,20 +4685,20 @@
 // CHECK-NEXT:  kindevent
 // CHECK-NEXT:  location
 // CHECK-NEXT:  
-// CHECK-NEXT:   line260
+// CHECK-NEXT:   line265
 // CHECK-NEXT:   col19
 // CHECK-NEXT:   file0
 // CHECK-NEXT:  
 // CHECK-NEXT:  ranges
 // CHECK-NEXT:  
 // CHECK-NEXT:
 // CHECK-NEXT: 
-// CHECK-NEXT:  line260
+// CHECK-NEXT:  line265
 // CHECK-NEXT:  col19
 // CHECK-NEXT:  file0
 // CHECK-NEXT: 
 // CHECK-NEXT: 
-// CHECK-NEXT:  line260
+// CHECK-NEXT:  line265
 // CHECK-NEXT:  col38
 // CHECK-NEXT:  file0
 // CHECK-NEXT: 
@@ -4709,20 +4714,20 @@
 // CHECK-NEXT:  kindevent
 // CHECK-NEXT:  location
 // CHECK-NEXT:  
-// CHECK-NEXT:   line260
+// CHECK-NEXT:   line265
 // CHECK-NEXT:   col19
 // CHECK-NEXT:   file0
 // CHECK-NEXT:  
 // CHECK-NEXT:  ranges
 // CHECK-NEXT:  
 // CHECK-NEXT:
 // CHECK-NEXT: 
-// CHECK-NEXT:  line260
+// CHECK-NEXT:  line265
 // CHECK-NEXT:  col19
 // CHECK-NEXT:  file0
 // CHECK-NEXT: 
 // CHECK-NEXT: 
-// CHECK-NEXT:  line260
+// CHECK-NEXT:  line265
 // CHECK-NEXT:  col38
 // CHECK-NEXT:  file0
 // CHECK-NEXT: 
@@ -4746,7 +4751,7 @@
 // CHECK-NEXT:   issue_hash_function_offset2
 // CHECK-NEXT:   location
 // CHECK-NEXT:   
-// CHECK-NEXT:line260
+// CHECK-NEXT:line265
 // CHECK-NEXT:col19
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
@@ -4762,25 +4767,25 @@
 // CHECK-NEXT: start
 // CHECK-NEXT:  
 // CHECK-NEXT:   
-// CHECK-NEXT:line259
+// CHECK-NEXT:line264
 // CHECK-NEXT:col3
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
 // CHECK-NEXT:   
-// CHECK-NEXT:line259
+// CHECK-NEXT:line264
 // CHECK-NEXT:col9
 // CHECK-NEXT:file0
 // CHECK-NEXT:   
 // CHECK-NEXT: