[nullsafe][bug fix] Introduce a param guiding optimistic treatment of third party params and fix a bug in the current behavior.

Summary:
We were wrongly not supressing third-party calls in
case third party repo is provided: there was a bug in the function is_modelled_externally.
because of this we accidentally stopped supressing third party and
started to advertise it in non-strict mode.

we do plan doing that, but little bit further ahead :)

So let's add a param guiding this.

Reviewed By: artempyanykh

Differential Revision: D18658271

fbshipit-source-id: 703f2675b

infer/man/man1/infer-full.txt

@@ -1544,6 +1544,12 @@ INTERNAL OPTIONS
        --nullable-annotation-name-reset
            Cancel the effect of --nullable-annotation-name.
 
+       --no-nullsafe-optimistic-third-party-params-in-non-strict
+           Deactivates: Nullsafe: in this mode we treat non annotated third
+           party method params as if they were annotated as nullable.
+           (Conversely:
+           --nullsafe-optimistic-third-party-params-in-non-strict)
+
        --nullsafe-strict-containers
            Activates: Warn when containers are used with nullable keys or
            values (Conversely: --no-nullsafe-strict-containers)

infer/src/base/Config.ml

@@ -1750,6 +1750,19 @@ and nullable_annotation =
   CLOpt.mk_string_opt ~long:"nullable-annotation-name" "Specify custom nullable annotation name"
 
 
+and nullsafe_optimistic_third_party_params_in_non_strict =
+  CLOpt.mk_bool
+    ~long:
+      "nullsafe-optimistic-third-party-params-in-non-strict"
+      (* Turned on for compatibility reasons.
+         Historically this is because there was no actionable way to change third party annotations.
+         Now that we have such a support, this behavior should be reconsidered, provided
+         our tooling and error reporting is friendly enough to be smoothly used by developers.
+      *) ~default:true
+    "Nullsafe: in this mode we treat non annotated third party method params as if they were \
+     annotated as nullable."
+
+
 and nullsafe_third_party_signatures =
   CLOpt.mk_string_opt ~long:"nullsafe-third-party-signatures"
     "Path to a folder with annotated signatures of third-party methods to be taken into account by \
@@ -3013,6 +3026,10 @@ and nelseg = !nelseg
 
 and nullable_annotation = !nullable_annotation
 
+and nullsafe_optimistic_third_party_params_in_non_strict =
+  !nullsafe_optimistic_third_party_params_in_non_strict
+
+
 and nullsafe_third_party_signatures = !nullsafe_third_party_signatures
 
 and nullsafe_third_party_location_for_messaging_only =

infer/src/base/Config.mli

@@ -498,6 +498,8 @@ val nullable_annotation : string option
 
 val nullsafe : bool
 
+val nullsafe_optimistic_third_party_params_in_non_strict : bool
+
 val nullsafe_third_party_signatures : string option
 
 val nullsafe_third_party_location_for_messaging_only : string option

infer/src/nullsafe/eradicateChecks.ml

@@ -427,13 +427,11 @@ let is_marked_third_party_in_config proc_name =
 
 
 (* if this method belongs to a third party code, but is not modelled neigher internally nor externally *)
-let is_third_party_without_model proc_name =
+let is_third_party_without_model proc_name model_source =
   let is_third_party =
     is_third_party_via_sig_files proc_name || is_marked_third_party_in_config proc_name
   in
-  is_third_party
+  is_third_party && Option.is_none model_source
-  && (not (Models.is_modelled_for_nullability_as_internal proc_name))
-  && not (Models.is_modelled_for_nullability_as_external proc_name)
 
 
 (** Check the parameters of a call. *)
@@ -471,16 +469,12 @@ let check_call_parameters ~is_strict_mode ~callee_annotated_signature tenv find_
       let rhs = InferredNullability.get_nullability nullability_actual in
       Result.iter_error (AssignmentRule.check ~is_strict_mode ~lhs ~rhs) ~f:report
   in
-  (* Currently, in a non-strict mode, Nullsafe does not check calls to unknown third-party functions, i.e.
-     we explicitly assume all params can accept null.
-     Historically this is because there was no actionable way to change third party annotations.
-     Now that we have such a support, this behavior might be reconsidered, provided
-     our tooling and error reporting is friendly enough to be smoothly used by developers.
-  *)
   let should_ignore_parameters_check =
     (* TODO(T52947663) model params in third-party non modelled method as a dedicated nullability type,
        so this logic can be moved to [AssignmentRule.check] *)
-    (not is_strict_mode) && is_third_party_without_model callee_pname
+    (not is_strict_mode) && Config.nullsafe_optimistic_third_party_params_in_non_strict
+    && is_third_party_without_model callee_pname
+         callee_annotated_signature.AnnotatedSignature.model_source
   in
   if not should_ignore_parameters_check then
     (* left to right to avoid guessing the different lengths *)

infer/src/nullsafe/models.ml

@@ -101,31 +101,6 @@ let get_modelled_annotated_signature tenv proc_attributes =
   annotated_signature |> correct_by_internal_models |> correct_by_external_models
 
 
-(** Return true when the procedure has been modelled for nullability. *)
-let is_modelled_for_nullability_as_internal proc_name =
-  (* TODO: get rid of this function, and propagate this information in get_modelled_annotated_signature instead
-     to avoid double calculation and make the code more clear.
-  *)
-  let proc_id = Typ.Procname.to_unique_id proc_name in
-  try
-    ignore (Hashtbl.find annotated_table_nullability proc_id) ;
-    true
-  with Caml.Not_found -> false
-
-
-(** Return true when the procedure has been modelled for nullability as external third-party code. *)
-let is_modelled_for_nullability_as_external proc_name =
-  (* TODO: get rid of this function, and propagate this information in get_modelled_annotated_signature instead
-     to avoid double calculation and make the code more clear.
-  *)
-  get_unique_repr proc_name
-  |> Option.map
-       ~f:
-         (ThirdPartyAnnotationInfo.find_nullability_info
-            (ThirdPartyAnnotationGlobalRepo.get_repo ()))
-  |> Option.is_some
-
-
 (** Check if the procedure is one of the known Preconditions.checkNotNull. *)
 let is_check_not_null proc_name =
   table_has_procedure check_not_null_table proc_name || match_method_name proc_name "checkNotNull"