From a705373e02f78cf836383c74fd49d965bedec29e Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Tue, 19 Nov 2019 04:00:21 -0800 Subject: [PATCH] [nullsafe] ThirdPartyAnnotationInfoStorage stores files names and line numbers for signatures Summary: In the follow up diff we will use it to render more informative error message when type error is relevant to the fact that the code is third-party Reviewed By: artempyanykh Differential Revision: D18569783 fbshipit-source-id: 4f6dcc404 --- .../src/nullsafe/ThirdPartyAnnotationInfo.ml | 28 +++++++---- .../src/nullsafe/ThirdPartyAnnotationInfo.mli | 11 ++-- .../ThirdPartyAnnotationInfoLoader.ml | 3 +- infer/src/nullsafe/models.ml | 3 +- .../nullsafe/ThirdPartyAnnotationInfoTests.ml | 50 ++++++++++++------- 5 files changed, 62 insertions(+), 33 deletions(-) diff --git a/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml b/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml index 0720c8500..4e6b1311f 100644 --- a/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml +++ b/infer/src/nullsafe/ThirdPartyAnnotationInfo.ml @@ -7,9 +7,13 @@ open! IStd module Hashtbl = Caml.Hashtbl -type storage = (ThirdPartyMethod.unique_repr, ThirdPartyMethod.nullability) Hashtbl.t +type signature_info = {filename: string; line_number: int; nullability: ThirdPartyMethod.nullability} -let create_storage () = Hashtbl.create 1 +type storage = {signature_map: signature_map; filenames: string list} + +and signature_map = (ThirdPartyMethod.unique_repr, signature_info) Hashtbl.t + +let create_storage () = {signature_map= Hashtbl.create 1; filenames= []} type file_parsing_error = {line_number: int; unparsable_method: string; parsing_error: ThirdPartyMethod.parsing_error} @@ -28,21 +32,25 @@ let bind_list_with_index ~init list ~f = Result.bind acc ~f:(fun acc -> f acc index elem) ) -let parse_line_and_add_to_storage storage _line_index line = +let parse_line_and_add_to_storage signature_map ~filename ~line_index line = let open Result in ThirdPartyMethod.parse line >>= fun (signature, nullability) -> Ok - ( Hashtbl.add storage signature nullability ; - storage ) + ( Hashtbl.add signature_map signature {filename; line_number= line_index + 1; nullability} ; + signature_map ) -let add_from_signature_file storage ~lines = +let add_from_signature_file storage ~filename ~lines = (* each line in a file should represent a method signature *) - bind_list_with_index lines ~init:storage ~f:(fun storage index method_as_str -> - parse_line_and_add_to_storage storage index method_as_str + let open Result in + let new_filenames = storage.filenames @ [filename] in + bind_list_with_index lines ~init:storage.signature_map + ~f:(fun signature_map line_index method_as_str -> + parse_line_and_add_to_storage signature_map ~filename ~line_index method_as_str |> Result.map_error ~f:(fun parsing_error -> - {line_number= index + 1; unparsable_method= method_as_str; parsing_error} ) ) + {line_number= line_index + 1; unparsable_method= method_as_str; parsing_error} ) ) + >>= fun new_map -> Ok {signature_map= new_map; filenames= new_filenames} -let find_nullability_info storage unique_repr = Hashtbl.find_opt storage unique_repr +let find_nullability_info {signature_map} unique_repr = Hashtbl.find_opt signature_map unique_repr diff --git a/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli b/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli index 793fb6ca9..2400a4c66 100644 --- a/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli +++ b/infer/src/nullsafe/ThirdPartyAnnotationInfo.mli @@ -8,6 +8,11 @@ open! IStd (** In-memory storage the information about nullability annotation of third-party methods. *) +type signature_info = + { filename: string (** File where the particular signature is stored *) + ; line_number: int (** Line number with this signature *) + ; nullability: ThirdPartyMethod.nullability } + type storage val create_storage : unit -> storage @@ -17,11 +22,11 @@ type file_parsing_error = val pp_parsing_error : Format.formatter -> file_parsing_error -> unit -val add_from_signature_file : storage -> lines:string list -> (storage, file_parsing_error) result +val add_from_signature_file : + storage -> filename:string -> lines:string list -> (storage, file_parsing_error) result (** Parse the information from the signature file, and add it to the storage *) -val find_nullability_info : - storage -> ThirdPartyMethod.unique_repr -> ThirdPartyMethod.nullability option +val find_nullability_info : storage -> ThirdPartyMethod.unique_repr -> signature_info option (** The main method. Do we have an information about the third-party method? If we do not, or it is not a third-party method, returns None. Otherwise returns the nullability information. diff --git a/infer/src/nullsafe/ThirdPartyAnnotationInfoLoader.ml b/infer/src/nullsafe/ThirdPartyAnnotationInfoLoader.ml index 670ae8a57..644457bcf 100644 --- a/infer/src/nullsafe/ThirdPartyAnnotationInfoLoader.ml +++ b/infer/src/nullsafe/ThirdPartyAnnotationInfoLoader.ml @@ -15,9 +15,8 @@ let pp_load_error fmt {filename; parsing_error} = let add_from_file storage ~path_to_repo_dir ~sig_file = let lines = In_channel.read_lines (path_to_repo_dir ^ "/" ^ sig_file) in - ThirdPartyAnnotationInfo.add_from_signature_file storage ~lines + ThirdPartyAnnotationInfo.add_from_signature_file storage ~filename:sig_file ~lines |> Result.map_error ~f:(fun parsing_error -> {filename= sig_file; parsing_error}) - |> Result.bind ~f:(fun _ -> Ok storage) let load ~path_to_repo_dir = diff --git a/infer/src/nullsafe/models.ml b/infer/src/nullsafe/models.ml index bdc10d17a..a3b1b5889 100644 --- a/infer/src/nullsafe/models.ml +++ b/infer/src/nullsafe/models.ml @@ -85,7 +85,8 @@ let get_modelled_annotated_signature tenv proc_attributes = ~f: (ThirdPartyAnnotationInfo.find_nullability_info (ThirdPartyAnnotationGlobalRepo.get_repo ())) - |> Option.map ~f:to_modelled_nullability + |> Option.map ~f:(fun ThirdPartyAnnotationInfo.{nullability} -> + to_modelled_nullability nullability ) |> Option.value_map (* If we found information in third-party repo, overwrite annotated signature *) ~f:(AnnotatedSignature.set_modelled_nullability proc_name ann_sig) diff --git a/infer/src/unit/nullsafe/ThirdPartyAnnotationInfoTests.ml b/infer/src/unit/nullsafe/ThirdPartyAnnotationInfoTests.ml index 094a47f91..dab8ca718 100644 --- a/infer/src/unit/nullsafe/ThirdPartyAnnotationInfoTests.ml +++ b/infer/src/unit/nullsafe/ThirdPartyAnnotationInfoTests.ml @@ -9,32 +9,37 @@ open! IStd open OUnit2 module F = Format -let assert_has_nullability_info storage unique_repr ~expected_nullability = +let assert_has_nullability_info ?expected_file ?expected_line storage unique_repr + ~expected_nullability = match ThirdPartyAnnotationInfo.find_nullability_info storage unique_repr with | None -> assert_failure (F.asprintf "Expected to find info for %a, but it was not found" ThirdPartyMethod.pp_unique_repr unique_repr) - | Some nullability -> + | Some {filename; line_number; nullability} -> assert_equal expected_nullability nullability ~msg: (F.asprintf "Nullability info for %a does not match" ThirdPartyMethod.pp_unique_repr unique_repr) - ~printer:(Pp.string_of_pp ThirdPartyMethod.pp_nullability) + ~printer:(Pp.string_of_pp ThirdPartyMethod.pp_nullability) ; + Option.iter expected_file ~f:(fun expected_file -> + assert_equal expected_file filename ~msg:"Filename does not match" ) ; + Option.iter expected_line ~f:(fun expected_line -> + assert_equal expected_line line_number ~msg:"Line number does not match" ) let assert_no_info storage unique_repr = match ThirdPartyAnnotationInfo.find_nullability_info storage unique_repr with | None -> () - | Some nullability -> + | Some {nullability} -> assert_failure (F.asprintf "Did not expect to find nullability info for method %a, but found %a" ThirdPartyMethod.pp_unique_repr unique_repr ThirdPartyMethod.pp_nullability nullability) -let add_from_annot_file_and_check_success storage ~lines = - match ThirdPartyAnnotationInfo.add_from_signature_file storage ~lines with +let add_from_annot_file_and_check_success storage ~filename ~lines = + match ThirdPartyAnnotationInfo.add_from_signature_file storage ~filename ~lines with | Ok storage -> storage | Error parsing_error -> @@ -43,8 +48,8 @@ let add_from_annot_file_and_check_success storage ~lines = ThirdPartyAnnotationInfo.pp_parsing_error parsing_error) -let add_from_annot_file_and_check_failure storage ~lines ~expected_error_line_number = - match ThirdPartyAnnotationInfo.add_from_signature_file storage ~lines with +let add_from_annot_file_and_check_failure storage ~filename ~lines ~expected_error_line_number = + match ThirdPartyAnnotationInfo.add_from_signature_file storage ~filename ~lines with | Ok _ -> assert_failure "Expected to not be able to parse the file, but it was successfully parsed instead" @@ -60,15 +65,19 @@ let basic_find = let lines = ["a.A#foo(b.B)"; "b.B#bar(c.C, @Nullable d.D) @Nullable"] in (* Load some functions from the file *) let storage = - add_from_annot_file_and_check_success (ThirdPartyAnnotationInfo.create_storage ()) ~lines + add_from_annot_file_and_check_success ~filename:"test.sig" + (ThirdPartyAnnotationInfo.create_storage ()) + ~lines in (* Make sure we can find what we just stored *) assert_has_nullability_info storage {class_name= "a.A"; method_name= Method "foo"; param_types= ["b.B"]} - ~expected_nullability:{ret_nullability= Nonnull; param_nullability= [Nonnull]} ; + ~expected_nullability:{ret_nullability= Nonnull; param_nullability= [Nonnull]} + ~expected_file:"test.sig" ~expected_line:1 ; assert_has_nullability_info storage {class_name= "b.B"; method_name= Method "bar"; param_types= ["c.C"; "d.D"]} - ~expected_nullability:{ret_nullability= Nullable; param_nullability= [Nonnull; Nullable]} ; + ~expected_nullability:{ret_nullability= Nullable; param_nullability= [Nonnull; Nullable]} + ~expected_file:"test.sig" ~expected_line:2 ; (* Make sure we can not find stuff we did not store *) (* Wrong class name *) assert_no_info storage {class_name= "a.AB"; method_name= Method "foo"; param_types= ["b.B"]} ; @@ -97,7 +106,9 @@ let overload_resolution = in (* Load some functions from the file *) let storage = - add_from_annot_file_and_check_success (ThirdPartyAnnotationInfo.create_storage ()) ~lines + add_from_annot_file_and_check_success ~filename:"test.sig" + (ThirdPartyAnnotationInfo.create_storage ()) + ~lines in (* Make sure we can find what we just stored *) (* a.b.SomeClass.foo with 1 param *) @@ -153,21 +164,26 @@ let can_add_several_files = (* 1. Add file and check if we added info *) let file1 = ["a.A#foo(b.B)"; "b.B#bar(c.C, @Nullable d.D) @Nullable"] in let storage = - add_from_annot_file_and_check_success (ThirdPartyAnnotationInfo.create_storage ()) ~lines:file1 + add_from_annot_file_and_check_success + (ThirdPartyAnnotationInfo.create_storage ()) + ~filename:"file1.sig" ~lines:file1 in assert_has_nullability_info storage {class_name= "a.A"; method_name= Method "foo"; param_types= ["b.B"]} - ~expected_nullability:{ret_nullability= Nonnull; param_nullability= [Nonnull]} ; + ~expected_nullability:{ret_nullability= Nonnull; param_nullability= [Nonnull]} + ~expected_file:"file1.sig" ~expected_line:1 ; (* 2. Add another file and check if we added info *) let file2 = ["e.E#baz(f.F)"; "g.G#(h.H, @Nullable i.I) @Nullable"] in - let storage = add_from_annot_file_and_check_success storage ~lines:file2 in + let storage = add_from_annot_file_and_check_success storage ~filename:"file2.sig" ~lines:file2 in assert_has_nullability_info storage {class_name= "e.E"; method_name= Method "baz"; param_types= ["f.F"]} - ~expected_nullability:{ret_nullability= Nonnull; param_nullability= [Nonnull]} ; + ~expected_nullability:{ret_nullability= Nonnull; param_nullability= [Nonnull]} + ~expected_file:"file2.sig" ~expected_line:1 ; (* 3. Ensure we did not forget the content from the first file *) assert_has_nullability_info storage {class_name= "a.A"; method_name= Method "foo"; param_types= ["b.B"]} ~expected_nullability:{ret_nullability= Nonnull; param_nullability= [Nonnull]} + ~expected_file:"file1.sig" ~expected_line:1 let should_not_forgive_unparsable_strings = @@ -183,7 +199,7 @@ let should_not_forgive_unparsable_strings = (* Ensure we can add the good file, but can not add the bad one *) add_from_annot_file_and_check_success (ThirdPartyAnnotationInfo.create_storage ()) ~lines:file_ok |> ignore ; - add_from_annot_file_and_check_failure + add_from_annot_file_and_check_failure ~filename:"test.sig" (ThirdPartyAnnotationInfo.create_storage ()) ~lines:file_bad ~expected_error_line_number:2