Enforcing subtyping rules for @Expensive and @PerformanceCritical

Summary: public
This adds the following subtyping rules:
- methods that are not annotated with Expensive cannot be overwritten by a method annotated with Expensive
- methods annotated with PerformanceCritical must be overwitten by method annotated with PerformanceCritical

Reviewed By: cristianoc

Differential Revision: D2636076

fb-gh-sync-id: eb616c9

infer/src/checkers/performanceCritical.ml

@@ -10,8 +10,14 @@
 module L = Logging
 
 
-let calls_expensive_method = "CHECKERS_CALLS_EXPENSIVE_METHOD"
+let calls_expensive_method =
+  "CHECKERS_CALLS_EXPENSIVE_METHOD"
 
+let expensive_overrides_unexpensive =
+  "CHECKERS_EXPENSIVE_OVERRIDES_UNANNOTATED"
+
+let unannotated_overrides_performance_critical =
+  "CHECKERS_UNANNOTATED_OVERRIDES_PERFOMANCE_CRITICAL"
 
 let search_expensive_call checked_pnames expensive_callee (pname, _) =
   match expensive_callee with
@@ -47,17 +53,63 @@ let is_expensive attributes =
   check_attributes Annotations.ia_is_expensive attributes
 
 
-let callback_performance_checker _ _ _ tenv pname pdesc : unit =
+let check_method check pname =
+  match AttributesTable.load_attributes pname with
+  | None -> false
+  | Some attributes -> check_attributes check attributes
+
+
+let method_is_performance_critical pname =
+  check_method Annotations.ia_is_performance_critical pname
+
+
+let method_is_expensive pname =
+  check_method Annotations.ia_is_expensive pname
+
+
+let callback_performance_checker _ _ _ 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
   let expensive_call_found =
     let checked_pnames = ref Procname.Set.empty in
     Cfg.Procdesc.fold_calls
       (search_expensive_call checked_pnames)
       None
       pdesc in
+
+  let check_expensive_subtyping_rules overridden_pname =
+    if not (method_is_expensive overridden_pname) then
+      let description =
+        Printf.sprintf
+          "Method %s overrides unannotated method %s and cannot be annotated with @%s"
+          (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
+
+  if expensive then
+    PatternMatch.proc_iter_overridden_methods check_expensive_subtyping_rules tenv pname;
+  if not performance_critical then
+    PatternMatch.proc_iter_overridden_methods check_performance_critical_subtyping_rules tenv pname;
+
   match expensive_call_found with
   | None -> ()
-  | Some callee_pname when is_performance_critical attributes ->
+  | Some callee_pname when performance_critical ->
       let description =
         Printf.sprintf "Method %s annotated with @%s calls method %s annotated with @%s"
           (Procname.to_simplified_string pname)
@@ -65,8 +117,8 @@ let callback_performance_checker _ _ _ tenv pname pdesc : unit =
           (Procname.to_string callee_pname)
           Annotations.expensive in
       Checkers.ST.report_error
-        pname pdesc calls_expensive_method (Cfg.Procdesc.get_loc pdesc) description
-  | Some _ when not (is_expensive attributes) ->
+        pname pdesc calls_expensive_method loc description
+  | Some _ when not expensive ->
       let ret_annot, param_annot = attributes.ProcAttributes.method_annotation in
       let updated_method_annot =
         (Annotations.expensive_annotation, true) :: ret_annot, param_annot in

infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java

@@ -42,4 +42,9 @@ public class ExpensiveCallExample {
     methodWrapper();
   }
 
+  @PerformanceCritical
+  void callingExpensiveMethodFromInterface(ExpensiveInterfaceExample object) {
+    object.m5();
+  }
+
 }

infer/tests/codetoanalyze/java/checkers/ExpensiveInterfaceExample.java

@@ -0,0 +1,38 @@
+/*
+* Copyright (c) 2013 - present Facebook, Inc.
+* All rights reserved.
+*
+* This source code is licensed under the BSD style license found in the
+* LICENSE file in the root directory of this source tree. An additional grant
+* of patent rights can be found in the PATENTS file in the same directory.
+*/
+
+package codetoanalyze.java.checkers;
+
+import com.facebook.infer.annotation.Expensive;
+import com.facebook.infer.annotation.PerformanceCritical;
+
+
+public interface ExpensiveInterfaceExample {
+
+  interface I {
+
+    @PerformanceCritical
+    public void m1();
+
+    public void m2();
+
+  }
+
+  class C {
+
+    public void m3() {}
+
+    public void m4() {}
+
+  }
+
+  @Expensive
+  public void m5();
+
+}

infer/tests/codetoanalyze/java/checkers/ExpensiveSubtypingExample.java

@@ -0,0 +1,21 @@
+/*
+* Copyright (c) 2013 - present Facebook, Inc.
+* All rights reserved.
+*
+* This source code is licensed under the BSD style license found in the
+* LICENSE file in the root directory of this source tree. An additional grant
+* of patent rights can be found in the PATENTS file in the same directory.
+*/
+
+package codetoanalyze.java.checkers;
+
+import com.facebook.infer.annotation.Expensive;
+
+public class ExpensiveSubtypingExample extends ExpensiveInterfaceExample.C {
+
+  @Expensive
+  public void m3() {}
+
+  public void m4() {}
+
+}

infer/tests/codetoanalyze/java/checkers/PerformanceCriticalSubtypingExample.java

@@ -0,0 +1,18 @@
+/*
+* Copyright (c) 2013 - present Facebook, Inc.
+* All rights reserved.
+*
+* This source code is licensed under the BSD style license found in the
+* LICENSE file in the root directory of this source tree. An additional grant
+* of patent rights can be found in the PATENTS file in the same directory.
+*/
+
+package codetoanalyze.java.checkers;
+
+public class PerformanceCriticalSubtypingExample implements ExpensiveInterfaceExample.I {
+
+  public void m1() {}
+
+  public void m2() {}
+
+}

infer/tests/endtoend/java/checkers/ExpensiveCallTest.java

@@ -23,9 +23,10 @@ import utils.InferResults;
 public class ExpensiveCallTest {
 
   public static final String SOURCE_FILE =
-      "infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java";
+    "infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java";
 
-  public static final String CALLS_EXPENSIVE_METHOD = "CHECKERS_CALLS_EXPENSIVE_METHOD";
+  public static final String CALLS_EXPENSIVE_METHOD =
+    "CHECKERS_CALLS_EXPENSIVE_METHOD";
 
   private static InferResults inferResults;
 
@@ -40,7 +41,8 @@ public class ExpensiveCallTest {
       throws IOException, InterruptedException, InferException {
     String[] methods = {
         "directlyCallingExpensiveMethod",
-        "indirectlyCallingExpensiveMethod"
+        "indirectlyCallingExpensiveMethod",
+        "callingExpensiveMethodFromInterface",
     };
     assertThat(
         "Results should contain " + CALLS_EXPENSIVE_METHOD,

infer/tests/endtoend/java/checkers/ExpensiveSubtypingTest.java

@@ -0,0 +1,54 @@
+/*
+* Copyright (c) 2015 - present Facebook, Inc.
+* All rights reserved.
+*
+* This source code is licensed under the BSD style license found in the
+* LICENSE file in the root directory of this source tree. An additional grant
+* of patent rights can be found in the PATENTS file in the same directory.
+*/
+
+package endtoend.java.checkers;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static utils.matchers.ResultContainsExactly.containsExactly;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.IOException;
+
+import utils.InferException;
+import utils.InferResults;
+
+public class ExpensiveSubtypingTest {
+
+  public static final String SOURCE_FILE =
+    "infer/tests/codetoanalyze/java/checkers/ExpensiveSubtypingExample.java";
+
+  public static final String EXPENSIVE_OVERRIDES_UNANNOTATED =
+    "CHECKERS_EXPENSIVE_OVERRIDES_UNANNOTATED";
+
+  private static InferResults inferResults;
+
+  @BeforeClass
+  public static void loadResults() throws InterruptedException, IOException {
+    inferResults =
+      InferResults.loadCheckersResults(ExpensiveSubtypingTest.class, SOURCE_FILE);
+  }
+
+  @Test
+  public void matchErrors()
+    throws IOException, InterruptedException, InferException {
+    String[] methods = {
+      "m3",
+    };
+    assertThat(
+        "Results should contain " + EXPENSIVE_OVERRIDES_UNANNOTATED,
+        inferResults,
+        containsExactly(
+            EXPENSIVE_OVERRIDES_UNANNOTATED,
+            SOURCE_FILE,
+            methods));
+    }
+
+}

infer/tests/endtoend/java/checkers/PerformanceCriticalSubtypingTest.java

@@ -0,0 +1,54 @@
+/*
+* Copyright (c) 2015 - present Facebook, Inc.
+* All rights reserved.
+*
+* This source code is licensed under the BSD style license found in the
+* LICENSE file in the root directory of this source tree. An additional grant
+* of patent rights can be found in the PATENTS file in the same directory.
+*/
+
+package endtoend.java.checkers;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static utils.matchers.ResultContainsExactly.containsExactly;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.IOException;
+
+import utils.InferException;
+import utils.InferResults;
+
+public class PerformanceCriticalSubtypingTest {
+
+  public static final String SOURCE_FILE =
+    "infer/tests/codetoanalyze/java/checkers/PerformanceCriticalSubtypingExample.java";
+
+  public static final String UNANNOTATED_OVERRIDES_PERFOMANCE_CRITICAL =
+    "CHECKERS_UNANNOTATED_OVERRIDES_PERFOMANCE_CRITICAL";
+
+  private static InferResults inferResults;
+
+  @BeforeClass
+  public static void loadResults() throws InterruptedException, IOException {
+    inferResults =
+      InferResults.loadCheckersResults(PerformanceCriticalSubtypingTest.class, SOURCE_FILE);
+  }
+
+  @Test
+  public void matchErrors()
+    throws IOException, InterruptedException, InferException {
+    String[] methods = {
+      "m1",
+    };
+    assertThat(
+        "Results should contain " + UNANNOTATED_OVERRIDES_PERFOMANCE_CRITICAL,
+        inferResults,
+        containsExactly(
+            UNANNOTATED_OVERRIDES_PERFOMANCE_CRITICAL,
+            SOURCE_FILE,
+            methods));
+    }
+
+}