From dc16825ba79cb1c7125e2b83c82773b1e7b7141f Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Fri, 6 Nov 2020 04:32:06 -0800 Subject: [PATCH] [nullsafe][annotation graph] Support `Object.equals()` Summary: There is a feature in Nullsafe that is interfering with "annotation graph" feature. Because of this we would not detect provisional violations for misuses of params of equals() (They will be recorded as user facing rather than provisional issues). This diff turns this feature off for annotation graph mode. Reviewed By: artempyanykh Differential Revision: D24726655 fbshipit-source-id: 4b7577667 --- infer/src/nullsafe/eradicate.ml | 15 +++++++-- .../AnnotationGraph.java | 6 ++++ .../java/nullsafe-annotation-graph/issues.exp | 32 +++++++++++++------ 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/infer/src/nullsafe/eradicate.ml b/infer/src/nullsafe/eradicate.ml index bc0a90775..b13da55d3 100644 --- a/infer/src/nullsafe/eradicate.ml +++ b/infer/src/nullsafe/eradicate.ml @@ -25,8 +25,19 @@ let callback1 ({IntraproceduralAnalysis.proc_desc= curr_pdesc; _} as analysis_da has very special meaning and nullability limitations (it can never be null). *) TypeOrigin.This - else if PatternMatch.Java.is_override_of_lang_object_equals curr_pname then - TypeOrigin.CurrMethodParameter ObjectEqualsOverride + (* An extra feature of Nullsafe. If the method overrides `Object.equals()`, but does not specify @Nullable annotation for the param, + a normal "inconsistent subclass parameter annotation" will be issued. + But apart from this we want to issue additional violations every time the value is used as a non-nullable. + This is to distinct "benign" cases where the value is simply not annotated from real bugs where e.g. the value is actually dereferenced. + We achieve this via having a special TypeOrigin for this param, which will be considered as nullable in relevant places. + *) + else if + PatternMatch.Java.is_override_of_lang_object_equals curr_pname + && (* Turn this feature off for annotation graph mode. In this mode not annotated, but potentially nullable params + are treated in special way, and this conflicts with the trick that is being made here. + *) + not Config.nullsafe_annotation_graph + then TypeOrigin.CurrMethodParameter ObjectEqualsOverride else TypeOrigin.CurrMethodParameter (Normal param_signature) in let inferred_nullability = InferredNullability.create origin in diff --git a/infer/tests/codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java b/infer/tests/codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java index d10a05ec9..a6902e9b8 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java +++ b/infer/tests/codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java @@ -72,6 +72,12 @@ public class AnnotationGraph { methodC().toString(); // violation for methodC } + + @Override + public boolean equals(Object obj) { + // violation for obj + return toString() == obj.toString(); + } } class SomeExternalClass { diff --git a/infer/tests/codetoanalyze/java/nullsafe-annotation-graph/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-annotation-graph/issues.exp index 3d7519940..b80ec7b50 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-annotation-graph/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-annotation-graph/issues.exp @@ -19,20 +19,20 @@ AnnotationGraph: kind: Field field_name: fieldD num_violations: 1 - dependent_point_ids: [m3] + dependent_point_ids: [m4] Annotation point: - id: m3 + id: m4 kind: Method method_info: method_name: methodA params: java.lang.String, boolean access_level: Private num_violations: 0 - dependent_point_ids: [m5] + dependent_point_ids: [m6] Annotation point: - id: m5 + id: m6 kind: Method method_info: method_name: methodB @@ -42,7 +42,7 @@ AnnotationGraph: dependent_point_ids: [] Annotation point: - id: m6 + id: m7 kind: Method method_info: method_name: methodC @@ -52,7 +52,18 @@ AnnotationGraph: dependent_point_ids: [] Annotation point: - id: p4 + id: p3 + kind: Param + method_info: + method_name: equals + params: java.lang.Object + access_level: Public + param_num: 0 + num_violations: 1 + dependent_point_ids: [] + + Annotation point: + id: p5 kind: Param method_info: method_name: methodA @@ -60,11 +71,11 @@ AnnotationGraph: access_level: Private param_num: 0 num_violations: 0 - dependent_point_ids: [f0, m3] + dependent_point_ids: [f0, m4] -codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java, Linters_dummy_method, 12, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], AnnotationGraph, codetoanalyze.java.nullsafe_annotation_graph, issues: 3, curr_mode: "Default" -codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java, Linters_dummy_method, 77, ERADICATE_ANNOTATION_GRAPH, no_bucket, INFO, [], SomeExternalClass, codetoanalyze.java.nullsafe_annotation_graph +codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java, Linters_dummy_method, 12, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], AnnotationGraph, codetoanalyze.java.nullsafe_annotation_graph, issues: 4, curr_mode: "Default" +codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java, Linters_dummy_method, 83, ERADICATE_ANNOTATION_GRAPH, no_bucket, INFO, [], SomeExternalClass, codetoanalyze.java.nullsafe_annotation_graph AnnotationGraph: Annotation point: id: p0 @@ -78,7 +89,8 @@ AnnotationGraph: dependent_point_ids: [] -codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java, Linters_dummy_method, 77, ERADICATE_META_CLASS_CAN_BE_NULLSAFE, no_bucket, ADVICE, [Congrats! `SomeExternalClass` is free of nullability issues. Mark it `@Nullsafe(Nullsafe.Mode.LOCAL)` to prevent regressions.], SomeExternalClass, codetoanalyze.java.nullsafe_annotation_graph, issues: 0, curr_mode: "Default", promote_mode: "Strict" +codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java, Linters_dummy_method, 83, ERADICATE_META_CLASS_CAN_BE_NULLSAFE, no_bucket, ADVICE, [Congrats! `SomeExternalClass` is free of nullability issues. Mark it `@Nullsafe(Nullsafe.Mode.LOCAL)` to prevent regressions.], SomeExternalClass, codetoanalyze.java.nullsafe_annotation_graph, issues: 0, curr_mode: "Default", promote_mode: "Strict" codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java, codetoanalyze.java.nullsafe_annotation_graph.AnnotationGraph.(), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `fieldD` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method], AnnotationGraph, codetoanalyze.java.nullsafe_annotation_graph codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java, codetoanalyze.java.nullsafe_annotation_graph.AnnotationGraph.(), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `fieldB` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method], AnnotationGraph, codetoanalyze.java.nullsafe_annotation_graph codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java, codetoanalyze.java.nullsafe_annotation_graph.AnnotationGraph.(), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `fieldA` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method], AnnotationGraph, codetoanalyze.java.nullsafe_annotation_graph +codetoanalyze/java/nullsafe-annotation-graph/AnnotationGraph.java, codetoanalyze.java.nullsafe_annotation_graph.AnnotationGraph.equals(java.lang.Object):boolean, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [Parameter `obj` is missing `@Nullable` declaration: according to the Java Specification, for any object `x` call `x.equals(null)` should properly return false.], AnnotationGraph, codetoanalyze.java.nullsafe_annotation_graph