[nullsafe][refactor][messaging] Refactor code + more consistent messaging

Summary:
In the previos diff we restructured error rendering utils for
TypeOrigin.MethodCall.

In this diff we do the same with TypeOrigin field: lets make the code
consistent.

We also clearly distinct third party from all other possible cases in
this branch.

This changes messaging and reported errors for strict modes (see test cases), and I believe this is a net improvement.

Reviewed By: artempyanykh

Differential Revision: D20139741

fbshipit-source-id: 84f502553

infer/src/nullsafe/ErrorRenderingUtils.ml

@@ -84,19 +84,17 @@ let get_field_class_name field_name =
   |> Option.value_map ~f:(fun (classname, _) -> classname) ~default:"the field class"
 
 
-let mk_coming_from nullsafe_mode nullability =
+let mk_coming_from_unchecked_or_locally_checked_case_only nullsafe_mode nullability =
   match (nullsafe_mode, nullability) with
-  | NullsafeMode.Strict, Nullability.ThirdPartyNonnull
   | NullsafeMode.Strict, Nullability.UncheckedNonnull ->
-      Some "non-strict classes"
+      "non-strict classes"
   | NullsafeMode.Strict, Nullability.LocallyCheckedNonnull ->
-      Some "nullsafe-local classes"
-  | NullsafeMode.Local _, Nullability.ThirdPartyNonnull ->
-      Some "third-party classes"
+      "nullsafe-local classes"
   | NullsafeMode.Local _, Nullability.UncheckedNonnull ->
-      Some "non-nullsafe classes"
+      "non-nullsafe classes"
   | _ ->
-      None
+      Logging.die Logging.InternalError
+        "Should be called only for locally checked or unchecked nonnull cases"
 
 
 let mk_recommendation nullsafe_mode nullability what =
@@ -110,14 +108,15 @@ let mk_recommendation nullsafe_mode nullability what =
       None
 
 
-let mk_recommendation_for_third_party_field nullsafe_mode nullability field =
-  match (nullsafe_mode, nullability) with
-  | NullsafeMode.Strict, Nullability.ThirdPartyNonnull ->
-      Some (F.sprintf "access %s via a nullsafe strict getter" field)
-  | NullsafeMode.Local _, Nullability.ThirdPartyNonnull ->
-      Some (F.sprintf "access %s via a nullsafe getter" field)
-  | _ ->
-      None
+let mk_recommendation_for_third_party_field nullsafe_mode field =
+  match nullsafe_mode with
+  | NullsafeMode.Strict ->
+      F.sprintf "access %s via a nullsafe strict getter" field
+  | NullsafeMode.Local _ ->
+      F.sprintf "access %s via a nullsafe getter" field
+  | NullsafeMode.Default ->
+      Logging.die Logging.InternalError
+        "Should not happen: we should tolerate third party in default mode"
 
 
 let get_info object_origin nullsafe_mode bad_nullability =
@@ -136,7 +135,6 @@ let get_info object_origin nullsafe_mode bad_nullability =
             (* This method makes sense only for non-nullable violations *)
             None
         | Nullability.StrictNonnull ->
-            (* This method makes sense only for non-nullable violations *)
             Logging.die InternalError "There should not be type violations involving StrictNonnull"
         | Nullability.ThirdPartyNonnull ->
             let suggested_third_party_sig_file =
@@ -157,7 +155,9 @@ let get_info object_origin nullsafe_mode bad_nullability =
               , F.sprintf "add the correct signature to %s" where_to_add_signature
               , IssueType.eradicate_unvetted_third_party_in_nullsafe )
         | Nullability.UncheckedNonnull | Nullability.LocallyCheckedNonnull ->
-            let* from = mk_coming_from nullsafe_mode bad_nullability in
+            let from =
+              mk_coming_from_unchecked_or_locally_checked_case_only nullsafe_mode bad_nullability
+            in
             let+ recommendation =
               let what_to_strictify =
                 Option.value (get_method_class_name pname) ~default:offending_object
@@ -184,13 +184,27 @@ 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 = mk_coming_from nullsafe_mode bad_nullability in
-      let+ recommendation =
-        Option.first_some
-          (mk_recommendation_for_third_party_field nullsafe_mode bad_nullability unqualified_name)
-          (mk_recommendation nullsafe_mode bad_nullability (get_field_class_name field_name))
+      let+ coming_from_explanation, recommendation, issue_type =
+        match bad_nullability with
+        | Nullability.Null | Nullability.Nullable ->
+            (* 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 ->
+            let from =
+              mk_coming_from_unchecked_or_locally_checked_case_only nullsafe_mode bad_nullability
+            in
+            let+ recommendation =
+              mk_recommendation nullsafe_mode bad_nullability (get_field_class_name field_name)
+            in
+            (from, recommendation, IssueType.eradicate_unchecked_usage_in_nullsafe)
       in
-      let issue_type = IssueType.eradicate_unchecked_usage_in_nullsafe in
       { offending_object= qualified_name
       ; object_loc
       ; coming_from_explanation

infer/tests/codetoanalyze/java/nullsafe-default/issues.exp

@@ -156,7 +156,7 @@ codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsa
 codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$StrictNullsafe.BAD_returnFromNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$NonNullsafe.returnVal()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 198. Either add a local check for null or assertion, or make NullsafeMode$NonNullsafe nullsafe strict.]
 codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_passThirdPartyToUnchecked():codetoanalyze.java.nullsafe_default.NullsafeMode$UncheckedParams, 0, ERADICATE_UNVETTED_THIRD_PARTY_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.getUncheckedLong(...)`: `@NullsafeLocal(trust=all)` mode prohibits using values coming from not vetted third party methods without a check. Result of this call is used at line 113. Either add a local check for null or assertion, or add the correct signature to nullsafe-default/third-party-signatures/some.test.pckg.sig.]
 codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnFromUnvettedThirdParty():java.lang.String, 0, ERADICATE_UNVETTED_THIRD_PARTY_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.returnUnspecified()`: `@NullsafeLocal(trust=all)` mode prohibits using values coming from not vetted third party methods without a check. Result of this call is used at line 93. Either add a local check for null or assertion, or add the correct signature to nullsafe-default/third-party-signatures/some.test.pckg.sig.]
-codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnNonNullableFieldFromThirdParty():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.nonNullableField`: `@NullsafeLocal(trust=all)` mode prohibits using values coming from third-party classes without a check. This field is used at line 101. Either add a local check for null or assertion, or access `nonNullableField` via a nullsafe getter.]
+codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnNonNullableFieldFromThirdParty():java.lang.String, 0, ERADICATE_UNVETTED_THIRD_PARTY_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.nonNullableField`: `@NullsafeLocal(trust=all)` mode prohibits using values coming from third-party classes without a check. This field is used at line 101. Either add a local check for null or assertion, or access `nonNullableField` via a nullsafe getter.]
 codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnNullFromNonNulsafe():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`BAD_returnNullFromNonNulsafe()`: return type is declared non-nullable but the method returns a nullable value: call to returnNull() at line 89.]
 codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnNullableFieldFromThirdParty():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`BAD_returnNullableFieldFromThirdParty()`: return type is declared non-nullable but the method returns a nullable value: field nullableField at line 97.]
 codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustNoneNullsafe.BAD_returnFromNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$NonNullsafe.returnVal()`: `@NullsafeLocal(trust=none)` mode prohibits using values coming from non-nullsafe classes without a check. Result of this call is used at line 155. Either add a local check for null or assertion, or make NullsafeMode$NonNullsafe nullsafe.]
@@ -253,7 +253,7 @@ codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe
 codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_dereferenceNullableFieldIsBad():void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, ERROR, [`__new(...).nullable` is nullable and is not locally checked for null when calling `toString()`.]
 codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_dereferenceNullableMethodIsBad():void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, ERROR, [`__new(...).getNullable()` is nullable and is not locally checked for null when calling `toString()`.]
 codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_dereferenceNullableStaticMethodIsBad():void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, ERROR, [`staticNullable()` is nullable and is not locally checked for null when calling `toString()`.]
-codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.dereferenceFieldIsBAD():void, 0, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.nonNullableField`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. This field is used at line 49. Either add a local check for null or assertion, or access `nonNullableField` via a nullsafe strict getter.]
+codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.dereferenceFieldIsBAD():void, 0, ERADICATE_UNVETTED_THIRD_PARTY_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.nonNullableField`: `@NullsafeStrict` mode prohibits using values coming from third-party classes without a check. This field is used at line 49. Either add a local check for null or assertion, or access `nonNullableField` via a nullsafe strict getter.]
 codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.dereferenceSpecifiedAsNullableIsBAD():void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, ERROR, [`StrictModeForThirdParty.obj.returnSpecifiedAsNullable()` is nullable and is not locally checked for null when calling `toString()`: call to ThirdPartyTestClass.returnSpecifiedAsNullable() at line 45 (declared nullable in nullsafe-default/third-party-signatures/some.test.pckg.sig at line 2).]
 codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.dereferenceUnspecifiedIsBAD():void, 0, ERADICATE_UNVETTED_THIRD_PARTY_IN_NULLSAFE, no_bucket, ERROR, [`ThirdPartyTestClass.returnUnspecified()`: `@NullsafeStrict` mode prohibits using values coming from not vetted third party methods without a check. Result of this call is used at line 41. Either add a local check for null or assertion, or add the correct signature to nullsafe-default/third-party-signatures/some.test.pckg.sig.]
 codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.passingNullableParamToUnspecifiedIsBAD():void, 0, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, ERROR, [Third-party `ThirdPartyTestClass.paramUnspecified(...)` is missing a signature that would allow passing a nullable to param #1(`param`). Actual argument `getNullable()` is nullable. Consider adding the correct signature of `ThirdPartyTestClass.paramUnspecified(...)` to nullsafe-default/third-party-signatures/some.test.pckg.sig.]