[nullsafe] Make trust list operate over sets instead of lists

Summary:
1. Most of trust list operations are abstract anyway, we don't actually
rely on the fact that this is list
2. Inside NullsafeMode.ml, we effectively need set operations, which is both more
idiomatic to express and Ocaml and faster
3. This will simplify implementation of the next diff which introduces
mode intersect operation

Reviewed By: artempyanykh

Differential Revision: D21207207

fbshipit-source-id: 0c1fc4426
master
Mitya Lyubarskiy 5 years ago committed by Facebook GitHub Bot
parent 247ecb813d
commit d3ad7880b2

@ -18,6 +18,12 @@ module Map = Caml.Map.Make (struct
let compare = compare let compare = compare
end) end)
module Set = Caml.Set.Make (struct
type nonrec t = t
let compare = compare
end)
let make ~package ~classname = let make ~package ~classname =
match package with Some "" -> {package= None; classname} | _ -> {package; classname} match package with Some "" -> {package= None; classname} | _ -> {package; classname}

@ -11,6 +11,8 @@ type t [@@deriving compare, equal]
module Map : Caml.Map.S with type key = t module Map : Caml.Map.S with type key = t
module Set : Caml.Set.S with type elt = t
val make : package:string option -> classname:string -> t val make : package:string option -> classname:string -> t
val from_string : string -> t val from_string : string -> t

@ -48,7 +48,7 @@ let mode_to_json = function
`Default `Default
| NullsafeMode.Local NullsafeMode.Trust.All -> | NullsafeMode.Local NullsafeMode.Trust.All ->
`LocalTrustAll `LocalTrustAll
| NullsafeMode.Local (NullsafeMode.Trust.Only []) -> | NullsafeMode.Local (NullsafeMode.Trust.Only names) when JavaClassName.Set.is_empty names ->
`LocalTrustNone `LocalTrustNone
| NullsafeMode.Local (NullsafeMode.Trust.Only _) -> | NullsafeMode.Local (NullsafeMode.Trust.Only _) ->
`LocalTrustSome `LocalTrustSome
@ -96,16 +96,22 @@ let make_meta_issue all_issues current_mode class_name =
match mode_to_promote_to with match mode_to_promote_to with
| Some mode_to_promote_to -> | Some mode_to_promote_to ->
(* This class is not @Nullsafe yet, but can become such! *) (* This class is not @Nullsafe yet, but can become such! *)
let trust_none_mode =
"`@Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({}))`"
in
let trust_all_mode = "`@Nullsafe(Nullsafe.Mode.Local)`" in
let promo_recommendation = let promo_recommendation =
match mode_to_promote_to with match mode_to_promote_to with
| NullsafeMode.Local NullsafeMode.Trust.All -> | NullsafeMode.Local NullsafeMode.Trust.All ->
"`@Nullsafe(Nullsafe.Mode.Local)`" trust_all_mode
| NullsafeMode.Local (NullsafeMode.Trust.Only []) | NullsafeMode.Local (NullsafeMode.Trust.Only names)
when JavaClassName.Set.is_empty names ->
trust_none_mode
| NullsafeMode.Strict | NullsafeMode.Strict
(* We don't recommend "strict" for now as it is harder to keep a class in strict mode than it "trust none" mode. (* We don't recommend "strict" for now as it is harder to keep a class in strict mode than it "trust none" mode.
Trust none is almost as safe alternative, but adding a dependency will require just updating trust list, Trust none is almost as safe alternative, but adding a dependency will require just updating trust list,
without need to strictify it first. *) -> without need to strictify it first. *) ->
"`@Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({}))`" trust_none_mode
| NullsafeMode.Default | NullsafeMode.Local (NullsafeMode.Trust.Only _) -> | NullsafeMode.Default | NullsafeMode.Local (NullsafeMode.Trust.Only _) ->
Logging.die InternalError "Unexpected promotion mode" Logging.die InternalError "Unexpected promotion mode"
in in

@ -49,7 +49,8 @@ let is_considered_nonnull ~nullsafe_mode nullability =
match nullsafe_mode with match nullsafe_mode with
| NullsafeMode.Strict -> | NullsafeMode.Strict ->
StrictNonnull StrictNonnull
| NullsafeMode.Local (NullsafeMode.Trust.Only []) -> | NullsafeMode.Local (NullsafeMode.Trust.Only classes) when JavaClassName.Set.is_empty classes
->
(* Though "trust none" is technically a subcase of trust some, (* Though "trust none" is technically a subcase of trust some,
we need this pattern to be different from the one below so we can detect possible we need this pattern to be different from the one below so we can detect possible
promotions from "trust some" to "trust none" *) promotions from "trust some" to "trust none" *)

@ -9,20 +9,23 @@ open! IStd
module F = Format module F = Format
module Trust = struct module Trust = struct
type t = All | Only of JavaClassName.t list [@@deriving compare, equal] type t = All | Only of JavaClassName.Set.t [@@deriving compare, equal]
let none = Only [] let none = Only JavaClassName.Set.empty
let extract_trust_list = function let extract_trust_list = function
| Annot.Array class_values -> | Annot.Array class_values ->
(* The only elements of this array can be class names; therefore short-circuit and return None if it's not the case. *) (* The only elements of this array can be class names; therefore short-circuit and return None if it's not the case. *)
IList.traverse_opt class_values ~f:(fun el -> let trust_list =
match el with IList.traverse_opt class_values ~f:(fun el ->
| Annot.Class class_typ -> match el with
Typ.name class_typ | Annot.Class class_typ ->
|> Option.map ~f:(fun name -> Typ.Name.Java.get_java_class_name_exn name) Typ.name class_typ
| _ -> |> Option.map ~f:(fun name -> Typ.Name.Java.get_java_class_name_exn name)
None ) | _ ->
None )
in
Option.map trust_list ~f:JavaClassName.Set.of_list
| _ -> | _ ->
None None
@ -47,15 +50,14 @@ module Trust = struct
(* We are interested only in explicit lists *) (* We are interested only in explicit lists *)
false false
| Only classes -> | Only classes ->
List.exists classes ~f:(JavaClassName.equal name) JavaClassName.Set.exists (JavaClassName.equal name) classes
let is_stricter ~stricter ~weaker = let is_stricter ~stricter ~weaker =
let is_stricter_trust_list stricter_list weaker_list = let is_stricter_trust_list stricter_set weaker_set =
(* stricter trust list should be a strict subset of the weaker one *) (* stricter trust list should be a strict subset of the weaker one *)
List.length stricter_list < List.length weaker_list JavaClassName.Set.cardinal stricter_set < JavaClassName.Set.cardinal weaker_set
&& List.for_all stricter_list ~f:(fun strict_name -> && JavaClassName.Set.subset stricter_set weaker_set
List.exists weaker_list ~f:(fun name -> JavaClassName.equal name strict_name) )
in in
match (stricter, weaker) with match (stricter, weaker) with
| All, All | All, Only _ -> | All, All | All, Only _ ->
@ -70,7 +72,7 @@ module Trust = struct
match t with match t with
| All -> | All ->
F.fprintf fmt "all" F.fprintf fmt "all"
| Only [] -> | Only names when JavaClassName.Set.is_empty names ->
F.fprintf fmt "none" F.fprintf fmt "none"
| Only _names -> | Only _names ->
F.fprintf fmt "selected" F.fprintf fmt "selected"

@ -12,7 +12,7 @@ open! IStd
module Trust : sig module Trust : sig
[@@@warning "-32"] [@@@warning "-32"]
type t = All | Only of JavaClassName.t list [@@deriving compare, equal] type t = All | Only of JavaClassName.Set.t [@@deriving compare, equal]
val none : t val none : t

Loading…
Cancel
Save