From aa73d7001383c682e4c48b71d32f1ed762665082 Mon Sep 17 00:00:00 2001 From: Steven Troxler Date: Mon, 22 Jul 2024 15:43:11 -0700 Subject: [PATCH] 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 | 17 +++- source/analysis/test/astEnvironmentTest.ml | 6 +- .../test/unannotatedGlobalEnvironmentTest.ml | 86 ++++++++++--------- 3 files changed, 65 insertions(+), 44 deletions(-) diff --git a/source/analysis/astEnvironment.ml b/source/analysis/astEnvironment.ml index e6dbfa4cf1f..ab25d7f10ac 100644 --- a/source/analysis/astEnvironment.ml +++ b/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, diff --git a/source/analysis/test/astEnvironmentTest.ml b/source/analysis/test/astEnvironmentTest.ml index e3e7f555a34..4c7f302cd29 100644 --- a/source/analysis/test/astEnvironmentTest.ml +++ b/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 diff --git a/source/analysis/test/unannotatedGlobalEnvironmentTest.ml b/source/analysis/test/unannotatedGlobalEnvironmentTest.ml index 90b09c954f0..4c9392ec62e 100644 --- a/source/analysis/test/unannotatedGlobalEnvironmentTest.ml +++ b/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,14 +696,14 @@ 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__ @@ -709,14 +711,14 @@ 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", "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__ @@ -724,14 +726,14 @@ 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:[] - ~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__ @@ -739,14 +741,14 @@ let test_get_explicit_module_metadata = ~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,7 +763,7 @@ 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__ @@ -769,7 +771,7 @@ let test_get_implicit_module_metadata = ~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__ @@ -777,7 +779,7 @@ let test_get_implicit_module_metadata = ~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]; ]