From 761d2c56afbf06fd5f11fbf621b79a01976c5bcf Mon Sep 17 00:00:00 2001 From: Sungkeun Cho Date: Wed, 12 Aug 2020 02:10:24 -0700 Subject: [PATCH] [cost] Separate purity analysis and reporting Summary: This diff separates purity analysis and its reporting, since sometimes we want to use the purity analysis results in other checkers, but don't want to report purity issues. Reviewed By: ezgicicek, jvillard Differential Revision: D23054913 fbshipit-source-id: 12cc1fc42 --- infer/src/backend/registerCheckers.ml | 38 +++++++++++-------- infer/src/base/Checker.ml | 17 +++++++-- infer/src/base/Checker.mli | 3 +- infer/src/base/IssueType.ml | 2 +- .../checkers/{purity.ml => PurityAnalysis.ml} | 29 -------------- .../{purity.mli => PurityAnalysis.mli} | 2 - infer/src/checkers/PurityChecker.ml | 38 +++++++++++++++++++ infer/src/checkers/PurityChecker.mli | 12 ++++++ infer/src/checkers/impurity.ml | 5 ++- 9 files changed, 91 insertions(+), 55 deletions(-) rename infer/src/checkers/{purity.ml => PurityAnalysis.ml} (87%) rename infer/src/checkers/{purity.mli => PurityAnalysis.mli} (89%) create mode 100644 infer/src/checkers/PurityChecker.ml create mode 100644 infer/src/checkers/PurityChecker.mli diff --git a/infer/src/backend/registerCheckers.ml b/infer/src/backend/registerCheckers.ml index 99e4c25bd..c0c3f52e5 100644 --- a/infer/src/backend/registerCheckers.ml +++ b/infer/src/backend/registerCheckers.ml @@ -77,11 +77,31 @@ let all_checkers = (* The order of the list is important for those checkers that depend on other checkers having run before them. *) [ {checker= SelfInBlock; callbacks= [(intraprocedural SelfInBlock.checker, Clang)]} - ; { checker= Purity + ; { checker= BufferOverrunAnalysis + ; callbacks= + (let bo_analysis = + interprocedural Payloads.Fields.buffer_overrun_analysis + BufferOverrunAnalysis.analyze_procedure + in + [(bo_analysis, Clang); (bo_analysis, Java)] ) } + ; { checker= BufferOverrunChecker + ; callbacks= + (let bo_checker = + interprocedural2 Payloads.Fields.buffer_overrun_checker + Payloads.Fields.buffer_overrun_analysis BufferOverrunChecker.checker + in + [(bo_checker, Clang); (bo_checker, Java)] ) } + ; { checker= PurityAnalysis ; callbacks= (let purity = interprocedural2 Payloads.Fields.purity Payloads.Fields.buffer_overrun_analysis - Purity.checker + PurityAnalysis.checker + in + [(purity, Java); (purity, Clang)] ) } + ; { checker= PurityChecker + ; callbacks= + (let purity = + intraprocedural_with_field_dependency Payloads.Fields.purity PurityChecker.checker in [(purity, Java); (purity, Clang)] ) } ; { checker= Starvation @@ -161,20 +181,6 @@ let all_checkers = [ (intraprocedural_with_payload Payloads.Fields.nullsafe Eradicate.analyze_procedure, Java) ; (file NullsafeFileIssues Payloads.Fields.nullsafe FileLevelAnalysis.analyze_file, Java) ] } - ; { checker= BufferOverrunChecker - ; callbacks= - (let bo_checker = - interprocedural2 Payloads.Fields.buffer_overrun_checker - Payloads.Fields.buffer_overrun_analysis BufferOverrunChecker.checker - in - [(bo_checker, Clang); (bo_checker, Java)] ) } - ; { checker= BufferOverrunAnalysis - ; callbacks= - (let bo_analysis = - interprocedural Payloads.Fields.buffer_overrun_analysis - BufferOverrunAnalysis.analyze_procedure - in - [(bo_analysis, Clang); (bo_analysis, Java)] ) } ; { checker= Biabduction ; callbacks= (let biabduction = diff --git a/infer/src/base/Checker.ml b/infer/src/base/Checker.ml index 24a450eb4..8f6f7c83d 100644 --- a/infer/src/base/Checker.ml +++ b/infer/src/base/Checker.ml @@ -27,7 +27,8 @@ type t = | NullsafeDeprecated | PrintfArgs | Pulse - | Purity + | PurityAnalysis + | PurityChecker | Quandary | RacerD | ResourceLeakLabExercise @@ -252,7 +253,7 @@ let config_unsafe checker = for efficiency." ; cli_flags= Some {deprecated= []; show_in_help= true} ; enabled_by_default= false - ; activates= [BufferOverrunAnalysis; Purity] } + ; activates= [BufferOverrunAnalysis; PurityAnalysis] } | NullsafeDeprecated -> { id= "nullsafe" ; kind= Internal @@ -285,7 +286,15 @@ let config_unsafe checker = ; cli_flags= Some {deprecated= ["-ownership"]; show_in_help= true} ; enabled_by_default= false ; activates= [] } - | Purity -> + | PurityAnalysis -> + { id= "purity-analysis" + ; kind= Internal + ; support= supports_clang_and_java_experimental + ; short_documentation= "Internal part of the purity checker." + ; cli_flags= None + ; enabled_by_default= false + ; activates= [BufferOverrunAnalysis] } + | PurityChecker -> { id= "purity" ; kind= UserFacing @@ -295,7 +304,7 @@ let config_unsafe checker = "Detects pure (side-effect-free) functions. A different implementation of \"impurity\"." ; cli_flags= Some {deprecated= []; show_in_help= true} ; enabled_by_default= false - ; activates= [BufferOverrunAnalysis] } + ; activates= [PurityAnalysis] } | Quandary -> { id= "quandary" ; kind= diff --git a/infer/src/base/Checker.mli b/infer/src/base/Checker.mli index 7e8560aae..20b689d7d 100644 --- a/infer/src/base/Checker.mli +++ b/infer/src/base/Checker.mli @@ -26,7 +26,8 @@ type t = | NullsafeDeprecated | PrintfArgs | Pulse - | Purity + | PurityAnalysis + | PurityChecker | Quandary | RacerD | ResourceLeakLabExercise diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index a6a866cb6..a20b2177a 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -775,7 +775,7 @@ let pulse_memory_leak = let pure_function = - register ~id:"PURE_FUNCTION" Error Purity + register ~id:"PURE_FUNCTION" Error PurityChecker ~user_documentation:[%blob "../../documentation/issues/PURE_FUNCTION.md"] diff --git a/infer/src/checkers/purity.ml b/infer/src/checkers/PurityAnalysis.ml similarity index 87% rename from infer/src/checkers/purity.ml rename to infer/src/checkers/PurityAnalysis.ml index de7195488..fc3aea206 100644 --- a/infer/src/checkers/purity.ml +++ b/infer/src/checkers/PurityAnalysis.ml @@ -179,34 +179,6 @@ end module Analyzer = LowerHil.MakeAbstractInterpreter (TransferFunctions) -let should_report proc_name = - not - ( Procname.is_constructor proc_name - || - match proc_name with - | Procname.Java java_pname -> - Procname.Java.is_class_initializer java_pname || Procname.Java.is_access_method java_pname - | Procname.ObjC_Cpp name -> - Procname.ObjC_Cpp.is_destructor name - || Procname.ObjC_Cpp.is_objc_constructor name.method_name - | _ -> - false ) - - -let report_errors {InterproceduralAnalysis.proc_desc; err_log} astate_opt = - let proc_name = Procdesc.get_proc_name proc_desc in - match astate_opt with - | Some astate -> - if should_report proc_name && PurityDomain.is_pure astate then - let loc = Procdesc.get_loc proc_desc in - let exp_desc = F.asprintf "Side-effect free function %a" Procname.pp proc_name in - let ltr = [Errlog.make_trace_element 0 loc exp_desc []] in - Reporting.log_issue proc_desc err_log ~loc ~ltr Purity IssueType.pure_function exp_desc - | None -> - L.internal_error "Analyzer failed to compute purity information for %a@." Procname.pp - proc_name - - let compute_summary {InterproceduralAnalysis.proc_desc; tenv; analyze_dependency} inferbo_invariant_map = let proc_name = Procdesc.get_proc_name proc_desc in @@ -227,6 +199,5 @@ let checker analysis_data = (InterproceduralAnalysis.bind_payload ~f:snd analysis_data) in let astate_opt = compute_summary analysis_data inferbo_invariant_map in - report_errors analysis_data astate_opt ; Option.iter astate_opt ~f:(fun astate -> debug "Purity summary :%a \n" PurityDomain.pp astate) ; astate_opt diff --git a/infer/src/checkers/purity.mli b/infer/src/checkers/PurityAnalysis.mli similarity index 89% rename from infer/src/checkers/purity.mli rename to infer/src/checkers/PurityAnalysis.mli index 0de65b24c..7dead3f49 100644 --- a/infer/src/checkers/purity.mli +++ b/infer/src/checkers/PurityAnalysis.mli @@ -10,5 +10,3 @@ open! IStd val checker : (PurityDomain.summary option * BufferOverrunAnalysisSummary.t option) InterproceduralAnalysis.t -> PurityDomain.summary option - -val should_report : Procname.t -> bool diff --git a/infer/src/checkers/PurityChecker.ml b/infer/src/checkers/PurityChecker.ml new file mode 100644 index 000000000..e5857bef2 --- /dev/null +++ b/infer/src/checkers/PurityChecker.ml @@ -0,0 +1,38 @@ +(* + * 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. + *) + +open! IStd +module F = Format +module L = Logging + +let should_report proc_name = + not + ( Procname.is_constructor proc_name + || + match proc_name with + | Procname.Java java_pname -> + Procname.Java.is_class_initializer java_pname || Procname.Java.is_access_method java_pname + | Procname.ObjC_Cpp name -> + Procname.ObjC_Cpp.is_destructor name + || Procname.ObjC_Cpp.is_objc_constructor name.method_name + | _ -> + false ) + + +let checker {IntraproceduralAnalysis.proc_desc; err_log} astate_opt = + let proc_name = Procdesc.get_proc_name proc_desc in + match astate_opt with + | Some astate -> + if should_report proc_name && PurityDomain.is_pure astate then + let loc = Procdesc.get_loc proc_desc in + let exp_desc = F.asprintf "Side-effect free function %a" Procname.pp proc_name in + let ltr = [Errlog.make_trace_element 0 loc exp_desc []] in + Reporting.log_issue proc_desc err_log ~loc ~ltr PurityChecker IssueType.pure_function + exp_desc + | None -> + L.internal_error "Analyzer failed to compute purity information for %a@." Procname.pp + proc_name diff --git a/infer/src/checkers/PurityChecker.mli b/infer/src/checkers/PurityChecker.mli new file mode 100644 index 000000000..60d8ae273 --- /dev/null +++ b/infer/src/checkers/PurityChecker.mli @@ -0,0 +1,12 @@ +(* + * 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. + *) + +open! IStd + +val should_report : Procname.t -> bool + +val checker : IntraproceduralAnalysis.t -> PurityDomain.summary option -> unit diff --git a/infer/src/checkers/impurity.ml b/infer/src/checkers/impurity.ml index 9ae14e9f1..aa2cff7ac 100644 --- a/infer/src/checkers/impurity.ml +++ b/infer/src/checkers/impurity.ml @@ -155,7 +155,8 @@ let extract_impurity tenv pdesc (exec_state : ExecutionDomain.t) : ImpurityDomai let modified_globals = get_modified_globals pre_heap post post_stack in let skipped_calls = SkippedCalls.filter - (fun proc_name _ -> Purity.should_report proc_name && not (is_modeled_pure tenv proc_name)) + (fun proc_name _ -> + PurityChecker.should_report proc_name && not (is_modeled_pure tenv proc_name) ) astate.AbductiveDomain.skipped_calls in {modified_globals; modified_params; skipped_calls; exited} @@ -185,7 +186,7 @@ let checker {IntraproceduralAnalysis.proc_desc; tenv; err_log} pulse_summary_opt let modified = extract_impurity tenv proc_desc exec_state in ImpurityDomain.join acc modified ) in - if Purity.should_report proc_name && not (ImpurityDomain.is_pure impurity_astate) then + if PurityChecker.should_report proc_name && not (ImpurityDomain.is_pure impurity_astate) then let modified_ltr param_source set acc = ImpurityDomain.ModifiedVarSet.fold (ImpurityDomain.add_to_errlog ~nesting:1 param_source)