fix for duplicate edge bug

Summary:
This fixes a bug introduced in D90081785. We don't want duplicate edges between any pair of nodes and we have an invariant check for that. However, this could be violated in one situation:

If the list of `Symbol_def.with_reason` values returned by `get_sucessors` returns duplicates, this causes us to create duplicate edges. Example for a situation where this can actually happen:

```
class Foo {

  public static function foo(): void {
  }

  public static function bar(): void {
    self::bar();
    self::bar();
  }
}
```

`get_sucessors` for `Foo:foo` will return `Foo::bar` twice.

This diff fixes this by de-duplicating the sucessor list before further processing.

Reviewed By: madgen

Differential Revision: D90251100

fbshipit-source-id: e02e24beec2e56afb88bc9f50b403fedbe245767

Author: frank-emrich

hphp/hack/src/server/serverFindMyTests.ml

@@ -305,13 +305,18 @@ module Symbol_def = struct
         (** Invariant: The symbol's  constructor name matches that of the reason:
             (e.g., a Symbol_def.Method is matched with a Reason.Method) *)
   }
-  [@@deriving show, sexp, hash]
+  [@@deriving show, sexp, hash, ord]
 
   let class_from_name ctx class_name reason : with_strategy =
     let decl = get_class_decl_entry ctx class_name in
     let kind = Folded_class.kind decl in
     let strategy = Expansion_strategy.of_reason reason in
     { symbol_def = Classish { name = class_name; kind }; strategy }
+
+  (* Just here for Hash_set *)
+  module With_reason = struct
+    type t = with_reason [@@deriving sexp, hash, ord]
+  end
 end
 
 open Symbol_def
@@ -1106,8 +1111,13 @@ module Selection_graph = struct
         let new_distance = distance + 1 in
         (match get_successors ~ctx ~genv ~env symbol_s with
         | Result.Ok (referencing_defs, referencing_test_files) ->
-          List.iter
-            referencing_defs
+          (* referencing_defs may include duplicates.
+             We need to skip them to avoid creating duplicate edges between nodes *)
+          let referencing_defs_dedup =
+            Hash_set.of_list (module Symbol_def.With_reason) referencing_defs
+          in
+          Hash_set.iter
+            referencing_defs_dedup
             ~f:(fun { symbol_def = referencing_def; reason } ->
               let referencing_symbol_s : Symbol_def.with_strategy =
                 {

hphp/hack/test/integration/data/find_my_tests_repo/g/G_CallsTwice.php

@@ -0,0 +1,12 @@
+<?hh
+
+class G_CallsTwice {
+  public static function foo(): void {
+    G_Root::duplicateRoot();
+  }
+
+  public static function callsFooTwice(): void {
+    self::foo();
+    self::foo();
+  }
+}

hphp/hack/test/integration/data/find_my_tests_repo/g/G_Cycle.php

@@ -0,0 +1,17 @@
+<?hh
+
+class G_Cycle {
+
+  public static function a(): void {
+    G_Root::cycleRoot();
+    self::c();
+  }
+
+  public static function b(): void {
+    self::a();
+  }
+
+  public static function c(): void {
+    self::b();
+  }
+}

hphp/hack/test/integration/data/find_my_tests_repo/g/G_MultiPath.php

@@ -0,0 +1,23 @@
+<?hh
+
+class G_MultiPath {
+  // Short path: root <- path1 <- join (distance 2 from root to join)
+  public static function path1(): void {
+    G_Root::multiPathRoot();
+  }
+
+  // Long path: root <- path2a <- path2b <- join (distance 3 from root to join)
+  public static function path2a(): void {
+    G_Root::multiPathRoot();
+  }
+
+  public static function path2b(): void {
+    self::path2a();
+  }
+
+  // Join point: called via both paths
+  public static function join(): void {
+    self::path1();
+    self::path2b();
+  }
+}

hphp/hack/test/integration/data/find_my_tests_repo/g/G_Root.php

@@ -0,0 +1,7 @@
+<?hh
+
+class G_Root {
+  public static function duplicateRoot(): void {}
+  public static function cycleRoot(): void {}
+  public static function multiPathRoot(): void {}
+}

hphp/hack/test/integration/data/find_my_tests_repo/g/__tests__/G_CycleTest.php

@@ -0,0 +1,7 @@
+<?hh
+
+class G_CycleTest extends WWWTest {
+  public function test(): void {
+    G_Cycle::b();
+  }
+}

hphp/hack/test/integration/data/find_my_tests_repo/g/__tests__/G_DuplicateTest.php

@@ -0,0 +1,7 @@
+<?hh
+
+class G_DuplicateTest extends WWWTest {
+  public function test(): void {
+    G_CallsTwice::callsFooTwice();
+  }
+}

hphp/hack/test/integration/data/find_my_tests_repo/g/__tests__/G_MultiPathTest.php

@@ -0,0 +1,7 @@
+<?hh
+
+class G_MultiPathTest extends WWWTest {
+  public function test(): void {
+    G_MultiPath::join();
+  }
+}

hphp/hack/test/integration/test_find_my_tests.py

@@ -315,3 +315,48 @@ def test_typedefs_and_typeconsts2(self) -> None:
             ],
             max_distance=10,
         )
+
+    def test_duplicates(self) -> None:
+        """Tests two kinds of duplication:
+        - Passing the same root twice
+        - Having a method call an existing method in the graph twice
+
+        Uses files from g/ subdirectory
+        """
+        prefix = os.path.join(self.test_driver.repo_dir, "g", "__tests__")
+
+        self.check_has_tests_with_distances(
+            prefix=prefix,
+            symbols=["G_Root::duplicateRoot", "G_Root::duplicateRoot"],
+            expected_test_files=[("G_DuplicateTest.php", 3)],
+            max_distance=10,
+        )
+
+    def test_cycle(self) -> None:
+        """Tests that cycles in the call graph do not cause issues.
+
+        Uses files from g/ subdirectory
+        """
+        prefix = os.path.join(self.test_driver.repo_dir, "g", "__tests__")
+
+        self.check_has_tests_with_distances(
+            prefix=prefix,
+            symbols=["G_Root::cycleRoot"],
+            expected_test_files=[("G_CycleTest.php", 3)],
+            max_distance=10,
+        )
+
+    def test_multi_path(self) -> None:
+        """Tests that when there are multiple paths to the same method,
+        we use the shortest distance.
+
+        Uses files from g/ subdirectory.
+        """
+        prefix = os.path.join(self.test_driver.repo_dir, "g", "__tests__")
+
+        self.check_has_tests_with_distances(
+            prefix=prefix,
+            symbols=["G_Root::multiPathRoot"],
+            expected_test_files=[("G_MultiPathTest.php", 3)],
+            max_distance=10,
+        )