From d3ad7880b211d404b2d713f22d2ead03c9e01fc0 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Fri, 24 Apr 2020 09:36:51 -0700 Subject: [PATCH] [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 --- infer/src/IR/JavaClassName.ml | 6 +++++ infer/src/IR/JavaClassName.mli | 2 ++ infer/src/nullsafe/ClassLevelAnalysis.ml | 14 ++++++++--- infer/src/nullsafe/Nullability.ml | 3 ++- infer/src/nullsafe/NullsafeMode.ml | 32 +++++++++++++----------- infer/src/nullsafe/NullsafeMode.mli | 2 +- 6 files changed, 38 insertions(+), 21 deletions(-) diff --git a/infer/src/IR/JavaClassName.ml b/infer/src/IR/JavaClassName.ml index 44a4cd9cd..240dd4903 100644 --- a/infer/src/IR/JavaClassName.ml +++ b/infer/src/IR/JavaClassName.ml @@ -18,6 +18,12 @@ module Map = Caml.Map.Make (struct let compare = compare end) +module Set = Caml.Set.Make (struct + type nonrec t = t + + let compare = compare +end) + let make ~package ~classname = match package with Some "" -> {package= None; classname} | _ -> {package; classname} diff --git a/infer/src/IR/JavaClassName.mli b/infer/src/IR/JavaClassName.mli index de3147946..2edba0f7f 100644 --- a/infer/src/IR/JavaClassName.mli +++ b/infer/src/IR/JavaClassName.mli @@ -11,6 +11,8 @@ type t [@@deriving compare, equal] 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 from_string : string -> t diff --git a/infer/src/nullsafe/ClassLevelAnalysis.ml b/infer/src/nullsafe/ClassLevelAnalysis.ml index 6294b2faa..6681de528 100644 --- a/infer/src/nullsafe/ClassLevelAnalysis.ml +++ b/infer/src/nullsafe/ClassLevelAnalysis.ml @@ -48,7 +48,7 @@ let mode_to_json = function `Default | NullsafeMode.Local NullsafeMode.Trust.All -> `LocalTrustAll - | NullsafeMode.Local (NullsafeMode.Trust.Only []) -> + | NullsafeMode.Local (NullsafeMode.Trust.Only names) when JavaClassName.Set.is_empty names -> `LocalTrustNone | NullsafeMode.Local (NullsafeMode.Trust.Only _) -> `LocalTrustSome @@ -96,16 +96,22 @@ let make_meta_issue all_issues current_mode class_name = match mode_to_promote_to with | Some mode_to_promote_to -> (* 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 = match mode_to_promote_to with | NullsafeMode.Local NullsafeMode.Trust.All -> - "`@Nullsafe(Nullsafe.Mode.Local)`" - | NullsafeMode.Local (NullsafeMode.Trust.Only []) + trust_all_mode + | NullsafeMode.Local (NullsafeMode.Trust.Only names) + when JavaClassName.Set.is_empty names -> + trust_none_mode | 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. Trust none is almost as safe alternative, but adding a dependency will require just updating trust list, without need to strictify it first. *) -> - "`@Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({}))`" + trust_none_mode | NullsafeMode.Default | NullsafeMode.Local (NullsafeMode.Trust.Only _) -> Logging.die InternalError "Unexpected promotion mode" in diff --git a/infer/src/nullsafe/Nullability.ml b/infer/src/nullsafe/Nullability.ml index 1e3df12b1..b7eb6a808 100644 --- a/infer/src/nullsafe/Nullability.ml +++ b/infer/src/nullsafe/Nullability.ml @@ -49,7 +49,8 @@ let is_considered_nonnull ~nullsafe_mode nullability = match nullsafe_mode with | NullsafeMode.Strict -> 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, we need this pattern to be different from the one below so we can detect possible promotions from "trust some" to "trust none" *) diff --git a/infer/src/nullsafe/NullsafeMode.ml b/infer/src/nullsafe/NullsafeMode.ml index 56ab7d4e3..bbe946028 100644 --- a/infer/src/nullsafe/NullsafeMode.ml +++ b/infer/src/nullsafe/NullsafeMode.ml @@ -9,20 +9,23 @@ open! IStd module F = Format 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 | 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. *) - IList.traverse_opt class_values ~f:(fun el -> - match el with - | Annot.Class class_typ -> - Typ.name class_typ - |> Option.map ~f:(fun name -> Typ.Name.Java.get_java_class_name_exn name) - | _ -> - None ) + let trust_list = + IList.traverse_opt class_values ~f:(fun el -> + match el with + | Annot.Class class_typ -> + Typ.name class_typ + |> Option.map ~f:(fun name -> Typ.Name.Java.get_java_class_name_exn name) + | _ -> + None ) + in + Option.map trust_list ~f:JavaClassName.Set.of_list | _ -> None @@ -47,15 +50,14 @@ module Trust = struct (* We are interested only in explicit lists *) false | Only classes -> - List.exists classes ~f:(JavaClassName.equal name) + JavaClassName.Set.exists (JavaClassName.equal name) classes 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 *) - List.length stricter_list < List.length weaker_list - && List.for_all stricter_list ~f:(fun strict_name -> - List.exists weaker_list ~f:(fun name -> JavaClassName.equal name strict_name) ) + JavaClassName.Set.cardinal stricter_set < JavaClassName.Set.cardinal weaker_set + && JavaClassName.Set.subset stricter_set weaker_set in match (stricter, weaker) with | All, All | All, Only _ -> @@ -70,7 +72,7 @@ module Trust = struct match t with | All -> F.fprintf fmt "all" - | Only [] -> + | Only names when JavaClassName.Set.is_empty names -> F.fprintf fmt "none" | Only _names -> F.fprintf fmt "selected" diff --git a/infer/src/nullsafe/NullsafeMode.mli b/infer/src/nullsafe/NullsafeMode.mli index 9421226be..d57400d9d 100644 --- a/infer/src/nullsafe/NullsafeMode.mli +++ b/infer/src/nullsafe/NullsafeMode.mli @@ -12,7 +12,7 @@ open! IStd module Trust : sig [@@@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