[nullsafe] Make ErrorRenderingUtils `None`-safe

Summary:
# Problem

Yes, nullsafe is not null-safe, such an irony.

ErrorRenderingUtils overuses `option` and `let+` constructions. Most of
internal functions can return `None` when "something is wrong".

On top of this, "default" pattern match is overused either.

Because of this, `ErrorRenderingUtils.mk_nullsafe_special_issue` returns
optional type. In practice, this result can be None for many unclear
reasons, and it is super tricky to even understand them all.

This in turn forced AssignmentRule and DereferenceRule to process this
None is defensive way. The rules have some theory why None was returned,
and have assertions along the way.

Turns out those theories might be wrong. This diff will make triaging wrong assumptions easier.

Reviewed By: artempyanykh

Differential Revision: D20535720

fbshipit-source-id: 2b81e25b7

infer/src/nullsafe/AssignmentRule.ml

@@ -63,7 +63,7 @@ module ReportableViolation = struct
 
   let mk_description_for_bad_param_passed
       {model_source; param_signature; actual_param_expression; param_position; function_procname}
-      ~param_nullability ~nullability_evidence =
+      ~param_nullability_kind ~nullability_evidence =
     let nullability_evidence_as_suffix =
       Option.value_map nullability_evidence ~f:(fun evidence -> ": " ^ evidence) ~default:""
     in
@@ -73,15 +73,11 @@ module ReportableViolation = struct
       if String.equal actual_param_expression "null" then "is `null`"
       else
         let nullability_descr =
-          match param_nullability with
+          match param_nullability_kind with
-          | Nullability.Null ->
+          | ErrorRenderingUtils.UserFriendlyNullable.Null ->
               "`null`"
-          | Nullability.Nullable ->
+          | ErrorRenderingUtils.UserFriendlyNullable.Nullable ->
               "nullable"
-          | other ->
-              Logging.die InternalError
-                "mk_description_for_bad_param:: invariant violation: unexpected nullability %a"
-                Nullability.pp other
         in
         Format.asprintf "%a is %s" MF.pp_monospaced actual_param_expression nullability_descr
     in
@@ -147,18 +143,8 @@ module ReportableViolation = struct
         IssueType.eradicate_return_not_nullable
 
 
-  let get_description ~assignment_location assignment_type ~rhs_origin
+  let mk_nullsafe_issue_for_explicitly_nullable_values ~assignment_type ~rhs_origin
-      {nullsafe_mode; violation= {rhs}} =
+      ~explicit_rhs_nullable_kind ~assignment_location =
-    let special_message =
-      if not (NullsafeMode.equal NullsafeMode.Default nullsafe_mode) then
-        ErrorRenderingUtils.mk_special_nullsafe_issue ~nullsafe_mode ~bad_nullability:rhs
-          ~bad_usage_location:assignment_location rhs_origin
-      else None
-    in
-    match special_message with
-    | Some desc ->
-        desc
-    | _ ->
     let nullability_evidence =
       get_origin_opt assignment_type rhs_origin
       |> Option.bind ~f:(fun origin -> TypeOrigin.get_description origin)
@@ -181,48 +167,57 @@ module ReportableViolation = struct
       | PassingParamToFunction function_info ->
           Format.sprintf "%s%s"
             (mk_description_for_bad_param_passed function_info ~nullability_evidence
-                   ~param_nullability:rhs)
+               ~param_nullability_kind:explicit_rhs_nullable_kind)
             alternative_recommendation
       | AssigningToField field_name ->
           let rhs_description =
-                Nullability.(
+            match explicit_rhs_nullable_kind with
-                  match rhs with
+            | ErrorRenderingUtils.UserFriendlyNullable.Null ->
-                  | Null ->
                 "`null`"
-                  | Nullable ->
+            | ErrorRenderingUtils.UserFriendlyNullable.Nullable ->
                 "a nullable"
-                  | other ->
-                      Logging.die InternalError
-                        "violation_description(assign_field):: invariant violation: unexpected \
-                         nullability %a"
-                        Nullability.pp other)
           in
           Format.asprintf "%a is declared non-nullable but is assigned %s%s.%s" MF.pp_monospaced
             (Fieldname.get_field_name field_name)
             rhs_description nullability_evidence_as_suffix alternative_recommendation
       | ReturningFromFunction function_proc_name ->
           let return_description =
-                Nullability.(
+            match explicit_rhs_nullable_kind with
-                  match rhs with
+            | ErrorRenderingUtils.UserFriendlyNullable.Null ->
-                  | Null ->
                 (* Return `null` in all_whitelisted branches *)
                 "`null`"
-                  | Nullable ->
+            | ErrorRenderingUtils.UserFriendlyNullable.Nullable ->
                 "a nullable value"
-                  | other ->
-                      Logging.die InternalError
-                        "violation_description(ret_fun):: invariant violation: unexpected \
-                         nullability %a"
-                        Nullability.pp other)
           in
-              Format.asprintf
+          Format.asprintf "%a: return type is declared non-nullable but the method returns %s%s.%s"
-                "%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)
             return_description nullability_evidence_as_suffix alternative_recommendation
     in
     let issue_type = get_issue_type assignment_type in
     (error_message, issue_type, assignment_location)
+
+
+  let get_description ~assignment_location assignment_type ~rhs_origin
+      {nullsafe_mode; violation= {rhs}} =
+    let user_friendly_nullable =
+      ErrorRenderingUtils.UserFriendlyNullable.from_nullability rhs
+      |> IOption.if_none_eval ~f:(fun () ->
+             Logging.die InternalError
+               "get_description:: Assignment violation should not be possible for non-nullable \
+                values on right hand side" )
+    in
+    match user_friendly_nullable with
+    | ErrorRenderingUtils.UserFriendlyNullable.UntrustedNonnull untrusted_kind ->
+        (* Attempt to assigning a value which is not explictly declared as nullable,
+           but still can not be trusted in this particular mode.
+        *)
+        ErrorRenderingUtils.mk_nullsafe_issue_for_untrusted_values ~nullsafe_mode ~untrusted_kind
+          ~bad_usage_location:assignment_location rhs_origin
+    | ErrorRenderingUtils.UserFriendlyNullable.ExplainablyNullable explicit_kind ->
+        (* Attempt to assigning a value that can be explained to the user as nullable. *)
+        mk_nullsafe_issue_for_explicitly_nullable_values ~assignment_type ~rhs_origin
+          ~explicit_rhs_nullable_kind:explicit_kind ~assignment_location
 end
 
 let check ~lhs ~rhs =

infer/src/nullsafe/DereferenceRule.ml

@@ -34,19 +34,9 @@ module ReportableViolation = struct
     if should_show_origin then Some origin else None
 
 
-  let get_description {nullsafe_mode; violation= {nullability}} ~dereference_location
+  let mk_nullsafe_issue_for_explicitly_nullable_values ~explicit_kind ~dereference_type
-      dereference_type ~nullable_object_descr ~nullable_object_origin =
+      dereference_location ~nullable_object_descr ~nullable_object_origin =
     let module MF = MarkupFormatter in
-    let special_message =
-      if not (NullsafeMode.equal NullsafeMode.Default nullsafe_mode) then
-        ErrorRenderingUtils.mk_special_nullsafe_issue ~nullsafe_mode ~bad_nullability:nullability
-          ~bad_usage_location:dereference_location nullable_object_origin
-      else None
-    in
-    match special_message with
-    | Some desc ->
-        desc
-    | _ ->
     let what_is_dereferred_str =
       match dereference_type with
       | MethodCall _ | AccessToField _ -> (
@@ -92,23 +82,41 @@ module ReportableViolation = struct
         ~default:""
     in
     let description =
-          match nullability with
+      match explicit_kind with
-          | Nullability.Null ->
+      | ErrorRenderingUtils.UserFriendlyNullable.Null ->
           Format.sprintf
-                "NullPointerException will be thrown at this line! %s is `null` and is \
+            "NullPointerException will be thrown at this line! %s is `null` and is dereferenced \
-                 dereferenced via %s%s."
+             via %s%s."
             what_is_dereferred_str action_descr origin_descr
-          | Nullability.Nullable ->
+      | ErrorRenderingUtils.UserFriendlyNullable.Nullable ->
           Format.sprintf "%s is nullable and is not locally checked for null when %s%s.%s"
             what_is_dereferred_str action_descr origin_descr alternative_recommendation
-          | other ->
-              Logging.die InternalError
-                "violation_description:: invariant violation: unexpected nullability %a"
-                Nullability.pp other
     in
     (description, IssueType.eradicate_nullable_dereference, dereference_location)
 
 
+  let get_description {nullsafe_mode; violation= {nullability}} ~dereference_location
+      dereference_type ~nullable_object_descr ~nullable_object_origin =
+    let user_friendly_nullable =
+      ErrorRenderingUtils.UserFriendlyNullable.from_nullability nullability
+      |> IOption.if_none_eval ~f:(fun () ->
+             Logging.die InternalError
+               "get_description:: Dereference violation should not be possible for non-nullable \
+                values" )
+    in
+    match user_friendly_nullable with
+    | ErrorRenderingUtils.UserFriendlyNullable.UntrustedNonnull untrusted_kind ->
+        (* Attempt to dereference a value which is not explictly declared as nullable,
+           but still can not be trusted in this particular mode.
+        *)
+        ErrorRenderingUtils.mk_nullsafe_issue_for_untrusted_values ~nullsafe_mode ~untrusted_kind
+          ~bad_usage_location:dereference_location nullable_object_origin
+    | ErrorRenderingUtils.UserFriendlyNullable.ExplainablyNullable explicit_kind ->
+        (* Attempt to dereference value that can be explained to the user as nullable. *)
+        mk_nullsafe_issue_for_explicitly_nullable_values ~explicit_kind ~dereference_type
+          dereference_location ~nullable_object_descr ~nullable_object_origin
+
+
   let get_severity {nullsafe_mode} = NullsafeMode.severity nullsafe_mode
 end
 

infer/src/nullsafe/ErrorRenderingUtils.ml

@@ -8,6 +8,28 @@
 open! IStd
 module F = Format
 
+module UserFriendlyNullable = struct
+  type t = ExplainablyNullable of explainably_nullable_kind | UntrustedNonnull of untrusted_kind
+
+  and explainably_nullable_kind = Nullable | Null
+
+  and untrusted_kind = ThirdPartyNonnull | UncheckedNonnull | LocallyCheckedNonnull
+
+  let from_nullability = function
+    | Nullability.Nullable ->
+        Some (ExplainablyNullable Nullable)
+    | Nullability.Null ->
+        Some (ExplainablyNullable Null)
+    | Nullability.UncheckedNonnull ->
+        Some (UntrustedNonnull UncheckedNonnull)
+    | Nullability.LocallyCheckedNonnull ->
+        Some (UntrustedNonnull LocallyCheckedNonnull)
+    | Nullability.ThirdPartyNonnull ->
+        Some (UntrustedNonnull ThirdPartyNonnull)
+    | Nullability.StrictNonnull ->
+        None
+end
+
 let is_object_nullability_self_explanatory ~object_expression (object_origin : TypeOrigin.t) =
   (* Fundamentally, object can be of two kinds:
      1. Indirect: local variable that was instantiated before.
@@ -83,28 +105,40 @@ let get_field_class_name field_name =
   |> Option.value_map ~f:(fun (classname, _) -> classname) ~default:"the field class"
 
 
-let mk_coming_from_unchecked_or_locally_checked_case_only nullsafe_mode nullability =
+let mk_coming_from_unchecked_or_locally_checked_case_only nullsafe_mode untrusted_kind =
-  match (nullsafe_mode, nullability) with
+  match (nullsafe_mode, untrusted_kind) with
-  | NullsafeMode.Strict, Nullability.UncheckedNonnull ->
+  | NullsafeMode.Strict, UserFriendlyNullable.UncheckedNonnull ->
       "non-strict classes"
-  | NullsafeMode.Strict, Nullability.LocallyCheckedNonnull ->
+  | NullsafeMode.Strict, UserFriendlyNullable.LocallyCheckedNonnull ->
       "nullsafe-local classes"
-  | NullsafeMode.Local _, Nullability.UncheckedNonnull ->
+  | NullsafeMode.Local _, _ ->
       "non-nullsafe classes"
-  | _ ->
+  | NullsafeMode.Default, _ ->
-      Logging.die Logging.InternalError
+      Logging.die InternalError
-        "Should be called only for locally checked or unchecked nonnull cases"
+        "mk_coming_from_unchecked_or_locally_checked_case_only:: not applicable to default mode"
+  | _, UserFriendlyNullable.ThirdPartyNonnull ->
+      Logging.die InternalError
+        "mk_coming_from_unchecked_or_locally_checked_case_only:: not applicable to \
+         ThirdPartyNonnull case"
 
 
-let mk_recommendation nullsafe_mode nullability what =
+let mk_strictification_advice_unchecked_or_locally_checked_case_only nullsafe_mode untrusted_kind
-  match (nullsafe_mode, nullability) with
+    ~what_to_strictify =
-  | NullsafeMode.Strict, Nullability.UncheckedNonnull
+  match untrusted_kind with
-  | NullsafeMode.Strict, Nullability.LocallyCheckedNonnull ->
+  | UserFriendlyNullable.UncheckedNonnull | UserFriendlyNullable.LocallyCheckedNonnull -> (
-      Some (F.sprintf "make %s nullsafe strict" what)
+    match nullsafe_mode with
-  | NullsafeMode.Local _, Nullability.UncheckedNonnull ->
+    | NullsafeMode.Strict ->
-      Some (F.sprintf "make %s nullsafe" what)
+        F.sprintf "make %s nullsafe strict" what_to_strictify
-  | _ ->
+    | NullsafeMode.Local _ ->
-      None
+        F.sprintf "make %s nullsafe" what_to_strictify
+    | NullsafeMode.Default ->
+        Logging.die InternalError
+          "mk_recommendation_unchecked_or_locally_checked_case_only:: should not be called for \
+           default mode" )
+  | UserFriendlyNullable.ThirdPartyNonnull ->
+      Logging.die InternalError
+        "mk_recommendation_unchecked_or_locally_checked_case_only:: not applicable to \
+         ThirdPartyNonnull case"
 
 
 let mk_recommendation_for_third_party_field nullsafe_mode field =
@@ -114,12 +148,12 @@ let mk_recommendation_for_third_party_field nullsafe_mode field =
   | NullsafeMode.Local _ ->
       F.sprintf "access %s via a nullsafe getter" field
   | NullsafeMode.Default ->
-      Logging.die Logging.InternalError
+      Logging.die InternalError
-        "Should not happen: we should tolerate third party in default mode"
+        "mk_recommendation_for_third_party_field:: Should not happen: we should tolerate third \
+         party in default mode"
 
 
-let get_info object_origin nullsafe_mode bad_nullability =
+let get_info object_origin nullsafe_mode untrusted_kind =
-  let open IOption.Let_syntax in
   match object_origin with
   | TypeOrigin.MethodCall {pname; call_loc} ->
       let offending_object =
@@ -128,14 +162,9 @@ let get_info object_origin nullsafe_mode bad_nullability =
       in
       let object_loc = call_loc in
       let what_is_used = "Result of this call" in
-      let+ coming_from_explanation, recommendation, issue_type =
+      let coming_from_explanation, recommendation, issue_type =
-        match bad_nullability with
+        match untrusted_kind with
-        | Nullability.Null | Nullability.Nullable ->
+        | UserFriendlyNullable.ThirdPartyNonnull ->
-            (* This method makes sense only for non-nullable violations *)
-            None
-        | Nullability.StrictNonnull ->
-            Logging.die InternalError "There should not be type violations involving StrictNonnull"
-        | Nullability.ThirdPartyNonnull ->
             let suggested_third_party_sig_file =
               ThirdPartyAnnotationInfo.lookup_related_sig_file_for_proc
                 (ThirdPartyAnnotationGlobalRepo.get_repo ())
@@ -149,19 +178,19 @@ let get_info object_origin nullsafe_mode bad_nullability =
                   (* this can happen when third party is registered in a deprecated way (not in third party repository) *)
                 ~default:"the third party signature storage"
             in
-            return
             ( "not vetted third party methods"
             , F.sprintf "add the correct signature to %s" where_to_add_signature
             , IssueType.eradicate_unvetted_third_party_in_nullsafe )
-        | Nullability.UncheckedNonnull | Nullability.LocallyCheckedNonnull ->
+        | UserFriendlyNullable.UncheckedNonnull | UserFriendlyNullable.LocallyCheckedNonnull ->
             let from =
-              mk_coming_from_unchecked_or_locally_checked_case_only nullsafe_mode bad_nullability
+              mk_coming_from_unchecked_or_locally_checked_case_only nullsafe_mode untrusted_kind
             in
-            let+ recommendation =
+            let recommendation =
               let what_to_strictify =
                 Option.value (get_method_class_name pname) ~default:offending_object
               in
-              mk_recommendation nullsafe_mode bad_nullability what_to_strictify
+              mk_strictification_advice_unchecked_or_locally_checked_case_only nullsafe_mode
+                untrusted_kind ~what_to_strictify
             in
             let issue_type = IssueType.eradicate_unchecked_usage_in_nullsafe in
             (from, recommendation, issue_type)
@@ -183,24 +212,19 @@ let get_info object_origin nullsafe_mode bad_nullability =
       (* TODO: currently we do not support third-party annotations for fields. Because of this,
          render error like it is a non-stict class. *)
       let what_is_used = "This field" in
-      let+ coming_from_explanation, recommendation, issue_type =
+      let coming_from_explanation, recommendation, issue_type =
-        match bad_nullability with
+        match untrusted_kind with
-        | Nullability.Null | Nullability.Nullable ->
+        | UserFriendlyNullable.ThirdPartyNonnull ->
-            (* This method makes sense only for non-nullable violations *)
-            None
-        | Nullability.StrictNonnull ->
-            Logging.die InternalError "There should not be type violations involving StrictNonnull"
-        | Nullability.ThirdPartyNonnull ->
-            return
             ( "third-party classes"
             , mk_recommendation_for_third_party_field nullsafe_mode unqualified_name
             , IssueType.eradicate_unvetted_third_party_in_nullsafe )
-        | Nullability.UncheckedNonnull | Nullability.LocallyCheckedNonnull ->
+        | UserFriendlyNullable.UncheckedNonnull | UserFriendlyNullable.LocallyCheckedNonnull ->
             let from =
-              mk_coming_from_unchecked_or_locally_checked_case_only nullsafe_mode bad_nullability
+              mk_coming_from_unchecked_or_locally_checked_case_only nullsafe_mode untrusted_kind
             in
-            let+ recommendation =
+            let recommendation =
-              mk_recommendation nullsafe_mode bad_nullability (get_field_class_name field_name)
+              mk_strictification_advice_unchecked_or_locally_checked_case_only nullsafe_mode
+                untrusted_kind ~what_to_strictify:(get_field_class_name field_name)
             in
             (from, recommendation, IssueType.eradicate_unchecked_usage_in_nullsafe)
       in
@@ -210,19 +234,22 @@ let get_info object_origin nullsafe_mode bad_nullability =
       ; what_is_used
       ; recommendation
       ; issue_type }
-  | _ ->
+  | other ->
-      None
+      Logging.die InternalError
+        "get_info:: untrusted_kind is possible only for MethodCall and Field origins, got %s \
+         instead"
+        (TypeOrigin.to_string other)
 
 
-let mk_special_nullsafe_issue ~nullsafe_mode ~bad_nullability ~bad_usage_location object_origin =
+let mk_nullsafe_issue_for_untrusted_values ~nullsafe_mode ~untrusted_kind ~bad_usage_location
-  let open IOption.Let_syntax in
+    object_origin =
-  let+ { offending_object
+  let { offending_object
       ; object_loc
       ; coming_from_explanation
       ; what_is_used
       ; recommendation
       ; issue_type } =
-    get_info object_origin nullsafe_mode bad_nullability
+    get_info object_origin nullsafe_mode untrusted_kind
   in
   let description =
     F.asprintf

infer/src/nullsafe/ErrorRenderingUtils.mli

@@ -9,22 +9,44 @@
 
 open! IStd
 
+(** "Effectively nullable values" from the user perspective. Depending on context, convention, and
+    mode, Nullsafe treats such and such things as nullable or non-null. At some point this needs to
+    be explain to the user. *)
+module UserFriendlyNullable : sig
+  type t =
+    | ExplainablyNullable of explainably_nullable_kind
+        (** Value that is nullable according to nullsafe semantics and conventions. It can be
+            nullable because of an explicit annotation, models, default nullability conventions,
+            etc. *)
+    | UntrustedNonnull of untrusted_kind
+        (** Value is not nullable per se, but we still can not treat it as non-null in current mode.
+            From the user perspective, it is a very different case: violations of this type need to
+            be explained in a way so that it is clear why exactly can not nullsafe trust it in this
+            context. *)
+
+  and explainably_nullable_kind = Nullable | Null
+
+  and untrusted_kind = ThirdPartyNonnull | UncheckedNonnull | LocallyCheckedNonnull
+
+  val from_nullability : Nullability.t -> t option
+end
+
 val is_object_nullability_self_explanatory : object_expression:string -> TypeOrigin.t -> bool
 (** In order to understand why such and such object is nullable (or not nullable), we render its
     origin. In some cases this is redundant and adds extra noise for the user. *)
 
-val mk_special_nullsafe_issue :
+val mk_nullsafe_issue_for_untrusted_values :
      nullsafe_mode:NullsafeMode.t
-  -> bad_nullability:Nullability.t
+  -> untrusted_kind:UserFriendlyNullable.untrusted_kind
   -> bad_usage_location:Location.t
   -> TypeOrigin.t
-  -> (string * IssueType.t * Location.t) option
+  -> string * IssueType.t * Location.t
 (** Situation when we tried to use nonnull values in a nullsafe mode that does not trust them to be
-    non-nullable. From the user perspective, this case is different from normal nullable assignment
+    non-nullable: [untrusted_kind]. From the user perspective, this case is different from normal
-    or dereference violation: what needs to be described is why does not this mode trust this value
+    nullable assignment or dereference violation: what needs to be described is why does not this
-    (and what are possible actions). Returns a tuple (error message, issue type, error location).
+    mode trust this value (and what are possible actions). Returns a tuple (error message, issue
-    NOTE: Location of the error will be NOT in the place when the value is used (that is
+    type, error location). NOTE: Location of the error will be NOT in the place when the value is
-    [bad_usage_location]), but where the value is first obtained from. *)
+    used (that is [bad_usage_location]), but where the value is first obtained from. *)
 
 val find_alternative_nonnull_method_description : TypeOrigin.t -> string option
 (** If type origin is the result of a nullable method call that have a known nonnullable alternative