Add synthetic triggered dependencies for FunctionDefinitions

Summary:
I've been debugging a Pyre incremental problem for the past day, which I can
repro at-large (i.e. it's repeatable) but so far I can't actually get a small
repro which makes it difficult to pin down.

The issue we were seeing is that after applying a `pyre-upgrade` to a big
codebase, there were many files where we would have both an unused ignore
*and* the original error reported!

I eventually realized the line numbers were mismatched; the actual type
errors had incorrect locations, whereas the unused ignores matched the
updated source file. This indicates that TypeEnvironment had errors from the
old code, and then ErrorsEnvironment / Postprocessing was applying suppressions
using the current raw source against these out-of-date type errors.

---------------

I still do not know *exactly* why this happens at the level of detail I would like to; after
skimming the code, it appears to me that we should be getting triggered dependencies
for `FunctionDefinitionEnvironment` reads, which means any previously-checked module
ought to be re-checked after the raw source changes.

But I did notice two things:
- there was exactly one place where UnannotatedGlobalEnvironment is treated differently
  than FunctionDefinitionEnvironment, which is that AstEnvironment *forces* triggered
  dependencies for all invalidated modules.
- while I still can't figure out why this would be necessary for pre-existing sources
  (I need to debug more, but I also need to ship a fix), I have confirmed that with
  this diff the bug disappears.
- In addition, I actually *can* see why this change is needed when *new* sources pop
  up: if we don't force a "fake" triggered dependency, we wind up not type checking
  new source files.

I'm going ahead and putting this up for review because even if I don't fully understand
the bug yet, I know that (a) this change fixes the problem, and (b) this change is necessary
in any case because of the new-source-file issue.

------------------

I filed T196249034 to keep investigating. I think it's pretty important to determine why
dependency tracked table reads aren't sufficient to prevent this kind of problem, but it
may take some time because so far I've failed to make a trivial repro.

Once I figure out the root cause, I should either simplify (if possible) or clearly document
exactly what the control flow is that makes this hook necessary.

Reviewed By: grievejia

Differential Revision: D59993520

fbshipit-source-id: 5beac07fd3b700b3ef90492094e7cf205b393ebc

source/analysis/astEnvironment.ml

@@ -204,17 +204,32 @@ module FromReadOnlyUpstream = struct
       let initial_invalidated_modules =
         RawSources.KeySet.of_list invalidated_modules_before_preprocessing
       in
+      (* In order to guarantee that we re-check all potentially-changed modules in incremental
+         update, we have to trigger dependencies manually for all invalidated raw sources.
+
+         TODO(T196249034) Investigate the bugs that appear if we remove this this. We used to filter
+         a lot of dependencies which made this obviously necessary, but it is no longer clear why we
+         need it today. *)
       let initial_triggered_dependencies =
         let register_module_components qualifier =
           SharedMemoryKeys.ComputeModuleComponents qualifier
           |> SharedMemoryKeys.DependencyKey.Registry.register
         in
+        let register_function_definitions qualifier =
+          SharedMemoryKeys.FunctionDefinitions qualifier
+          |> SharedMemoryKeys.DependencyKey.Registry.register
+        in
         List.map invalidated_modules_before_preprocessing ~f:register_module_components
+        @ List.map invalidated_modules_before_preprocessing ~f:register_function_definitions
         |> SharedMemoryKeys.DependencyKey.RegisteredSet.of_list
       in
       let fold_key registered (triggered_dependencies_sofar, invalidated_modules_sofar) =
+        (* For invalidated modules, count not only the modules whose raw source changed but also any
+           module where a processed source changed. This is necessary for `invalidated_modules` to
+           include downstream code with wildcard imports *)
         match SharedMemoryKeys.DependencyKey.get_key registered with
-        | SharedMemoryKeys.ComputeModuleComponents qualifier ->
+        | SharedMemoryKeys.ComputeModuleComponents qualifier
+        | SharedMemoryKeys.FunctionDefinitions qualifier ->
             ( SharedMemoryKeys.DependencyKey.RegisteredSet.add
                 registered
                 triggered_dependencies_sofar,

source/analysis/test/astEnvironmentTest.ml

@@ -267,7 +267,11 @@ let test_parse_sources context =
   assert_equal
     ~printer:[%show: SharedMemoryKeys.dependency list]
     ~cmp:[%compare.equal: SharedMemoryKeys.dependency list]
-    [SharedMemoryKeys.ComputeModuleComponents !&"c"; type_check_foo]
+    [
+      SharedMemoryKeys.ComputeModuleComponents !&"c";
+      SharedMemoryKeys.FunctionDefinitions !&"c";
+      type_check_foo;
+    ]
     triggered_dependencies;
   (* Add some new modules and verify the update *)
   assert_equal

source/analysis/test/unannotatedGlobalEnvironmentTest.ml

@@ -516,7 +516,7 @@ let assert_update
     ~original_sources
     ~new_sources
     ~middle_queries_with_expectations
-    ~expected_triggers
+    ~expected_triggers_except_uge_and_fde
     ?post_queries_with_expectations
     context
   =
@@ -651,14 +651,16 @@ let assert_update
     |> List.map ~f:SharedMemoryKeys.DependencyKey.get_key
     |> List.to_string ~f:SharedMemoryKeys.show_dependency
   in
-  let expected_trigger_set =
-    SharedMemoryKeys.DependencyKey.RegisteredSet.of_list expected_triggers
+  let expected_trigger_set_except_uge_and_fde =
+    SharedMemoryKeys.DependencyKey.RegisteredSet.of_list expected_triggers_except_uge_and_fde
   in
-  let actual_downstream_trigger_set =
+  let actual_trigger_set_except_uge_and_fde =
     let fold_trigger_set sofar triggered =
       let fold_one registered sofar =
         match SharedMemoryKeys.DependencyKey.get_key registered with
-        | ComputeModuleComponents _ -> sofar
+        | ComputeModuleComponents _
+        | FunctionDefinitions _ ->
+            sofar
         | _ -> SharedMemoryKeys.DependencyKey.RegisteredSet.add registered sofar
       in
       SharedMemoryKeys.DependencyKey.RegisteredSet.fold fold_one triggered sofar
@@ -672,8 +674,8 @@ let assert_update
   assert_equal
     ~cmp:SharedMemoryKeys.DependencyKey.RegisteredSet.equal
     ~printer
-    expected_trigger_set
-    actual_downstream_trigger_set
+    expected_trigger_set_except_uge_and_fde
+    actual_trigger_set_except_uge_and_fde
 
 
 let type_check_dependency =
@@ -694,59 +696,59 @@ let test_get_explicit_module_metadata =
            ~original_sources:[AssertUpdateSource.Local ("a.py", "x: int = 1")]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)]
            ~new_sources:[AssertUpdateSource.Local ("a.py", "x: int = 1")]
-           ~expected_triggers:[]
+           ~expected_triggers_except_uge_and_fde:[]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:[AssertUpdateSource.External ("a.py", "x: int = 1")]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)]
            ~new_sources:[AssertUpdateSource.External ("a.py", "x: int = 1")]
-           ~expected_triggers:[]
+           ~expected_triggers_except_uge_and_fde:[]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)];
       (* Updated code *)
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:[AssertUpdateSource.Local ("a.py", "x: int = 1")]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)]
            ~new_sources:[AssertUpdateSource.Local ("a.py", "a: int = 2")]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:[AssertUpdateSource.External ("a.py", "x: int = 1")]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)]
            ~new_sources:[AssertUpdateSource.External ("a.py", "a: int = 2")]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)];
       (* Deleted code *)
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:[AssertUpdateSource.Local ("a.py", "x: int = 1")]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)]
            ~new_sources:[]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `None)];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:[AssertUpdateSource.External ("a.py", "x: int = 1")]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)]
            ~new_sources:[]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `None)];
       (* Added code *)
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:[]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `None)]
            ~new_sources:[AssertUpdateSource.Local ("a.py", "x: int = 1")]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:[]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `None)]
            ~new_sources:[AssertUpdateSource.External ("a.py", "x: int = 1")]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Explicit)];
     ]
 
@@ -761,23 +763,23 @@ let test_get_implicit_module_metadata =
            ~original_sources:[AssertUpdateSource.Local ("a/b.py", "x: int = 1")]
            ~new_sources:[AssertUpdateSource.Local ("a/b.py", "x: int = 2")]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Implicit)]
-           ~expected_triggers:[]
+           ~expected_triggers_except_uge_and_fde:[]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Implicit)];
       (* Deleted nested module -> deleted implicit *)
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:[AssertUpdateSource.Local ("a/b.py", "x: int = 1")]
            ~new_sources:[]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Implicit)]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `None)];
       (* Added nested module -> added implicit *)
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:[]
            ~new_sources:[AssertUpdateSource.Local ("a/b.py", "x: int = 1")]
            ~middle_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `None)]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:[`ModuleMetadata (!&"a", dependency, `Implicit)];
     ]
 
@@ -799,7 +801,7 @@ let test_get_class_summary =
           x: str
       |})]
            ~middle_queries_with_expectations:[`Get ("test.Foo", dependency, Some 1)]
-           ~expected_triggers:[dependency];
+           ~expected_triggers_except_uge_and_fde:[dependency];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:
@@ -813,7 +815,7 @@ let test_get_class_summary =
           x: str
       |})]
            ~middle_queries_with_expectations:[`Get ("test.Missing", dependency, None)]
-           ~expected_triggers:[dependency];
+           ~expected_triggers_except_uge_and_fde:[dependency];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:
@@ -834,7 +836,7 @@ let test_get_class_summary =
                  );
              ]
            ~middle_queries_with_expectations:[`Get ("test.Foo", dependency, Some 1)]
-           ~expected_triggers:[dependency];
+           ~expected_triggers_except_uge_and_fde:[dependency];
       (* Last class definition wins *)
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
@@ -864,7 +866,7 @@ let test_get_class_summary =
                  );
              ]
            ~middle_queries_with_expectations:[`Get ("test.Foo", dependency, Some 2)]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:[`Get ("test.Foo", dependency, Some 1)];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
@@ -891,7 +893,7 @@ let test_get_class_summary =
                  );
              ]
            ~middle_queries_with_expectations:[`Get ("test.Foo", dependency, Some 1)]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:[`Get ("test.Foo", dependency, Some 1)];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
@@ -918,7 +920,7 @@ let test_get_class_summary =
                  );
              ]
            ~middle_queries_with_expectations:[`Get ("test.Foo", dependency, Some 1)]
-           ~expected_triggers:[]
+           ~expected_triggers_except_uge_and_fde:[]
            ~post_queries_with_expectations:[`Get ("test.Foo", dependency, Some 1)];
     ]
 
@@ -937,7 +939,7 @@ let test_class_exists_and_all_classes =
         class Foo:
           x: int
       |})]
-           ~expected_triggers:[dependency];
+           ~expected_triggers_except_uge_and_fde:[dependency];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:
@@ -947,7 +949,7 @@ let test_class_exists_and_all_classes =
       |})]
            ~middle_queries_with_expectations:[`Mem ("test.Foo", dependency, true)]
            ~new_sources:[]
-           ~expected_triggers:[dependency];
+           ~expected_triggers_except_uge_and_fde:[dependency];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:
@@ -961,7 +963,7 @@ let test_class_exists_and_all_classes =
           x: int
       |})]
            ~middle_queries_with_expectations:[`Mem ("test.Foo", dependency, true)]
-           ~expected_triggers:[];
+           ~expected_triggers_except_uge_and_fde:[];
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
            ~original_sources:
@@ -975,7 +977,7 @@ let test_class_exists_and_all_classes =
           x: str
       |})]
            ~middle_queries_with_expectations:[`Mem ("test.Foo", dependency, true)]
-           ~expected_triggers:[dependency];
+           ~expected_triggers_except_uge_and_fde:[dependency];
       (* all_classes *)
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
@@ -997,7 +999,7 @@ let test_class_exists_and_all_classes =
         x: str
     |})]
            ~middle_queries_with_expectations:[`AllClasses ["test.Bar"; "test.Foo"]]
-           ~expected_triggers:[]
+           ~expected_triggers_except_uge_and_fde:[]
            ~post_queries_with_expectations:[`AllClasses ["test.Foo"]];
     ]
 
@@ -1037,7 +1039,7 @@ let test_get_unannotated_global =
                           target_location = Location.WithModule.any;
                         }) );
              ]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:
              [
                `Global
@@ -1087,7 +1089,7 @@ let test_get_unannotated_global =
                         (Module.UnannotatedGlobal.ImportModule
                            { target = !&"target.member"; implicit_alias = false })) );
              ]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:
              [`Global (Reference.create "test.alias", dependency, None)];
       labeled_test_case __FUNCTION__ __LINE__
@@ -1125,7 +1127,7 @@ let test_get_unannotated_global =
                  );
              ]
              (* Location insensitive *)
-           ~expected_triggers:[]
+           ~expected_triggers_except_uge_and_fde:[]
            ~post_queries_with_expectations:
              [
                `Global
@@ -1162,7 +1164,7 @@ let test_get_unannotated_global =
                            { from = !&"target"; target = "member"; implicit_alias = true })) );
              ]
            ~new_sources:[]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:
              [`Global (Reference.create "test.member", dependency, None)];
       (* Adding a source should trigger lookup dependencies *)
@@ -1175,7 +1177,7 @@ let test_get_unannotated_global =
       |})]
            ~middle_queries_with_expectations:
              [`Global (Reference.create "test.member", dependency, None)]
-           ~expected_triggers:[dependency]
+           ~expected_triggers_except_uge_and_fde:[dependency]
            ~post_queries_with_expectations:
              [
                `Global
@@ -1195,7 +1197,7 @@ let test_get_unannotated_global =
     |})]
            ~middle_queries_with_expectations:[`Global (Reference.create "test.*", dependency, None)]
            ~new_sources:[]
-           ~expected_triggers:[dependency];
+           ~expected_triggers_except_uge_and_fde:[dependency];
       (let open Expression in
       let tuple_expression =
         node
@@ -1251,7 +1253,7 @@ let test_get_unannotated_global =
                         }) );
              ]
            ~new_sources:[]
-           ~expected_triggers:[dependency]);
+           ~expected_triggers_except_uge_and_fde:[dependency]);
       (* First global wins. *)
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
@@ -1283,7 +1285,7 @@ let test_get_unannotated_global =
                           target_location = Location.WithModule.any;
                         }) );
              ]
-           ~expected_triggers:[];
+           ~expected_triggers_except_uge_and_fde:[];
       (* Only recurse into ifs *)
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
@@ -1329,7 +1331,7 @@ let test_get_unannotated_global =
                           target_location = Location.WithModule.any;
                         }) );
              ]
-           ~expected_triggers:[];
+           ~expected_triggers_except_uge_and_fde:[];
       (* get defines *)
       (let open Expression in
       let open Statement in
@@ -1399,7 +1401,7 @@ let test_get_unannotated_global =
                                   (Expression.Constant Constant.NoneLiteral)));
                         ]) );
              ]
-           ~expected_triggers:[]
+           ~expected_triggers_except_uge_and_fde:[]
            ~post_queries_with_expectations:
              [
                `Global
@@ -1446,7 +1448,7 @@ let test_dependencies_and_new_values =
                `Get ("test.Foo", alias_dependency, Some 1);
                `Get ("test.Foo", type_check_dependency, Some 1);
              ]
-           ~expected_triggers:[alias_dependency; type_check_dependency];
+           ~expected_triggers_except_uge_and_fde:[alias_dependency; type_check_dependency];
       (* Addition should add values when previously they were missing *)
       labeled_test_case __FUNCTION__ __LINE__
       @@ assert_update
@@ -1457,7 +1459,7 @@ let test_dependencies_and_new_values =
     |})]
            ~middle_queries_with_expectations:
              [`Global (Reference.create "test.x", alias_dependency, None)]
-           ~expected_triggers:[alias_dependency]
+           ~expected_triggers_except_uge_and_fde:[alias_dependency]
            ~post_queries_with_expectations:
              [
                `Global
@@ -1490,7 +1492,7 @@ let test_dependencies_and_new_values =
       |})]
            ~middle_queries_with_expectations:
              [`GetRawSource (Reference.create "test", alias_dependency)]
-           ~expected_triggers:[alias_dependency];
+           ~expected_triggers_except_uge_and_fde:[alias_dependency];
     ]