diff --git a/infer/src/checkers/performanceCritical.ml b/infer/src/checkers/performanceCritical.ml index d3efa2619..f06c549c8 100644 --- a/infer/src/checkers/performanceCritical.ml +++ b/infer/src/checkers/performanceCritical.ml @@ -9,11 +9,6 @@ module L = Logging -(* Automatically detect if a method is considered as performance critical - by looking for the annotation in the super-types *) -let infer_performance_critical_methods = true - - (* Warning name when a performance critical method directly or indirectly calls a method annotatd as expensive *) let calls_expensive_method = @@ -24,14 +19,6 @@ let calls_expensive_method = let expensive_overrides_unexpensive = "CHECKERS_EXPENSIVE_OVERRIDES_UNANNOTATED" -(* Warning name for the subtyping rule: methods overriding methods annotated as performance critical - should also be annotated as performance critical *) -let unannotated_overrides_performance_critical = - "CHECKERS_UNANNOTATED_OVERRIDES_PERFOMANCE_CRITICAL" - -(* Triggers report on violation of the sybtyping rule for the performance critical annotation *) -let enforce_performance_critical_subtyping_rule = false - let check_attributes check attributes = let annotated_signature = Annotations.get_annotated_signature attributes in @@ -179,7 +166,8 @@ let check_one_procedure tenv pname pdesc = let loc = Cfg.Procdesc.get_loc pdesc in let attributes = Cfg.Procdesc.get_attributes pdesc in let expensive = is_expensive attributes - and performance_critical = is_performance_critical attributes in + and performance_critical = + method_overrides_performance_critical tenv pname in let check_expensive_subtyping_rules overridden_pname = if not (method_is_expensive tenv overridden_pname) then @@ -189,26 +177,14 @@ let check_one_procedure tenv pname pdesc = (Procname.to_string pname) (Procname.to_string overridden_pname) Annotations.expensive in - Checkers.ST.report_error - pname pdesc expensive_overrides_unexpensive loc description - - and check_performance_critical_subtyping_rules overridden_pname = - if method_is_performance_critical overridden_pname then - let description = - Printf.sprintf - "Method `%s` overrides method `%s` annotated with `%s` and should also be annotated" - (Procname.to_string pname) - (Procname.to_string overridden_pname) - Annotations.performance_critical in - Checkers.ST.report_error - pname pdesc unannotated_overrides_performance_critical loc description in + let exn = + Exceptions.Checkers + (expensive_overrides_unexpensive, Localise.verbatim_desc description) in + Reporting.log_error pname ~loc: (Some loc) ~ltr: None exn in if expensive then PatternMatch.proc_iter_overridden_methods check_expensive_subtyping_rules tenv pname; - if enforce_performance_critical_subtyping_rule && not performance_critical then - PatternMatch.proc_iter_overridden_methods - check_performance_critical_subtyping_rules tenv pname; let expensive_call_trees = let checked_pnames = ref Procname.Set.empty in @@ -221,9 +197,6 @@ let check_one_procedure tenv pname pdesc = | [] -> () | call_trees when performance_critical -> report_expensive_calls pname pdesc loc call_trees - | call_trees when infer_performance_critical_methods -> - if method_overrides_performance_critical tenv pname then - report_expensive_calls pname pdesc loc call_trees | _ -> () diff --git a/infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java b/infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java index 767556451..79d26cfce 100644 --- a/infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java +++ b/infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java @@ -17,6 +17,14 @@ import com.facebook.infer.annotation.Expensive; import com.facebook.infer.annotation.PerformanceCritical; +interface AnnotatedInterface { + + @PerformanceCritical + void annotatedPerformanceCriticalInInterface(); + +} + + class Other { @Expensive @@ -30,16 +38,14 @@ class Other { } -public class ExpensiveCallExample { +public class ExpensiveCallExample implements AnnotatedInterface { Other mOther; void nonExpensiveMethod() {} @Expensive - void expensiveMethod() { - mOther = new Other(); - } + void expensiveMethod() {} void methodWrapper() { expensiveMethod(); @@ -84,4 +90,8 @@ public class ExpensiveCallExample { return activity.findViewById(id); } + public void annotatedPerformanceCriticalInInterface() { + mOther.callsExpensive1(); + } + } diff --git a/infer/tests/codetoanalyze/java/checkers/ExpensiveInheritanceExample.java b/infer/tests/codetoanalyze/java/checkers/ExpensiveInheritanceExample.java index c6f1c423e..7f681ef1c 100644 --- a/infer/tests/codetoanalyze/java/checkers/ExpensiveInheritanceExample.java +++ b/infer/tests/codetoanalyze/java/checkers/ExpensiveInheritanceExample.java @@ -18,7 +18,9 @@ interface I { class A implements I { - @SuppressWarnings("infer") // In case we really want to implement foo as expensive + @SuppressWarnings("infer") + // Suppressing the sub-typing violation warning here as foo() is not annotated as @Expensive + // in the interface. This report is legit but is not relevant for the current test. @Expensive public void foo() {} @@ -30,25 +32,30 @@ class B extends A implements I { public class ExpensiveInheritanceExample { + // The objective of this test is to document the limitations of the checker, which just + // implements a type system. This means that the checker is flow insensitive and is only based + // on the static type information. Especially, it does not try to resolve dynamic dispatch. + // However, the checker is still exhaustive thanks to the sub-typing rule for + // the @Expensive annotation. @PerformanceCritical void shouldNotReportBecauseInterfaceNotAnnotated(I i) { i.foo(); } @PerformanceCritical - void reportsBecauseFooIsExpensiveInA() { - A a = new A(); + void reportsBecauseFooIsExpensiveInA(A a) { a.foo(); } @PerformanceCritical - void doesNotreportBecauseFooIsNotExpensiveInB() { - A a = new B(); - a.foo(); + void doesNotreportBecauseFooIsNotExpensiveInB(B b) { + b.foo(); } + native B createB(); + A actuallyReturnsObjectOfTypeB() { - return new B(); + return createB(); } @PerformanceCritical @@ -57,4 +64,11 @@ public class ExpensiveInheritanceExample { a.foo(); } + @PerformanceCritical + void doesReportBecauseFlowInsensitive(A a) { + if (a instanceof B) { + a.foo(); + } + } + } diff --git a/infer/tests/endtoend/java/checkers/ExpensiveCallTest.java b/infer/tests/endtoend/java/checkers/ExpensiveCallTest.java index 2b7678bf8..92834502b 100644 --- a/infer/tests/endtoend/java/checkers/ExpensiveCallTest.java +++ b/infer/tests/endtoend/java/checkers/ExpensiveCallTest.java @@ -46,6 +46,7 @@ public class ExpensiveCallTest { "longerCallStackToExpensive", "callsFindViewByIdFromView", "callsFindViewByIdFromActivity", + "annotatedPerformanceCriticalInInterface", }; assertThat( "Results should contain " + CALLS_EXPENSIVE_METHOD, diff --git a/infer/tests/endtoend/java/checkers/ExpensiveInheritanceTest.java b/infer/tests/endtoend/java/checkers/ExpensiveInheritanceTest.java index 83e7bbf9b..38d69f480 100644 --- a/infer/tests/endtoend/java/checkers/ExpensiveInheritanceTest.java +++ b/infer/tests/endtoend/java/checkers/ExpensiveInheritanceTest.java @@ -38,18 +38,17 @@ public class ExpensiveInheritanceTest { @Test public void matchErrors() - throws IOException, InterruptedException, InferException { + throws IOException, InterruptedException, InferException { String[] methods = { - "reportsBecauseFooIsExpensiveInA", - "reportsAssumingObjectOfTypeA", + "reportsBecauseFooIsExpensiveInA", + "reportsAssumingObjectOfTypeA", + "doesReportBecauseFlowInsensitive", }; - assertThat( - "Results should contain " + CALLS_EXPENSIVE_METHOD, - inferResults, - containsExactly( - CALLS_EXPENSIVE_METHOD, - SOURCE_FILE, - methods)); + assertThat("Results should contain " + CALLS_EXPENSIVE_METHOD, + inferResults, + containsExactly(CALLS_EXPENSIVE_METHOD, + SOURCE_FILE, + methods)); } }