From 0bf010d72a6297fc006b27ab3f9e95290760ff9d Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Wed, 4 Nov 2020 03:15:43 -0800 Subject: [PATCH] [nullsafe] Build and output the annotation graph Summary: This diff glues the previous work together. The ClassLevelAnalysis finds list of provisional violation, builds the graph based on them, and outputs this graph as a separate issue. Reviewed By: artempyanykh Differential Revision: D24682802 fbshipit-source-id: 8174da91a --- infer/man/man1/infer-full.txt | 1 + infer/man/man1/infer-report.txt | 1 + infer/man/man1/infer.txt | 1 + infer/src/absint/PatternMatch.ml | 8 + infer/src/absint/PatternMatch.mli | 2 + infer/src/atd/jsonbug.atd | 24 ++- infer/src/base/IssueType.ml | 7 + infer/src/base/IssueType.mli | 2 + infer/src/nullsafe/AnnotationGraph.ml | 210 ++++++++++++++++++++ infer/src/nullsafe/AnnotationGraph.mli | 18 ++ infer/src/nullsafe/ClassLevelAnalysis.ml | 87 ++++---- infer/src/nullsafe/NullsafeIssue.ml | 8 +- infer/src/nullsafe/ProvisionalViolation.ml | 25 +++ infer/src/nullsafe/ProvisionalViolation.mli | 17 ++ 14 files changed, 362 insertions(+), 49 deletions(-) create mode 100644 infer/src/nullsafe/AnnotationGraph.ml create mode 100644 infer/src/nullsafe/AnnotationGraph.mli create mode 100644 infer/src/nullsafe/ProvisionalViolation.ml create mode 100644 infer/src/nullsafe/ProvisionalViolation.mli diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index 5553eead1..7f1c0d0b6 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -426,6 +426,7 @@ OPTIONS DIVIDE_BY_ZERO (disabled by default), DO_NOT_REPORT (enabled by default), EMPTY_VECTOR_ACCESS (enabled by default), + ERADICATE_ANNOTATION_GRAPH (enabled by default), ERADICATE_BAD_NESTED_CLASS_ANNOTATION (enabled by default), ERADICATE_CONDITION_REDUNDANT (enabled by default), ERADICATE_FIELD_NOT_INITIALIZED (enabled by default), diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt index 867289276..41fd41dc0 100644 --- a/infer/man/man1/infer-report.txt +++ b/infer/man/man1/infer-report.txt @@ -134,6 +134,7 @@ OPTIONS DIVIDE_BY_ZERO (disabled by default), DO_NOT_REPORT (enabled by default), EMPTY_VECTOR_ACCESS (enabled by default), + ERADICATE_ANNOTATION_GRAPH (enabled by default), ERADICATE_BAD_NESTED_CLASS_ANNOTATION (enabled by default), ERADICATE_CONDITION_REDUNDANT (enabled by default), ERADICATE_FIELD_NOT_INITIALIZED (enabled by default), diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index d18358298..f31576bb5 100644 --- a/infer/man/man1/infer.txt +++ b/infer/man/man1/infer.txt @@ -426,6 +426,7 @@ OPTIONS DIVIDE_BY_ZERO (disabled by default), DO_NOT_REPORT (enabled by default), EMPTY_VECTOR_ACCESS (enabled by default), + ERADICATE_ANNOTATION_GRAPH (enabled by default), ERADICATE_BAD_NESTED_CLASS_ANNOTATION (enabled by default), ERADICATE_CONDITION_REDUNDANT (enabled by default), ERADICATE_FIELD_NOT_INITIALIZED (enabled by default), diff --git a/infer/src/absint/PatternMatch.ml b/infer/src/absint/PatternMatch.ml index d4727be7c..411dc6ec9 100644 --- a/infer/src/absint/PatternMatch.ml +++ b/infer/src/absint/PatternMatch.ml @@ -521,6 +521,14 @@ let lookup_attributes tenv proc_name = !found_attributes +let lookup_attributes_exn tenv proc_name = + match lookup_attributes tenv proc_name with + | Some result -> + result + | None -> + Logging.die InternalError "Did not find attributes for %a" Procname.pp proc_name + + (** return the set of instance fields that are assigned to a null literal in [procdesc] *) let get_fields_nullified procdesc = (* walk through the instructions and look for instance fields that are assigned to null *) diff --git a/infer/src/absint/PatternMatch.mli b/infer/src/absint/PatternMatch.mli index 34b0a7995..e715fcae0 100644 --- a/infer/src/absint/PatternMatch.mli +++ b/infer/src/absint/PatternMatch.mli @@ -170,6 +170,8 @@ val override_iter : (Procname.t -> unit) -> Tenv.t -> Procname.t -> unit val lookup_attributes : Tenv.t -> Procname.t -> ProcAttributes.t option +val lookup_attributes_exn : Tenv.t -> Procname.t -> ProcAttributes.t + val type_name_get_annotation : Tenv.t -> Typ.name -> Annot.Item.t option val type_get_annotation : Tenv.t -> Typ.t -> Annot.Item.t option diff --git a/infer/src/atd/jsonbug.atd b/infer/src/atd/jsonbug.atd index 74d5fa2a7..77a2053f4 100644 --- a/infer/src/atd/jsonbug.atd +++ b/infer/src/atd/jsonbug.atd @@ -35,12 +35,34 @@ type method_info = { call_line: int; } +type annotation_point_kind = [Method | Field | Param] + +type access_level = [Private | Protected | Public | Default] + +type annotation_point_method = { + method_name: string; + params: string list; + access_level: access_level; +} + +(* A potential place to annotate as [@Nullable] *) +type annotation_point = { + id: string; (* A symbolic id of an annotation point *) + kind: annotation_point_kind; + ?method_info: annotation_point_method option; (* Present iff this is a Method or Param annotation point *) + ?field_name: string option; (* Present iff this is a Field annotation point *) + ?param_num: int option; (* Present iff this is a Param annotation point *) + num_violations: int; (* How many violations (NOT counting violations happening in other annotation points) would be introduced if this point was annotated *) + dependent_point_ids: string list; (* List of other points that need to be annotated when this one is annotated *) +} + type nullsafe_extra = { class_name: string; package: string nullable; ?nullable_methods: method_info list option; (* If the issue is related to unsafe use of methods being nullable, here's the list of them *) ?unvetted_3rd_party: string list option; (* If the issue is related to the use of a not yet registered third-party methods, here's the list of their signatures *) - ?meta_issue_info: nullsafe_meta_issue_info option (* Should be present if the issue is "nullsafe meta issue" *) + ?meta_issue_info: nullsafe_meta_issue_info option; (* Should be present if the issue is "nullsafe meta issue" *) + ?annotation_graph: annotation_point list option; (* Annotation graph (only for annotation mode) *) } type extra = { diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index b18ac0594..b36636dd9 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -468,6 +468,13 @@ let empty_vector_access = ~user_documentation:[%blob "../../documentation/issues/EMPTY_VECTOR_ACCESS.md"] +(* A technical issue needed to output the annotation graph for the class - not intended to be surfaces to the end user *) +let eradicate_annotation_graph = + (* Enabled by default since this requires a special mode anyway *) + register ~id:"ERADICATE_ANNOTATION_GRAPH" ~hum:"Annotation Graph" Info Eradicate + ~user_documentation:"" + + (* Condition redundant is a very non-precise issue. Depending on the origin of what is compared with null, this can have a lot of reasons to be actually nullable. diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index df485b13a..0024285b5 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -154,6 +154,8 @@ val do_not_report : t val empty_vector_access : t +val eradicate_annotation_graph : t + val eradicate_condition_redundant : t val eradicate_field_not_initialized : t diff --git a/infer/src/nullsafe/AnnotationGraph.ml b/infer/src/nullsafe/AnnotationGraph.ml new file mode 100644 index 000000000..8aa8d96ec --- /dev/null +++ b/infer/src/nullsafe/AnnotationGraph.ml @@ -0,0 +1,210 @@ +(* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + *) +open! IStd + +type point_info = + { id: string + ; annotation: ProvisionalAnnotation.t + ; mutable num_violations: int + ; mutable dependent_points: ProvisionalAnnotation.t list } + +module AnnotationMap = Caml.Map.Make (struct + type t = ProvisionalAnnotation.t [@@deriving compare] +end) + +let get_provisional_annotation = function + | AnnotatedNullability.ProvisionallyNullable provisional_annotation -> + Some provisional_annotation + | _ -> + None + + +(* Corresponding provisional annotations for the return value and params, if any *) +let annotations_of_signature AnnotatedSignature.{ret; params} = + let annotated_nullability = + ret.ret_annotated_type.nullability + :: List.map params ~f:(fun AnnotatedSignature.{param_annotated_type= {nullability}} -> + nullability ) + in + List.filter_map annotated_nullability ~f:get_provisional_annotation + + +(* Given a list of provisional annotations, construct corresponding nodes for the graph *) +let graph_nodes_of_provisional_annotations annotations = + let get_id index annotation = + (* Symbolic prefix corresponding to annotation point type for convenience*) + let prefix = + match annotation with + | ProvisionalAnnotation.Field _ -> + "f" + | ProvisionalAnnotation.Method _ -> + "m" + | ProvisionalAnnotation.Param _ -> + "p" + in + Format.sprintf "%s%d" prefix index + in + let annotation_points = + List.mapi annotations ~f:(fun index annotation -> + ( annotation + , {id= get_id index annotation; annotation; num_violations= 0; dependent_points= []} ) ) + in + AnnotationMap.of_seq (Stdlib.List.to_seq annotation_points) + + +let build_graph_nodes tenv class_struct class_name = + let class_typ = Typ.mk_struct class_name in + let field_annotations = + class_struct.Struct.fields + |> List.filter_map ~f:(fun (field_name, _, _) -> + let AnnotatedField.{annotated_type= {nullability}} = + Option.value_exn + (AnnotatedField.get tenv field_name ~class_typ ~class_under_analysis:class_name) + in + get_provisional_annotation nullability ) + in + let method_signatures = + class_struct.Struct.methods + |> List.map ~f:(fun proc_name -> + let proc_attributes = Option.value_exn (PatternMatch.lookup_attributes tenv proc_name) in + AnnotatedSignature.get_for_class_under_analysis tenv proc_attributes ) + in + let method_and_param_annotations = + List.map method_signatures ~f:annotations_of_signature |> List.concat + in + graph_nodes_of_provisional_annotations (field_annotations @ method_and_param_annotations) + + +let get_offending_annotations = function + | ProvisionalViolation.Assignment violation -> + AssignmentRule.ProvisionalViolation.offending_annotations violation + | ProvisionalViolation.Dereference violation -> + DereferenceRule.ProvisionalViolation.offending_annotations violation + + +let get_fix_annotation = function + | ProvisionalViolation.Assignment violation -> + AssignmentRule.ProvisionalViolation.fix_annotation violation + | ProvisionalViolation.Dereference _ -> + None + + +let update_by_violation nodes provisional_violation = + let fix_annotation = get_fix_annotation provisional_violation in + Option.iter fix_annotation ~f:(fun annotation -> + if not (AnnotationMap.mem annotation nodes) then + Logging.die InternalError "Did not find a node for %a in the graph" ProvisionalAnnotation.pp + annotation ) ; + let offending_annotations = get_offending_annotations provisional_violation in + List.iter offending_annotations ~f:(fun annotation -> + match AnnotationMap.find_opt annotation nodes with + | Some annotation_point -> ( + match fix_annotation with + | Some fix_annotation -> + (* If that provisional annotation becomes real [@Nullable], that would raise an issue fixable by the other annotation. + Add the new edge in the graph. + *) + annotation_point.dependent_points <- + List.dedup_and_sort + (fix_annotation :: annotation_point.dependent_points) + ~compare:ProvisionalAnnotation.compare + | None -> + (* If that provisional annotation becomes real [@Nullable], that would lead to a violation + * (not related to other provisional annotations). + *) + annotation_point.num_violations <- annotation_point.num_violations + 1 ) + | None -> + Logging.die InternalError "Did not find a node for %a in the graph" + ProvisionalAnnotation.pp annotation ) + + +(* Given a list of provisional violations, connect corresponding nodes in the annotation graph *) +let update_nodes_and_set_edges nodes provisional_violations = + List.iter provisional_violations ~f:(update_by_violation nodes) + + +let get_kind_json = function + | ProvisionalAnnotation.Field _ -> + `Field + | ProvisionalAnnotation.Method _ -> + `Method + | ProvisionalAnnotation.Param _ -> + `Param + + +let get_field_name_json = function + | ProvisionalAnnotation.Field {field_name} -> + Some (Fieldname.get_field_name field_name) + | ProvisionalAnnotation.Method _ | ProvisionalAnnotation.Param _ -> + None + + +let get_param_num_json = function + | ProvisionalAnnotation.Param {num} -> + Some num + | ProvisionalAnnotation.Method _ | ProvisionalAnnotation.Field _ -> + None + + +let java_type_to_string java_type = Pp.string_of_pp (Typ.pp_java ~verbose:true) java_type + +let get_access_level tenv proc_name = + let proc_attributes = PatternMatch.lookup_attributes_exn tenv (Procname.Java proc_name) in + match ProcAttributes.get_access proc_attributes with + | Default -> + `Default + | Public -> + `Public + | Private -> + `Private + | Protected -> + `Protected + + +let get_method_info_json tenv annotation = + let open IOption.Let_syntax in + let+ proc_name = + match annotation with + | ProvisionalAnnotation.Param {method_info} -> + Some method_info + | ProvisionalAnnotation.Method proc_name -> + Some proc_name + | ProvisionalAnnotation.Field _ -> + None + in + let method_name = Procname.Java.get_method proc_name in + let params = Procname.Java.get_parameters proc_name |> List.map ~f:java_type_to_string in + let access_level = get_access_level tenv proc_name in + Jsonbug_t.{method_name; params; access_level} + + +let to_json_annotation_point tenv graph {id; annotation; num_violations; dependent_points} : + Jsonbug_t.annotation_point = + { id + ; kind= get_kind_json annotation + ; method_info= get_method_info_json tenv annotation + ; field_name= get_field_name_json annotation + ; param_num= get_param_num_json annotation + ; num_violations + ; dependent_point_ids= + List.map dependent_points ~f:(fun dependent_point -> + let node = AnnotationMap.find dependent_point graph in + node.id ) } + + +(* Convert the graph to the JSON representation *) +let to_json tenv graph = + AnnotationMap.bindings graph |> List.map ~f:snd + (* Sort by ids to get any stable order *) + |> List.sort ~compare:(fun {id= id1} {id= id2} -> String.compare id1 id2) + |> List.map ~f:(to_json_annotation_point tenv graph) + + +let build_graph tenv class_struct class_name provisional_violations = + let graph = build_graph_nodes tenv class_struct class_name in + update_nodes_and_set_edges graph provisional_violations ; + to_json tenv graph diff --git a/infer/src/nullsafe/AnnotationGraph.mli b/infer/src/nullsafe/AnnotationGraph.mli new file mode 100644 index 000000000..3bba6fe70 --- /dev/null +++ b/infer/src/nullsafe/AnnotationGraph.mli @@ -0,0 +1,18 @@ +(* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + *) +open! IStd + +(** Annotation graph for a Java class comprises of: * Nodes (annotation points): potential places in + the class declaration to be annotated as [@Nullable] * For each annotation point the graph + stores what will happen if this annotation point becomes real [@Nullable] (new violations that + will arise) * Edges: if annotating a point A will require annotating another point B as + [@Nullable], A -> B are connected in the graph. *) + +val build_graph : + Tenv.t -> Struct.t -> Typ.name -> ProvisionalViolation.t list -> Jsonbug_t.annotation_point list +(** Given a Java class and the list of all provisional violations found in that class, build the + annotation graph *) diff --git a/infer/src/nullsafe/ClassLevelAnalysis.ml b/infer/src/nullsafe/ClassLevelAnalysis.ml index 445f4eafe..f9f65e695 100644 --- a/infer/src/nullsafe/ClassLevelAnalysis.ml +++ b/infer/src/nullsafe/ClassLevelAnalysis.ml @@ -188,7 +188,8 @@ let report_meta_issue_for_top_level_class tenv source_file class_name class_stru ; package ; meta_issue_info= Some meta_issue_info ; unvetted_3rd_party= None - ; nullable_methods= None } + ; nullable_methods= None + ; annotation_graph= None } in log_issue ~issue_log ~loc:class_loc ~severity ~nullsafe_extra issue_type description @@ -214,7 +215,12 @@ let analyze_nullsafe_annotations tenv source_file class_name class_struct issue_ let package = JavaClassName.package class_name in let class_name = JavaClassName.classname class_name in Jsonbug_t. - {class_name; package; meta_issue_info= None; unvetted_3rd_party= None; nullable_methods= None} + { class_name + ; package + ; meta_issue_info= None + ; unvetted_3rd_party= None + ; nullable_methods= None + ; annotation_graph= None } in match NullsafeMode.check_problematic_class_annotation tenv class_name with | Ok () -> @@ -250,58 +256,45 @@ let analyze_nullsafe_annotations tenv source_file class_name class_struct issue_ IssueType.eradicate_bad_nested_class_annotation description -let process_issue_for_annotation_graph issue = - match issue with - | TypeErr.Condition_redundant _ - | TypeErr.Field_not_initialized _ - | TypeErr.Inconsistent_subclass _ - | TypeErr.Over_annotation _ -> - () - | TypeErr.Nullable_dereference {dereference_violation} -> - DereferenceRule.ProvisionalViolation.from dereference_violation - |> Option.iter ~f:(fun provisional_violation -> - let annotations = - DereferenceRule.ProvisionalViolation.offending_annotations provisional_violation - in - Logging.debug Analysis Medium - "Found provisional violation: dereference caused by any of %a\n" - (Pp.seq ProvisionalAnnotation.pp) annotations ) - | TypeErr.Bad_assignment {assignment_violation} -> - AssignmentRule.ProvisionalViolation.from assignment_violation - |> Option.iter ~f:(fun provisional_violation -> - let offending_annotations = - AssignmentRule.ProvisionalViolation.offending_annotations provisional_violation - in - let fix_annotation = - AssignmentRule.ProvisionalViolation.fix_annotation provisional_violation - in - let fix_annotation_descr = - Option.value_map fix_annotation - ~f:(fun annotation -> - Format.asprintf ", fixable by %a" ProvisionalAnnotation.pp annotation ) - ~default:"" - in - Logging.debug Analysis Medium - "Found provisional violation: assignment caused by any of %a%s\n" - (Pp.seq ProvisionalAnnotation.pp) offending_annotations fix_annotation_descr ) - - -let construct_annotation_graph _class_name class_info issue_log = +let report_annotation_graph source_file class_name class_struct annotation_graph issue_log = + let class_loc = get_class_loc source_file class_struct in + let package = JavaClassName.package class_name in + let class_name = JavaClassName.classname class_name in + let nullsafe_extra = + Jsonbug_t. + { class_name + ; package + ; meta_issue_info= None + ; unvetted_3rd_party= None + ; nullable_methods= None + ; annotation_graph= Some annotation_graph } + in + log_issue ~issue_log ~loc:class_loc ~severity:IssueType.Info ~nullsafe_extra + IssueType.eradicate_annotation_graph "" + + +let build_and_report_annotation_graph tenv source_file class_name class_struct class_info issue_log + = if not Config.nullsafe_annotation_graph then issue_log - else ( - (* TODO: actually construct the graph (just print provisional violations for now) *) - AggregatedSummaries.ClassInfo.get_summaries class_info - |> List.map ~f:(fun NullsafeSummary.{issues} -> issues) - |> List.fold ~init:[] ~f:( @ ) - |> List.iter ~f:process_issue_for_annotation_graph ; - issue_log ) + else + let class_typ_name = Typ.JavaClass class_name in + let provisional_violations = + AggregatedSummaries.ClassInfo.get_summaries class_info + |> List.map ~f:(fun NullsafeSummary.{issues} -> issues) + |> List.concat + |> List.filter_map ~f:ProvisionalViolation.of_issue + in + let annotation_graph = + AnnotationGraph.build_graph tenv class_struct class_typ_name provisional_violations + in + report_annotation_graph source_file class_name class_struct annotation_graph issue_log let analyze_class_impl tenv source_file class_name class_struct class_info issue_log = issue_log |> analyze_meta_issue_for_top_level_class tenv source_file class_name class_struct class_info |> analyze_nullsafe_annotations tenv source_file class_name class_struct - |> construct_annotation_graph class_name class_info + |> build_and_report_annotation_graph tenv source_file class_name class_struct class_info let analyze_class tenv source_file class_info issue_log = diff --git a/infer/src/nullsafe/NullsafeIssue.ml b/infer/src/nullsafe/NullsafeIssue.ml index 0b0828b23..dd36ab73f 100644 --- a/infer/src/nullsafe/NullsafeIssue.ml +++ b/infer/src/nullsafe/NullsafeIssue.ml @@ -93,4 +93,10 @@ let get_nullsafe_extra {third_party_dependent_methods; nullable_methods} proc_na let nullable_methods = if List.is_empty nullable_methods then None else Some (to_nullable_method_json nullable_methods) in - Jsonbug_t.{class_name; package; meta_issue_info= None; unvetted_3rd_party; nullable_methods} + Jsonbug_t. + { class_name + ; package + ; meta_issue_info= None + ; unvetted_3rd_party + ; nullable_methods + ; annotation_graph= None } diff --git a/infer/src/nullsafe/ProvisionalViolation.ml b/infer/src/nullsafe/ProvisionalViolation.ml new file mode 100644 index 000000000..b1c851a75 --- /dev/null +++ b/infer/src/nullsafe/ProvisionalViolation.ml @@ -0,0 +1,25 @@ +(* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + *) + +open! IStd + +type t = + | Assignment of AssignmentRule.ProvisionalViolation.t + | Dereference of DereferenceRule.ProvisionalViolation.t + +let of_issue = function + | TypeErr.Condition_redundant _ + | TypeErr.Field_not_initialized _ + | TypeErr.Inconsistent_subclass _ + | TypeErr.Over_annotation _ -> + None + | TypeErr.Nullable_dereference {dereference_violation} -> + DereferenceRule.ProvisionalViolation.from dereference_violation + |> Option.map ~f:(fun violation -> Dereference violation) + | TypeErr.Bad_assignment {assignment_violation} -> + AssignmentRule.ProvisionalViolation.from assignment_violation + |> Option.map ~f:(fun violation -> Assignment violation) diff --git a/infer/src/nullsafe/ProvisionalViolation.mli b/infer/src/nullsafe/ProvisionalViolation.mli new file mode 100644 index 000000000..498eb4b55 --- /dev/null +++ b/infer/src/nullsafe/ProvisionalViolation.mli @@ -0,0 +1,17 @@ +(* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + *) + +open! IStd + +(** A simple wrapper over provisional violations for Rules *) + +type t = + | Assignment of AssignmentRule.ProvisionalViolation.t + | Dereference of DereferenceRule.ProvisionalViolation.t + +val of_issue : TypeErr.err_instance -> t option +(** If the nullsafe issue is associated with a provisional violation, return it *)