Skip to content

Commit a2bc973

Browse files
artempyanykhfacebook-github-bot
authored andcommitted
[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
1 parent 341169f commit a2bc973

File tree

5 files changed

+40
-21
lines changed

5 files changed

+40
-21
lines changed

infer/src/nullsafe/AnnotatedField.ml

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,14 @@ let get tenv field_name ~class_typ ~class_under_analysis =
8989
(* This field is artifact of codegen and is not visible to the user.
9090
Surfacing it as non-strict is non-actionable for the user *)
9191
AnnotatedNullability.StrictNonnull SyntheticField
92-
else if Models.is_field_nonnullable field_name then
93-
AnnotatedNullability.StrictNonnull ModelledNonnull
94-
else nullability
92+
else
93+
match Models.is_field_nonnullable field_name with
94+
| Some true ->
95+
AnnotatedNullability.StrictNonnull ModelledNonnull
96+
| Some false ->
97+
AnnotatedNullability.Nullable ModelledNullable
98+
| _ ->
99+
nullability
95100
else nullability
96101
in
97102
let field_class =

infer/src/nullsafe/modelTables.ml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -747,6 +747,7 @@ let field_nullability =
747747
; ( "android.content.pm.ResolveInfo.providerInfo"
748748
, n (* Exactly one of activityInfo, serviceInfo, or providerInfo will be non-null. *) )
749749
; ("android.content.res.Configuration.locale", o)
750+
; ("android.graphics.BitmapFactory$Options.inBitmap", n)
750751
; ("android.graphics.Paint.Align.CENTER", o)
751752
; ("android.graphics.Paint.Align.LEFT", o)
752753
; ("android.graphics.Paint.Align.RIGHT", o)

infer/src/nullsafe/models.ml

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,5 +134,12 @@ let find_nonnullable_alternative proc_name =
134134

135135

136136
let is_field_nonnullable field_name =
137-
Hashtbl.find_opt field_nullability_table (Fieldname.to_full_string field_name)
138-
|> Option.value_map ~f:(fun is_nullable -> not is_nullable) ~default:false
137+
Logging.d_with_indent ~name:"is_field_nonnullable" (fun () ->
138+
let full_name = Fieldname.to_full_string field_name in
139+
let from_model =
140+
Hashtbl.find_opt field_nullability_table full_name
141+
(* Models return `is_nullable`, we need `is_notnull` *)
142+
|> Option.map ~f:not
143+
in
144+
Logging.d_printfln "Model for %s: %a" full_name (Pp.option Format.pp_print_bool) from_model ;
145+
from_model )

infer/src/nullsafe/models.mli

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,5 +48,5 @@ val find_nonnullable_alternative : Procname.Java.t -> ModelTables.nonnull_altern
4848
(** Check if a (nullable) method has a non-nullable alternative: A method that does the same as
4949
[proc_name] but asserts the result is not null before returning to the caller. *)
5050

51-
val is_field_nonnullable : Fieldname.t -> bool
52-
(** Check if a given field is known to be a non-nullable *)
51+
val is_field_nonnullable : Fieldname.t -> bool option
52+
(** Check if a given field has known nullability: true = nonnull, false = null *)

infer/src/nullsafe/typeCheck.ml

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -260,20 +260,26 @@ let funcall_exp_to_original_pvar_exp tenv curr_pname typestate exp ~is_assignmen
260260

261261
let add_field_to_typestate_if_absent tenv access_loc typestate pvar object_origin field_name
262262
~field_class_typ ~class_under_analysis =
263-
match TypeState.lookup_pvar pvar typestate with
264-
| Some _ ->
265-
typestate
266-
| None -> (
267-
match AnnotatedField.get tenv field_name ~class_typ:field_class_typ ~class_under_analysis with
268-
| Some AnnotatedField.{annotated_type= field_type} ->
269-
let range =
270-
( field_type.typ
271-
, InferredNullability.create
272-
(TypeOrigin.Field {object_origin; field_name; field_type; access_loc}) )
273-
in
274-
TypeState.add ~descr:"add_field_to_typestate_if_absent" pvar range typestate
275-
| None ->
276-
typestate )
263+
L.d_with_indent ~name:"add_field_to_typestate_if_absent" (fun () ->
264+
match TypeState.lookup_pvar pvar typestate with
265+
| Some _ ->
266+
typestate
267+
| None -> (
268+
match
269+
AnnotatedField.get tenv field_name ~class_typ:field_class_typ ~class_under_analysis
270+
with
271+
| Some AnnotatedField.{annotated_type= field_type} ->
272+
let range =
273+
( field_type.typ
274+
, InferredNullability.create
275+
(TypeOrigin.Field {object_origin; field_name; field_type; access_loc}) )
276+
in
277+
let _, inferred_nullability = range in
278+
L.d_printfln "Fieldname %a: typ=%a, inferred_nullability=%a" Fieldname.pp field_name
279+
AnnotatedType.pp field_type InferredNullability.pp inferred_nullability ;
280+
TypeState.add ~descr:"add_field_to_typestate_if_absent" pvar range typestate
281+
| None ->
282+
typestate ) )
277283

278284

279285
(* This does two things:

0 commit comments

Comments
 (0)