remove subtyping rule for the @PerformanceCritical annotation

Summary:public
Is seems that automatically inheriting annotations like `PerformanceCritical` or `NoAllocation` is the right thing to do in general. Otherwise, we need to enforce sub-typing rules which in the best case just adds a little bit of documentation, but could miss important issues when the code is not fully annotated. I am simplifying this part to avoid adding boilerplate code for the  `NoAllocation` case.

Reviewed By: sblackshear

Differential Revision: D2938627

fb-gh-sync-id: ddb668b
shipit-source-id: ddb668b

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
   | _ -> ()
 
 

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();
+  }
+
 }

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();
+    }
+  }
+
 }

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,

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));
   }
 
 }