From 58913e6a1c46d0e9584e68e12db4a00fe8b23839 Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Mon, 17 Feb 2020 08:38:42 -0800 Subject: [PATCH] [Infer] Dedup reports of weakSelf in Noescape block Summary: Once we identify a weakSelf variable that is being used in a Noescape block, we want to report only the first occurrence. Reviewed By: skcho Differential Revision: D19941502 fbshipit-source-id: 2b6d4648b --- infer/src/checkers/SelfInBlock.ml | 54 ++++++++++++------- .../objc/self-in-block/NoescapeBlock.m | 1 + .../objc/self-in-block/issues.exp | 3 +- 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/infer/src/checkers/SelfInBlock.ml b/infer/src/checkers/SelfInBlock.ml index 51e3acd6e..1d61f1fe4 100644 --- a/infer/src/checkers/SelfInBlock.ml +++ b/infer/src/checkers/SelfInBlock.ml @@ -134,9 +134,10 @@ module Domain = struct end type report_issues_result = - { weakSelfList: DomainData.t list + { reported_captured_strong_self: Pvar.Set.t + ; reported_weak_self_in_noescape_block: Pvar.Set.t ; selfList: DomainData.t list - ; reported_captured_strong_self: Pvar.Set.t } + ; weakSelfList: DomainData.t list } module TransferFunctions = struct module Domain = Domain @@ -436,16 +437,23 @@ let report_mix_self_weakself_issues summary domain (weakSelf : DomainData.t) (se Reporting.log_error summary ~ltr ~loc:self.loc IssueType.mixed_self_weakself message -let report_weakself_in_no_escape_block_issues summary domain (weakSelf : DomainData.t) procname = - let message = - F.asprintf - "This block uses `%a` at %a. This is probably not needed since the block is passed to the \ - method `%s` in a position annotated with NS_NOESCAPE. Use `self` instead." - (Pvar.pp Pp.text) weakSelf.pvar Location.pp weakSelf.loc - (Procname.to_simplified_string procname) - in - let ltr = make_trace_use_self_weakself domain in - Reporting.log_error summary ~ltr ~loc:weakSelf.loc IssueType.weak_self_in_noescape_block message +let report_weakself_in_no_escape_block_issues summary domain (weakSelf : DomainData.t) procname + reported_weak_self_in_noescape_block = + if not (Pvar.Set.mem weakSelf.pvar reported_weak_self_in_noescape_block) then ( + let reported_weak_self_in_noescape_block = + Pvar.Set.add weakSelf.pvar reported_weak_self_in_noescape_block + in + let message = + F.asprintf + "This block uses `%a` at %a. This is probably not needed since the block is passed to the \ + method `%s` in a position annotated with NS_NOESCAPE. Use `self` instead." + (Pvar.pp Pp.text) weakSelf.pvar Location.pp weakSelf.loc + (Procname.to_simplified_string procname) + in + let ltr = make_trace_use_self_weakself domain in + Reporting.log_error summary ~ltr ~loc:weakSelf.loc IssueType.weak_self_in_noescape_block message ; + reported_weak_self_in_noescape_block ) + else reported_weak_self_in_noescape_block let report_weakself_multiple_issue summary domain (weakSelf1 : DomainData.t) @@ -493,19 +501,27 @@ let report_issues summary domain attributes = in {result with reported_captured_strong_self} | DomainData.WEAK_SELF -> - ( match attributes.ProcAttributes.passed_as_noescape_block_to with - | Some procname -> - report_weakself_in_no_escape_block_issues summary domain domain_data procname - | None -> - () ) ; - {result with weakSelfList= domain_data :: result.weakSelfList} + let reported_weak_self_in_noescape_block = + match attributes.ProcAttributes.passed_as_noescape_block_to with + | Some procname -> + report_weakself_in_no_escape_block_issues summary domain domain_data procname + result.reported_weak_self_in_noescape_block + | None -> + result.reported_weak_self_in_noescape_block + in + { result with + reported_weak_self_in_noescape_block + ; weakSelfList= domain_data :: result.weakSelfList } | DomainData.SELF -> {result with selfList= domain_data :: result.selfList} | _ -> result in let report_issues_result_empty = - {weakSelfList= []; selfList= []; reported_captured_strong_self= Pvar.Set.empty} + { reported_captured_strong_self= Pvar.Set.empty + ; reported_weak_self_in_noescape_block= Pvar.Set.empty + ; selfList= [] + ; weakSelfList= [] } in let domain_bindings = Vars.bindings domain diff --git a/infer/tests/codetoanalyze/objc/self-in-block/NoescapeBlock.m b/infer/tests/codetoanalyze/objc/self-in-block/NoescapeBlock.m index 4e5662cd9..b7d1e1391 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/NoescapeBlock.m +++ b/infer/tests/codetoanalyze/objc/self-in-block/NoescapeBlock.m @@ -34,6 +34,7 @@ __weak __typeof(self) weakSelf = self; [allResults enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL* stop) { B* result = [weakSelf process:obj]; // bug + B* result1 = [weakSelf process:obj]; // no bug because of dedup if (result != nil) { [resultsList addObject:result]; } diff --git a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp index 47cd75cec..4f5535c68 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp +++ b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp @@ -1,4 +1,5 @@ -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/NoescapeBlock.m, objc_blockA::weak_in_noescape_block_bad:_1, 1, MULTIPLE_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf] +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,Using &weakSelf] codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::mixSelfWeakSelf_bad_1, 4, MIXED_SELF_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &self,Using &self] codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheck2_bad_6, 6, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null,Using &strongSelf not checked for null] codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheck3_bad_9, 2, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null]