diff --git a/infer/src/IR/Fieldname.ml b/infer/src/IR/Fieldname.ml index 2069f8d02..16adffdec 100644 --- a/infer/src/IR/Fieldname.ml +++ b/infer/src/IR/Fieldname.ml @@ -18,6 +18,8 @@ let get_field_name {field_name} = field_name let is_java {class_name} = Typ.Name.Java.is_class class_name +let is_java_synthetic t = is_java t && String.contains (get_field_name t) '$' + module T = struct type nonrec t = t [@@deriving compare] end diff --git a/infer/src/IR/Fieldname.mli b/infer/src/IR/Fieldname.mli index 22de15723..6c8ae97bc 100644 --- a/infer/src/IR/Fieldname.mli +++ b/infer/src/IR/Fieldname.mli @@ -20,6 +20,9 @@ val get_field_name : t -> string val is_java : t -> bool +val is_java_synthetic : t -> bool +(** Check if the field is autogenerated/synthetic **) + (** Set for fieldnames *) module Set : Caml.Set.S with type elt = t diff --git a/infer/src/nullsafe/AnnotatedSignature.ml b/infer/src/nullsafe/AnnotatedSignature.ml index 206124deb..29ef65d75 100644 --- a/infer/src/nullsafe/AnnotatedSignature.ml +++ b/infer/src/nullsafe/AnnotatedSignature.ml @@ -121,12 +121,14 @@ let get_for_class_under_analysis tenv proc_attributes = {result with nullsafe_mode} +let pp_ia fmt ia = if not (List.is_empty ia) then F.fprintf fmt "%a " Annot.Item.pp ia + +let pp_annotated_param fmt {mangled; param_annotation_deprecated; param_annotated_type} = + F.fprintf fmt " %a%a %a" pp_ia param_annotation_deprecated AnnotatedType.pp param_annotated_type + Mangled.pp mangled + + let pp proc_name fmt annotated_signature = - let pp_ia fmt ia = if not (List.is_empty ia) then F.fprintf fmt "%a " Annot.Item.pp ia in - let pp_annotated_param fmt {mangled; param_annotation_deprecated; param_annotated_type} = - F.fprintf fmt " %a%a %a" pp_ia param_annotation_deprecated AnnotatedType.pp param_annotated_type - Mangled.pp mangled - in let {ret_annotation_deprecated; ret_annotated_type} = annotated_signature.ret in F.fprintf fmt "[%a] %a%a %a (%a )" NullsafeMode.pp annotated_signature.nullsafe_mode pp_ia ret_annotation_deprecated AnnotatedType.pp ret_annotated_type diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index 4cba41c43..28617cd3c 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/infer/src/nullsafe/typeErr.ml @@ -82,6 +82,34 @@ type err_instance = ; assignment_type: AssignmentRule.ReportableViolation.assignment_type } [@@deriving compare] +(** Returns whether we can be certain that an [err_instance] is related to synthetic code, + autogen/codegen). + + NOTE: This implementation is very naive and handles only simplest cases (which is actually + enough for our use-case at the moment). The problem here is that in general: + + - any expression that has synthetic code inside can be considered "tainted", + - any expression with a "tainted" sub-expression is also "tainted", + - with "tainted" expressions all bets are off and error reporting should be suppressed. + + We could handle this properly by introducing [Nullability.Synthetic] but this is quite involved + (and hopefully won't be needed). *) +let is_synthetic_err = function + | Bad_assignment {assignment_type; _} -> ( + AssignmentRule.ReportableViolation.( + match assignment_type with + | PassingParamToFunction {function_procname; _} -> + (* NOTE: this currently doesn't cover the case when the actual + argument involves syntethic code *) + Procname.is_java_autogen_method function_procname + | AssigningToField fieldname -> + Fieldname.is_java_synthetic fieldname + | _ -> + false) ) + | _ -> + false + + let pp_err_instance fmt err_instance = match err_instance with | Condition_redundant _ -> @@ -216,86 +244,88 @@ let get_nonnull_explanation_for_condition_redudant (nonnull_origin : TypeOrigin. (** If error is reportable to the user, return its information. Otherwise return [None]. *) let get_error_info_if_reportable_lazy ~nullsafe_mode err_instance = let open IOption.Let_syntax in - match err_instance with - | Condition_redundant {is_always_true; condition_descr; nonnull_origin} -> - Some - ( lazy - ( P.sprintf "The condition %s might be always %b%s." - (Option.value condition_descr ~default:"") - is_always_true - (get_nonnull_explanation_for_condition_redudant nonnull_origin) - , IssueType.eradicate_condition_redundant - , None - , (* 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. - Until it is made non-precise, it is recommended to not turn this warning on. - But even when it is on, this should not be more than advice. - *) - IssueType.Advice ) ) - | Over_annotation {over_annotated_violation; violation_type} -> - Some - ( lazy - ( OverAnnotatedRule.violation_description over_annotated_violation violation_type + if is_synthetic_err err_instance then None + else + match err_instance with + | Condition_redundant {is_always_true; condition_descr; nonnull_origin} -> + Some + ( lazy + ( P.sprintf "The condition %s might be always %b%s." + (Option.value condition_descr ~default:"") + is_always_true + (get_nonnull_explanation_for_condition_redudant nonnull_origin) + , IssueType.eradicate_condition_redundant + , None + , (* 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. + Until it is made non-precise, it is recommended to not turn this warning on. + But even when it is on, this should not be more than advice. + *) + IssueType.Advice ) ) + | Over_annotation {over_annotated_violation; violation_type} -> + Some + ( lazy + ( OverAnnotatedRule.violation_description over_annotated_violation violation_type + , ( match violation_type with + | OverAnnotatedRule.FieldOverAnnoted _ -> + IssueType.eradicate_field_over_annotated + | OverAnnotatedRule.ReturnOverAnnotated _ -> + IssueType.eradicate_return_over_annotated ) + , None + , (* Very non-precise issue. Should be actually turned off unless for experimental purposes. *) + IssueType.Advice ) ) + | Field_not_initialized {field_name} -> + Some + ( lazy + ( Format.asprintf + "Field %a is declared non-nullable, so it should be initialized in the constructor \ + or in an `@Initializer` method" + MF.pp_monospaced + (Fieldname.get_field_name field_name) + , IssueType.eradicate_field_not_initialized + , None + , NullsafeMode.severity nullsafe_mode ) ) + | Bad_assignment {assignment_location; assignment_type; assignment_violation} -> + (* If violation is reportable, create tuple, otherwise None *) + let+ reportable_violation = + AssignmentRule.ReportableViolation.from nullsafe_mode assignment_violation + in + lazy + (let description, issue_type, error_location = + AssignmentRule.ReportableViolation.get_description ~assignment_location assignment_type + reportable_violation + in + let severity = AssignmentRule.ReportableViolation.get_severity reportable_violation in + (description, issue_type, Some error_location, severity) ) + | Nullable_dereference + {dereference_violation; dereference_location; nullable_object_descr; dereference_type} -> + (* If violation is reportable, create tuple, otherwise None *) + let+ reportable_violation = + DereferenceRule.ReportableViolation.from nullsafe_mode dereference_violation + in + lazy + (let description, issue_type, error_location = + DereferenceRule.ReportableViolation.get_description reportable_violation + ~dereference_location dereference_type ~nullable_object_descr + in + let severity = DereferenceRule.ReportableViolation.get_severity reportable_violation in + (description, issue_type, Some error_location, severity) ) + | Inconsistent_subclass + {inheritance_violation; violation_type; base_proc_name; overridden_proc_name} -> + (* If violation is reportable, create tuple, otherwise None *) + let+ reportable_violation = + InheritanceRule.ReportableViolation.from nullsafe_mode inheritance_violation + in + lazy + ( InheritanceRule.ReportableViolation.get_description reportable_violation violation_type + ~base_proc_name ~overridden_proc_name , ( match violation_type with - | OverAnnotatedRule.FieldOverAnnoted _ -> - IssueType.eradicate_field_over_annotated - | OverAnnotatedRule.ReturnOverAnnotated _ -> - IssueType.eradicate_return_over_annotated ) - , None - , (* Very non-precise issue. Should be actually turned off unless for experimental purposes. *) - IssueType.Advice ) ) - | Field_not_initialized {field_name} -> - Some - ( lazy - ( Format.asprintf - "Field %a is declared non-nullable, so it should be initialized in the constructor \ - or in an `@Initializer` method" - MF.pp_monospaced - (Fieldname.get_field_name field_name) - , IssueType.eradicate_field_not_initialized + | InconsistentReturn -> + IssueType.eradicate_inconsistent_subclass_return_annotation + | InconsistentParam _ -> + IssueType.eradicate_inconsistent_subclass_parameter_annotation ) , None - , NullsafeMode.severity nullsafe_mode ) ) - | Bad_assignment {assignment_location; assignment_type; assignment_violation} -> - (* If violation is reportable, create tuple, otherwise None *) - let+ reportable_violation = - AssignmentRule.ReportableViolation.from nullsafe_mode assignment_violation - in - lazy - (let description, issue_type, error_location = - AssignmentRule.ReportableViolation.get_description ~assignment_location assignment_type - reportable_violation - in - let severity = AssignmentRule.ReportableViolation.get_severity reportable_violation in - (description, issue_type, Some error_location, severity) ) - | Nullable_dereference - {dereference_violation; dereference_location; nullable_object_descr; dereference_type} -> - (* If violation is reportable, create tuple, otherwise None *) - let+ reportable_violation = - DereferenceRule.ReportableViolation.from nullsafe_mode dereference_violation - in - lazy - (let description, issue_type, error_location = - DereferenceRule.ReportableViolation.get_description reportable_violation - ~dereference_location dereference_type ~nullable_object_descr - in - let severity = DereferenceRule.ReportableViolation.get_severity reportable_violation in - (description, issue_type, Some error_location, severity) ) - | Inconsistent_subclass - {inheritance_violation; violation_type; base_proc_name; overridden_proc_name} -> - (* If violation is reportable, create tuple, otherwise None *) - let+ reportable_violation = - InheritanceRule.ReportableViolation.from nullsafe_mode inheritance_violation - in - lazy - ( InheritanceRule.ReportableViolation.get_description reportable_violation violation_type - ~base_proc_name ~overridden_proc_name - , ( match violation_type with - | InconsistentReturn -> - IssueType.eradicate_inconsistent_subclass_return_annotation - | InconsistentParam _ -> - IssueType.eradicate_inconsistent_subclass_parameter_annotation ) - , None - , InheritanceRule.ReportableViolation.get_severity reportable_violation ) + , InheritanceRule.ReportableViolation.get_severity reportable_violation ) (** If error is reportable to the user, return description, severity etc. Otherwise return None. *) diff --git a/infer/tests/codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java b/infer/tests/codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java new file mode 100644 index 000000000..951f168b3 --- /dev/null +++ b/infer/tests/codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java @@ -0,0 +1,63 @@ +/* + * 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. + */ + +package codetoanalyze.java.nullsafe; + +import android.annotation.SuppressLint; +import javax.annotation.Nullable; + +public class SyntheticErrorSuppressions { + static class Fragment { + @SuppressLint("ReturnOverAnnotated") + @Nullable + public static Object getContext() { + return new Object(); + } + + public static void setContext(final Object context) {} + } + + static class ClassWithSyntheticCode { + @Nullable private Object $ul_fieldNullable; + private Object $ul_fieldNotNull; + + // This method contains $ which is *extremely* rarely used in user code, but + // used extensively in autogenerated code, therefore is good marker. + private static void $ul_iAmAutogen( + final ClassWithSyntheticCode instance, final Object context) {} + + @Nullable + private Object $ul_iAmNullableAutogen() { + return $ul_fieldNullable; + } + + public void passingIncorrectParamToSyntheticMethod_OK() { + $ul_iAmAutogen(this, Fragment.getContext()); + } + + public void assigningAnythingToSyntheticField_OK() { + $ul_fieldNotNull = null; + } + + // Following cases are hard to support since synthetic code can be a part of + // a complex expression, and we need some more sophisticated mechanisms to + // handle that. On the bright side, this is not something that happens often + // in the wild. + + public void passingSyntheticParamToAnyMethod_OK_FP() { + Fragment.setContext($ul_fieldNullable); + } + + public String dereferencingSyntheticNullableField_OK_FP() { + return $ul_fieldNullable.toString(); + } + + public String dereferencingNullableSyntheticMethodCall_OK_FP() { + return $ul_iAmNullableAutogen().toString(); + } + } +} diff --git a/infer/tests/codetoanalyze/java/nullsafe/issues.exp b/infer/tests/codetoanalyze/java/nullsafe/issues.exp index 4b4bf9561..c94894b40 100644 --- a/infer/tests/codetoanalyze/java/nullsafe/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe/issues.exp @@ -352,6 +352,10 @@ codetoanalyze/java/nullsafe/SwitchCase.java, Linters_dummy_method, 63, ERADICATE codetoanalyze/java/nullsafe/SwitchCase.java, codetoanalyze.java.nullsafe.SwitchCase.getNullableColor():codetoanalyze.java.nullsafe.Color, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, ADVICE, [Method `getNullableColor()` is annotated with `@Nullable` but never returns null.] codetoanalyze/java/nullsafe/SwitchCase.java, codetoanalyze.java.nullsafe.SwitchCase.switchOnNullIsBad():java.lang.String, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [NullPointerException will be thrown at this line! `color` is `null` and is dereferenced via calling `ordinal()`: null constant at line 14.] codetoanalyze/java/nullsafe/SwitchCase.java, codetoanalyze.java.nullsafe.SwitchCase.switchOnNullableIsBad():java.lang.String, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`color` is nullable and is not locally checked for null when calling `ordinal()`: call to getNullableColor() at line 28.] +codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java, Linters_dummy_method, 13, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], SyntheticErrorSuppressions, codetoanalyze.java.nullsafe, issues: 3, curr_mode: "Default" +codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java, codetoanalyze.java.nullsafe.SyntheticErrorSuppressions$ClassWithSyntheticCode.dereferencingNullableSyntheticMethodCall_OK_FP():java.lang.String, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`$ul_iAmNullableAutogen(...)` is nullable and is not locally checked for null when calling `toString()`: call to $ul_iAmNullableAutogen() at line 60.] +codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java, codetoanalyze.java.nullsafe.SyntheticErrorSuppressions$ClassWithSyntheticCode.dereferencingSyntheticNullableField_OK_FP():java.lang.String, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`SyntheticErrorSuppressions$ClassWithSyntheticCode.$ul_fieldNullable` is nullable and is not locally checked for null when calling `toString()`.] +codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java, codetoanalyze.java.nullsafe.SyntheticErrorSuppressions$ClassWithSyntheticCode.passingSyntheticParamToAnyMethod_OK_FP():void, 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`SyntheticErrorSuppressions$Fragment.setContext(...)`: parameter #1(`context`) is declared non-nullable but the argument `SyntheticErrorSuppressions$ClassWithSyntheticCode.$ul_fieldNullable` is nullable.] codetoanalyze/java/nullsafe/TrueFalseOnNull.java, Linters_dummy_method, 17, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], TrueFalseOnNull, codetoanalyze.java.nullsafe, issues: 17, curr_mode: "Default" codetoanalyze/java/nullsafe/TrueFalseOnNull.java, codetoanalyze.java.nullsafe.TrueFalseOnNull$EarlyReturn.testEarlyReturn(java.lang.CharSequence,java.lang.CharSequence):void, 6, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`s2` is nullable and is not locally checked for null when calling `toString()`.] codetoanalyze/java/nullsafe/TrueFalseOnNull.java, codetoanalyze.java.nullsafe.TrueFalseOnNull$TestNonStaticOneParam.falseOnNullNegativeBranchIsBAD(java.lang.String):void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`s` is nullable and is not locally checked for null when calling `toString()`.]