From 4bc4376c85bcc200019b48b2452d975a78a90955 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Tue, 11 Aug 2020 06:24:41 -0700 Subject: [PATCH] [nullsafe][refactor] Migrate parts of Nullsafe to Procname.Java.t Summary: This is part of work aimed to reduce usage of language-agnostics modules in Java-specific parts of nullsafe. This diff: 1/ migrates AssignmentRule 2/ passes Procname.Java.t around from the start of analysis. For now we have places where we have both procname and java procname. This is fine as we can get rid of it in follow up diffs. Reviewed By: artempyanykh Differential Revision: D23052226 fbshipit-source-id: 0125de297 --- infer/src/IR/Procname.ml | 8 ++++ infer/src/IR/Procname.mli | 3 ++ infer/src/nullsafe/AssignmentRule.ml | 12 +++--- infer/src/nullsafe/AssignmentRule.mli | 4 +- .../src/nullsafe/ThirdPartyAnnotationInfo.ml | 18 +++++---- .../src/nullsafe/ThirdPartyAnnotationInfo.mli | 3 ++ infer/src/nullsafe/eradicate.ml | 37 ++++++++++++------- infer/src/nullsafe/eradicateChecks.ml | 25 +++++-------- infer/src/nullsafe/typeCheck.ml | 6 +-- infer/src/nullsafe/typeErr.ml | 2 +- 10 files changed, 69 insertions(+), 49 deletions(-) diff --git a/infer/src/IR/Procname.ml b/infer/src/IR/Procname.ml index b6a073e9e..72bbab9d8 100644 --- a/infer/src/IR/Procname.ml +++ b/infer/src/IR/Procname.ml @@ -393,6 +393,14 @@ let with_block_parameters base blocks = WithBlockParameters (base, blocks) let is_java = function Java _ -> true | _ -> false +let as_java_exn ~explanation t = + match t with + | Java java -> + java + | _ -> + Logging.die InternalError "Expected Java procname: %s" explanation + + (* TODO: deprecate this unfortunately named function and use is_clang instead *) let is_c_method = function ObjC_Cpp _ -> true | _ -> false diff --git a/infer/src/IR/Procname.mli b/infer/src/IR/Procname.mli index ae0432202..8d80db03f 100644 --- a/infer/src/IR/Procname.mli +++ b/infer/src/IR/Procname.mli @@ -292,6 +292,9 @@ val is_constructor : t -> bool val is_java : t -> bool (** Check if this is a Java procedure name. *) +val as_java_exn : explanation:string -> t -> Java.t +(** Converts to a Java.t. Throws if [is_java] is false *) + val with_block_parameters : t -> Block.block_name list -> t (** Create a procedure name instantiated with block parameters from a base procedure name and a list of block procedure names (the arguments). *) diff --git a/infer/src/nullsafe/AssignmentRule.ml b/infer/src/nullsafe/AssignmentRule.ml index 324d28abd..42030f38c 100644 --- a/infer/src/nullsafe/AssignmentRule.ml +++ b/infer/src/nullsafe/AssignmentRule.ml @@ -14,7 +14,7 @@ module ReportableViolation = struct type assignment_type = | PassingParamToFunction of function_info | AssigningToField of Fieldname.t - | ReturningFromFunction of Procname.t + | ReturningFromFunction of Procname.Java.t [@@deriving compare] and function_info = @@ -22,7 +22,7 @@ module ReportableViolation = struct ; kind: AnnotatedSignature.kind ; actual_param_expression: string ; param_position: int - ; function_procname: Procname.t } + ; function_procname: Procname.Java.t } let from nullsafe_mode ({lhs; rhs} as violation) = let falls_under_optimistic_third_party = @@ -94,7 +94,7 @@ module ReportableViolation = struct So we phrase it differently to remain truthful, but as specific as possible. *) let suggested_third_party_sig_file = - ThirdPartyAnnotationInfo.lookup_related_sig_file_for_proc + ThirdPartyAnnotationInfo.lookup_related_sig_file_for_java_proc (ThirdPartyAnnotationGlobalRepo.get_repo ()) function_procname in @@ -106,7 +106,7 @@ module ReportableViolation = struct (* this can happen when third party is registered in a deprecated way (not in third party repository) *) ~default:"the third party signature storage" in - let procname_str = Procname.to_simplified_string ~withclass:true function_procname in + let procname_str = Procname.Java.to_simplified_string ~withclass:true function_procname in Format.asprintf "Third-party %a is missing a signature that would allow passing a nullable to param \ #%d%a. Actual argument %s%s. Consider adding the correct signature of %a to %s." @@ -131,7 +131,7 @@ module ReportableViolation = struct in Format.asprintf "%a: parameter #%d%a is declared non-nullable%s but the argument %s%s." MF.pp_monospaced - (Procname.to_simplified_string ~withclass:true function_procname) + (Procname.Java.to_simplified_string ~withclass:true function_procname) param_position pp_param_name param_signature.mangled nonnull_evidence argument_description nullability_evidence_as_suffix @@ -193,7 +193,7 @@ module ReportableViolation = struct in Format.asprintf "%a: return type is declared non-nullable but the method returns %s%s.%s" MF.pp_monospaced - (Procname.to_simplified_string ~withclass:false function_proc_name) + (Procname.Java.to_simplified_string ~withclass:false function_proc_name) return_description nullability_evidence_as_suffix alternative_recommendation in let issue_type = get_issue_type assignment_type in diff --git a/infer/src/nullsafe/AssignmentRule.mli b/infer/src/nullsafe/AssignmentRule.mli index 8fdc03a52..bd7fea9f9 100644 --- a/infer/src/nullsafe/AssignmentRule.mli +++ b/infer/src/nullsafe/AssignmentRule.mli @@ -27,7 +27,7 @@ module ReportableViolation : sig type assignment_type = | PassingParamToFunction of function_info | AssigningToField of Fieldname.t - | ReturningFromFunction of Procname.t + | ReturningFromFunction of Procname.Java.t [@@deriving compare] and function_info = @@ -35,7 +35,7 @@ module ReportableViolation : sig ; kind: AnnotatedSignature.kind ; actual_param_expression: string ; param_position: int - ; function_procname: Procname.t } + ; function_procname: Procname.Java.t } val get_severity : t -> IssueType.severity (** Severity of the violation to be reported *) diff --git a/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml b/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml index 967ef2245..2728b91d1 100644 --- a/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml +++ b/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml @@ -103,17 +103,19 @@ let lookup_related_sig_file {filenames} ~package = |> List.max_elt ~compare:(fun name1 name2 -> String.length name1 - String.length name2) -let lookup_related_sig_file_for_proc storage procname = - let package = - match procname with - | Procname.Java java_pname -> - Procname.Java.get_package java_pname - | _ -> - None - in +let lookup_related_sig_file_for_java_proc storage procname = + let package = Procname.Java.get_package procname in Option.bind package ~f:(fun package -> lookup_related_sig_file storage ~package) +let lookup_related_sig_file_for_proc storage procname = + match procname with + | Procname.Java java_pname -> + lookup_related_sig_file_for_java_proc storage java_pname + | _ -> + None + + let is_third_party_proc storage procname = let is_from_config = match procname with diff --git a/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli b/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli index eb92f5ed1..578effda3 100644 --- a/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli +++ b/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli @@ -50,6 +50,9 @@ val lookup_related_sig_file : storage -> package:string -> string option val lookup_related_sig_file_for_proc : storage -> Procname.t -> string option (** If the function is third-party (based on its package), return relevant .sig file *) +val lookup_related_sig_file_for_java_proc : storage -> Procname.Java.t -> string option +(** If the function is third-party (based on its package), return relevant .sig file *) + val is_third_party_proc : storage -> Procname.t -> bool (** Checks whether a required procname comes from third-party code based on available .sig files and config flags. NOTE: considering config flags is done for compatibility with the legacy behaviour diff --git a/infer/src/nullsafe/eradicate.ml b/infer/src/nullsafe/eradicate.ml index e9ee6dec9..a4b2cde3e 100644 --- a/infer/src/nullsafe/eradicate.ml +++ b/infer/src/nullsafe/eradicate.ml @@ -12,7 +12,7 @@ module F = Format (* make sure that this is initialized *) let () = NullsafeInit.init () -let callback1 ({IntraproceduralAnalysis.proc_desc= curr_pdesc; _} as analysis_data) +let callback1 ({IntraproceduralAnalysis.proc_desc= curr_pdesc; _} as analysis_data) ~curr_java_pname find_canonical_duplicate calls_this checks idenv annotated_signature linereader proc_loc : bool * TypeState.t option = let curr_pname = Procdesc.get_proc_name curr_pdesc in @@ -39,8 +39,8 @@ let callback1 ({IntraproceduralAnalysis.proc_desc= curr_pdesc; _} as analysis_da List.fold ~f:add_formal ~init:typestate_empty annotated_signature.AnnotatedSignature.params in (* Check the nullable flag computed for the return value and report inconsistencies. *) - let check_return find_canonical_duplicate exit_node final_typestate annotated_signature loc : unit - = + let check_return ~java_pname find_canonical_duplicate exit_node final_typestate + annotated_signature loc : unit = let {AnnotatedSignature.ret_annotated_type} = annotated_signature.AnnotatedSignature.ret in let ret_pvar = Procdesc.get_ret_var curr_pdesc in let ret_range = TypeState.lookup_pvar ret_pvar final_typestate in @@ -57,16 +57,17 @@ let callback1 ({IntraproceduralAnalysis.proc_desc= curr_pdesc; _} as analysis_da ~f:(fun f -> f curr_pdesc ret_annotated_type.typ typ_found_opt loc) checks.TypeCheck.check_ret_type ; if checks.TypeCheck.eradicate then - EradicateChecks.check_return_annotation analysis_data find_canonical_duplicate ret_range - annotated_signature ret_implicitly_nullable loc + EradicateChecks.check_return_annotation analysis_data ~java_pname find_canonical_duplicate + ret_range annotated_signature ret_implicitly_nullable loc in let do_before_dataflow initial_typestate = if Config.eradicate_verbose then L.result "Initial Typestate@\n%a@." TypeState.pp initial_typestate in - let do_after_dataflow find_canonical_duplicate final_typestate = + let do_after_dataflow ~java_pname find_canonical_duplicate final_typestate = let exit_node = Procdesc.get_exit_node curr_pdesc in - check_return find_canonical_duplicate exit_node final_typestate annotated_signature proc_loc + check_return ~java_pname find_canonical_duplicate exit_node final_typestate annotated_signature + proc_loc in let module DFTypeCheck = DataFlow.MakeDF (struct type t = TypeState.t @@ -98,14 +99,15 @@ let callback1 ({IntraproceduralAnalysis.proc_desc= curr_pdesc; _} as analysis_da let transitions = DFTypeCheck.run curr_pdesc initial_typestate in match transitions (Procdesc.get_exit_node curr_pdesc) with | DFTypeCheck.Transition (final_typestate, _, _) -> - do_after_dataflow find_canonical_duplicate final_typestate ; + do_after_dataflow ~java_pname:curr_java_pname find_canonical_duplicate final_typestate ; (!calls_this, Some final_typestate) | DFTypeCheck.Dead_state -> (!calls_this, None) -let analyze_one_procedure ({IntraproceduralAnalysis.tenv; proc_desc; _} as analysis_data) calls_this - checks annotated_signature linereader proc_loc : unit = +let analyze_one_procedure ~java_pname + ({IntraproceduralAnalysis.tenv; proc_desc; _} as analysis_data) calls_this checks + annotated_signature linereader proc_loc : unit = let idenv = IDEnv.create proc_desc in let find_duplicate_nodes = State.mk_find_duplicate_nodes proc_desc in let find_canonical_duplicate node = @@ -153,10 +155,12 @@ let analyze_one_procedure ({IntraproceduralAnalysis.tenv; proc_desc; _} as analy && checks.TypeCheck.eradicate then ( let typestates_for_curr_constructor_and_all_initializer_methods = - Initializers.final_initializer_typestates_lazy tenv proc_name proc_desc typecheck_proc + Initializers.final_initializer_typestates_lazy tenv proc_name proc_desc + (typecheck_proc ~curr_java_pname:java_pname) in let typestates_for_all_constructors_incl_current = - Initializers.final_constructor_typestates_lazy tenv proc_name typecheck_proc + Initializers.final_constructor_typestates_lazy tenv proc_name + (typecheck_proc ~curr_java_pname:java_pname) in EradicateChecks.check_constructor_initialization analysis_data find_canonical_duplicate start_node ~nullsafe_mode:annotated_signature.AnnotatedSignature.nullsafe_mode @@ -167,7 +171,8 @@ let analyze_one_procedure ({IntraproceduralAnalysis.tenv; proc_desc; _} as analy match typestate_opt with None -> () | Some typestate -> do_typestate typestate in let calls_this, final_typestate_opt = - typecheck_proc true proc_name proc_desc (Some (annotated_signature, proc_loc, idenv)) + typecheck_proc ~curr_java_pname:java_pname true proc_name proc_desc + (Some (annotated_signature, proc_loc, idenv)) in do_final_typestate final_typestate_opt calls_this ; if checks.TypeCheck.eradicate then @@ -206,6 +211,9 @@ let analyze checks ({IntraproceduralAnalysis.proc_desc; tenv; _} as analysis_dat | None -> (* start the analysis! *) let calls_this = ref false in + let java_pname = + Procname.as_java_exn ~explanation:"Would have skipped the analysis otherwise" proc_name + in let annotated_signature = AnnotatedSignature.get_for_class_under_analysis tenv (Procdesc.get_attributes proc_desc) in @@ -218,7 +226,8 @@ let analyze checks ({IntraproceduralAnalysis.proc_desc; tenv; _} as analysis_dat (* The main method - during this the actual analysis will happen and TypeErr will be populated with issues (and some of them - reported). *) - analyze_one_procedure analysis_data calls_this checks annotated_signature linereader loc ; + analyze_one_procedure ~java_pname analysis_data calls_this checks annotated_signature + linereader loc ; (* Collect issues that were detected during analysis and put them in summary for further processing *) let issues = TypeErr.get_errors () |> List.map ~f:(fun (issues, _) -> issues) in (* Report errors of "farall" class - those could not be reported during analysis phase. *) diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 6688960df..0f7a9be88 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -325,19 +325,17 @@ let check_constructor_initialization () -let check_return_not_nullable ({IntraproceduralAnalysis.proc_desc= curr_pdesc; _} as analysis_data) - ~nullsafe_mode find_canonical_duplicate loc (ret_signature : AnnotatedSignature.ret_signature) - ret_inferred_nullability = +let check_return_not_nullable analysis_data ~nullsafe_mode ~java_pname find_canonical_duplicate loc + (ret_signature : AnnotatedSignature.ret_signature) ret_inferred_nullability = (* Returning from a function is essentially an assignment the actual return value to the formal `return` *) let lhs = ret_signature.ret_annotated_type.nullability in let rhs = ret_inferred_nullability in Result.iter_error (AssignmentRule.check ~lhs ~rhs) ~f:(fun assignment_violation -> - let curr_pname = Procdesc.get_proc_name curr_pdesc in TypeErr.register_error analysis_data find_canonical_duplicate (Bad_assignment { assignment_violation ; assignment_location= loc - ; assignment_type= ReturningFromFunction curr_pname }) + ; assignment_type= ReturningFromFunction java_pname }) None ~nullsafe_mode loc ) @@ -360,20 +358,18 @@ let check_return_overrannotated (** Check the annotations when returning from a method. *) -let check_return_annotation ({IntraproceduralAnalysis.proc_desc= curr_pdesc; _} as analysis_data) - find_canonical_duplicate ret_range (annotated_signature : AnnotatedSignature.t) - ret_implicitly_nullable loc : unit = - let curr_pname = Procdesc.get_proc_name curr_pdesc in +let check_return_annotation analysis_data ~java_pname find_canonical_duplicate ret_range + (annotated_signature : AnnotatedSignature.t) ret_implicitly_nullable loc : unit = match ret_range with (* Disables the warnings since it is not clear how to annotate the return value of lambdas *) - | Some _ - when match curr_pname with Java java_pname -> Procname.Java.is_lambda java_pname | _ -> false -> + | Some _ when Procname.Java.is_lambda java_pname -> () | Some (_, ret_inferred_nullability) -> (* TODO(T54308240) Model ret_implicitly_nullable in AnnotatedNullability *) if not ret_implicitly_nullable then - check_return_not_nullable analysis_data ~nullsafe_mode:annotated_signature.nullsafe_mode - find_canonical_duplicate loc annotated_signature.ret ret_inferred_nullability ; + check_return_not_nullable ~java_pname analysis_data + ~nullsafe_mode:annotated_signature.nullsafe_mode find_canonical_duplicate loc + annotated_signature.ret ret_inferred_nullability ; if Config.eradicate_return_over_annotated then check_return_overrannotated analysis_data find_canonical_duplicate loc annotated_signature.ret ~nullsafe_mode:annotated_signature.nullsafe_mode @@ -408,9 +404,8 @@ type resolved_param = (** Check the parameters of a call. *) let check_call_parameters ({IntraproceduralAnalysis.tenv; _} as analysis_data) ~nullsafe_mode - ~callee_annotated_signature find_canonical_duplicate node callee_attributes resolved_params loc + ~callee_pname ~callee_annotated_signature find_canonical_duplicate node resolved_params loc instr_ref : unit = - let callee_pname = callee_attributes.ProcAttributes.proc_name in let check {num= param_position; formal; actual= orig_e2, nullability_actual} = let report ~nullsafe_mode assignment_violation = let actual_param_expression = diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index 7521d4140..bb2f6808c 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/infer/src/nullsafe/typeCheck.ml @@ -1061,9 +1061,9 @@ let calc_typestate_after_call node typestate1 call_params callee_pname_java instr_ref loc (typecheck_expr analysis_data ~nullsafe_mode find_canonical_duplicate calls_this checks) ; if checks.eradicate then - EradicateChecks.check_call_parameters analysis_data ~nullsafe_mode - ~callee_annotated_signature find_canonical_duplicate node callee_attributes - resolved_params loc instr_ref ; + EradicateChecks.check_call_parameters ~callee_pname:callee_pname_java analysis_data + ~nullsafe_mode ~callee_annotated_signature find_canonical_duplicate node resolved_params + loc instr_ref ; if Models.is_check_not_null callee_pname then match Models.get_check_not_null_parameter callee_pname with | Some index -> diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index 28617cd3c..8b67cf8ab 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/infer/src/nullsafe/typeErr.ml @@ -101,7 +101,7 @@ let is_synthetic_err = function | PassingParamToFunction {function_procname; _} -> (* NOTE: this currently doesn't cover the case when the actual argument involves syntethic code *) - Procname.is_java_autogen_method function_procname + Procname.Java.is_autogen_method function_procname | AssigningToField fieldname -> Fieldname.is_java_synthetic fieldname | _ ->