[SelfInBlock] Check for when weakSelf is used in a no escaping block

Summary: This adds a check for when developers use weakSelf (and maybe strongSelf) in a block, when there is no need for it, because it won't cause a retain cycle. In general there is an annotation in Objective-C for methods that take blocks, NS_NOESCAPE, that means that the passed block won't leave the scope, i.e. is "no escaping".  So we report when weakSelf is used and the block has the annotation.

Reviewed By: ngorogiannis

Differential Revision: D19662468

fbshipit-source-id: f5ac695aa

infer/man/man1/infer-full.txt

@@ -515,6 +515,7 @@ OPTIONS
              USE_AFTER_LIFETIME (enabled by default),
              Unknown_proc (enabled by default),
              VECTOR_INVALIDATION (enabled by default),
+             WEAK_SELF_IN_NO_ESCAPE_BLOCK (enabled by default),
              Wrong_argument_number (enabled by default),
              ZERO_ALLOCATION (disabled by default),
              ZERO_EXECUTION_TIME (disabled by default).

infer/man/man1/infer-report.txt

@@ -249,6 +249,7 @@ OPTIONS
              USE_AFTER_LIFETIME (enabled by default),
              Unknown_proc (enabled by default),
              VECTOR_INVALIDATION (enabled by default),
+             WEAK_SELF_IN_NO_ESCAPE_BLOCK (enabled by default),
              Wrong_argument_number (enabled by default),
              ZERO_ALLOCATION (disabled by default),
              ZERO_EXECUTION_TIME (disabled by default).

infer/man/man1/infer.txt

@@ -515,6 +515,7 @@ OPTIONS
              USE_AFTER_LIFETIME (enabled by default),
              Unknown_proc (enabled by default),
              VECTOR_INVALIDATION (enabled by default),
+             WEAK_SELF_IN_NO_ESCAPE_BLOCK (enabled by default),
              Wrong_argument_number (enabled by default),
              ZERO_ALLOCATION (disabled by default),
              ZERO_EXECUTION_TIME (disabled by default).

infer/src/base/IssueType.ml

@@ -498,6 +498,8 @@ let untrusted_variable_length_array = register_from_string "UNTRUSTED_VARIABLE_L
 
 let vector_invalidation = register_from_string "VECTOR_INVALIDATION"
 
+let weak_self_in_noescape_block = register_from_string "WEAK_SELF_IN_NO_ESCAPE_BLOCK"
+
 let wrong_argument_number =
   register_from_string "Wrong_argument_number" ~hum:"Wrong Argument Number"
 

infer/src/base/IssueType.mli

@@ -347,6 +347,8 @@ val user_controlled_sql_risk : t
 
 val vector_invalidation : t
 
+val weak_self_in_noescape_block : t
+
 val wrong_argument_number : t
 
 val zero_cost_call : kind:CostKind.t -> t

infer/src/checkers/SelfInBlock.ml

@@ -398,6 +398,25 @@ let report_mix_self_weakself_issues summary domain =
       ()
 
 
+let report_weakself_in_no_escape_block_issues summary domain attributes =
+  if attributes.ProcAttributes.is_no_escape_block then
+    let weakSelf_opt =
+      Vars.filter (fun _ {kind} -> DomainData.is_weak_self kind) domain |> Vars.choose_opt
+    in
+    match weakSelf_opt with
+    | Some (_, {pvar= weakSelf; loc= weakLoc}) ->
+        let message =
+          F.asprintf
+            "This block uses `%a` at %a. This is probably not needed since the block is passed to \
+             a method in a position annotated with NS_NOESCAPE. Use `self` instead."
+            (Pvar.pp Pp.text) weakSelf Location.pp weakLoc
+        in
+        let ltr = make_trace_use_self_weakself domain in
+        Reporting.log_error summary ~ltr ~loc:weakLoc IssueType.weak_self_in_noescape_block message
+    | _ ->
+        ()
+
+
 let report_weakself_multiple_issues summary domain =
   let weakSelfs =
     Vars.filter (fun _ {kind} -> DomainData.is_weak_self kind) domain
@@ -467,12 +486,14 @@ let checker {Callbacks.exe_env; summary} =
   let procname = Summary.get_proc_name summary in
   let tenv = Exe_env.get_tenv exe_env procname in
   let proc_data = ProcData.make summary tenv () in
+  let attributes = Summary.get_attributes summary in
   ( if Procname.is_objc_block procname then
     match Analyzer.compute_post proc_data ~initial with
     | Some domain ->
         report_mix_self_weakself_issues summary domain.vars ;
         report_weakself_multiple_issues summary domain.vars ;
-        report_captured_strongself_issues summary domain.vars
+        report_captured_strongself_issues summary domain.vars ;
+        report_weakself_in_no_escape_block_issues summary domain.vars attributes
     | None ->
         () ) ;
   summary

infer/tests/codetoanalyze/objc/self-in-block/NoescapeBlock.m

@@ -0,0 +1,65 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#import <Foundation/Foundation.h>
+
+@interface B : NSObject
+@end
+
+@interface ArrayUtils : NSObject
+
++ (void)enumerate:(void (^)(id obj, NSUInteger idx, BOOL* stop))block;
+@end
+
+@interface A : NSObject
+- (NSMutableArray*)allResultsList:(NSArray<B*>*)allResults;
+
+- (B*)process:(B*)obj;
+@end
+
+@implementation A
+
+- (B*)process:(B*)obj {
+  return obj;
+}
+
+- (NSMutableArray<B*>*)weak_in_noescape_block_bad:(NSArray<B*>*)allResults {
+  NSMutableArray<B*>* resultsList = [[NSMutableArray<B*> alloc] init];
+  __weak __typeof(self) weakSelf = self;
+  [allResults enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL* stop) {
+    B* result = [weakSelf process:obj]; // bug
+    if (result != nil) {
+      [resultsList addObject:result];
+    }
+  }];
+  return resultsList;
+}
+
+- (NSMutableArray<B*>*)weak_in_noescape_block_good:(NSArray<B*>*)allResults {
+  NSMutableArray<B*>* resultsList = [[NSMutableArray<B*> alloc] init];
+  [allResults enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL* stop) {
+    B* result = [self process:obj]; // no bug
+    if (result != nil) {
+      [resultsList addObject:result];
+    }
+  }];
+  return resultsList;
+}
+
+- (NSMutableArray<B*>*)weak_in_noescape_block1_good:(NSArray<B*>*)allResults {
+  NSMutableArray<B*>* resultsList = [[NSMutableArray<B*> alloc] init];
+  __weak __typeof(self) weakSelf = self;
+  [ArrayUtils enumerate:^(id obj, NSUInteger idx, BOOL* stop) {
+    B* result = [weakSelf process:obj]; // no bug
+    if (result != nil) {
+      [resultsList addObject:result];
+    }
+  }];
+  return resultsList;
+}
+
+@end

infer/tests/codetoanalyze/objc/self-in-block/issues.exp

@@ -1,3 +1,4 @@
+codetoanalyze/objc/self-in-block/NoescapeBlock.m, objc_blockA::weak_in_noescape_block_bad:_1, 1, WEAK_SELF_IN_NO_ESCAPE_BLOCK, no_bucket, ERROR, [Using &weakSelf]
 codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::mixSelfWeakSelf_bad_1, 5, MIXED_SELF_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &self,Using &self]
 codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheck3_bad_7, 2, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null]
 codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheckOnce_bad_5, 6, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null,Using &strongSelf not checked for null]