From a2bc97312561a74a4906efc53b4ef253642793ed Mon Sep 17 00:00:00 2001 From: Artem Pianykh Date: Tue, 13 Apr 2021 03:31:37 -0700 Subject: [PATCH] [nullsafe] Fix a bug in handling of modelled nullable fields Summary: The problem is that `Models.is_field_nonnullable` didn't differentiate between - having a nullable model (in which case we want the field to be NULLABLE), - not having a model at all (in which case we want the field to be THIRDPARTY_NONNULL). The problem was noticed only now because previously we didn't have any NULLABLE field models. Reviewed By: ngorogiannis Differential Revision: D27709508 fbshipit-source-id: b98c8f86f --- infer/src/nullsafe/AnnotatedField.ml | 11 ++++++--- infer/src/nullsafe/modelTables.ml | 1 + infer/src/nullsafe/models.ml | 11 +++++++-- infer/src/nullsafe/models.mli | 4 ++-- infer/src/nullsafe/typeCheck.ml | 34 ++++++++++++++++------------ 5 files changed, 40 insertions(+), 21 deletions(-) diff --git a/infer/src/nullsafe/AnnotatedField.ml b/infer/src/nullsafe/AnnotatedField.ml index 96e0cbae9..f29f70e51 100644 --- a/infer/src/nullsafe/AnnotatedField.ml +++ b/infer/src/nullsafe/AnnotatedField.ml @@ -89,9 +89,14 @@ let get tenv field_name ~class_typ ~class_under_analysis = (* This field is artifact of codegen and is not visible to the user. Surfacing it as non-strict is non-actionable for the user *) AnnotatedNullability.StrictNonnull SyntheticField - else if Models.is_field_nonnullable field_name then - AnnotatedNullability.StrictNonnull ModelledNonnull - else nullability + else + match Models.is_field_nonnullable field_name with + | Some true -> + AnnotatedNullability.StrictNonnull ModelledNonnull + | Some false -> + AnnotatedNullability.Nullable ModelledNullable + | _ -> + nullability else nullability in let field_class = diff --git a/infer/src/nullsafe/modelTables.ml b/infer/src/nullsafe/modelTables.ml index 0c0028e7f..d649d550d 100644 --- a/infer/src/nullsafe/modelTables.ml +++ b/infer/src/nullsafe/modelTables.ml @@ -747,6 +747,7 @@ let field_nullability = ; ( "android.content.pm.ResolveInfo.providerInfo" , n (* Exactly one of activityInfo, serviceInfo, or providerInfo will be non-null. *) ) ; ("android.content.res.Configuration.locale", o) + ; ("android.graphics.BitmapFactory$Options.inBitmap", n) ; ("android.graphics.Paint.Align.CENTER", o) ; ("android.graphics.Paint.Align.LEFT", o) ; ("android.graphics.Paint.Align.RIGHT", o) diff --git a/infer/src/nullsafe/models.ml b/infer/src/nullsafe/models.ml index aa227cf35..b1cca9ced 100644 --- a/infer/src/nullsafe/models.ml +++ b/infer/src/nullsafe/models.ml @@ -134,5 +134,12 @@ let find_nonnullable_alternative proc_name = let is_field_nonnullable field_name = - Hashtbl.find_opt field_nullability_table (Fieldname.to_full_string field_name) - |> Option.value_map ~f:(fun is_nullable -> not is_nullable) ~default:false + Logging.d_with_indent ~name:"is_field_nonnullable" (fun () -> + let full_name = Fieldname.to_full_string field_name in + let from_model = + Hashtbl.find_opt field_nullability_table full_name + (* Models return `is_nullable`, we need `is_notnull` *) + |> Option.map ~f:not + in + Logging.d_printfln "Model for %s: %a" full_name (Pp.option Format.pp_print_bool) from_model ; + from_model ) diff --git a/infer/src/nullsafe/models.mli b/infer/src/nullsafe/models.mli index 24ec88414..818aa1927 100644 --- a/infer/src/nullsafe/models.mli +++ b/infer/src/nullsafe/models.mli @@ -48,5 +48,5 @@ val find_nonnullable_alternative : Procname.Java.t -> ModelTables.nonnull_altern (** Check if a (nullable) method has a non-nullable alternative: A method that does the same as [proc_name] but asserts the result is not null before returning to the caller. *) -val is_field_nonnullable : Fieldname.t -> bool -(** Check if a given field is known to be a non-nullable *) +val is_field_nonnullable : Fieldname.t -> bool option +(** Check if a given field has known nullability: true = nonnull, false = null *) diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index bb5f4ebf5..690b2c421 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/infer/src/nullsafe/typeCheck.ml @@ -260,20 +260,26 @@ let funcall_exp_to_original_pvar_exp tenv curr_pname typestate exp ~is_assignmen let add_field_to_typestate_if_absent tenv access_loc typestate pvar object_origin field_name ~field_class_typ ~class_under_analysis = - match TypeState.lookup_pvar pvar typestate with - | Some _ -> - typestate - | None -> ( - match AnnotatedField.get tenv field_name ~class_typ:field_class_typ ~class_under_analysis with - | Some AnnotatedField.{annotated_type= field_type} -> - let range = - ( field_type.typ - , InferredNullability.create - (TypeOrigin.Field {object_origin; field_name; field_type; access_loc}) ) - in - TypeState.add ~descr:"add_field_to_typestate_if_absent" pvar range typestate - | None -> - typestate ) + L.d_with_indent ~name:"add_field_to_typestate_if_absent" (fun () -> + match TypeState.lookup_pvar pvar typestate with + | Some _ -> + typestate + | None -> ( + match + AnnotatedField.get tenv field_name ~class_typ:field_class_typ ~class_under_analysis + with + | Some AnnotatedField.{annotated_type= field_type} -> + let range = + ( field_type.typ + , InferredNullability.create + (TypeOrigin.Field {object_origin; field_name; field_type; access_loc}) ) + in + let _, inferred_nullability = range in + L.d_printfln "Fieldname %a: typ=%a, inferred_nullability=%a" Fieldname.pp field_name + AnnotatedType.pp field_type InferredNullability.pp inferred_nullability ; + TypeState.add ~descr:"add_field_to_typestate_if_absent" pvar range typestate + | None -> + typestate ) ) (* This does two things: