From e08d3ddd88d52c2128b46a4427f5e3ea408d2937 Mon Sep 17 00:00:00 2001 From: Danny Yang Date: Tue, 23 Jul 2024 14:35:56 -0700 Subject: [PATCH] plumbing strict overrides flag Summary: This diff plumbs the new flag through to suppress or show the strict overrides error in typechecking. Reviewed By: kinto0 Differential Revision: D60085379 fbshipit-source-id: e8a706541fb154d2948fe69172ad59c79a12ea32 --- source/analysis/analysisError.ml | 9 ++- source/analysis/analysisError.mli | 7 +- source/analysis/postprocessing.ml | 8 ++- source/analysis/test/analysisErrorTest.ml | 20 +++++- .../analysis/test/integration/overrideTest.ml | 65 ++++++++++++++++++- 5 files changed, 100 insertions(+), 9 deletions(-) diff --git a/source/analysis/analysisError.ml b/source/analysis/analysisError.ml index 9261973dba3..e5882e112ac 100644 --- a/source/analysis/analysisError.ml +++ b/source/analysis/analysisError.ml @@ -4063,13 +4063,18 @@ let filter ~resolution errors = List.filter ~f:(fun error -> not (should_filter error)) errors -let suppress ~mode ~ignore_codes error = +let suppress + ~mode + ~ignore_codes + ~type_check_controls:{ EnvironmentControls.TypeCheckControls.include_strict_override_errors; _ } + error + = let suppress_in_strict ({ kind; _ } as error) = if due_to_analysis_limitations error then true else match kind with - | InvalidOverride { decorator = MissingOverride; _ } -> true + | InvalidOverride { decorator = MissingOverride; _ } -> not include_strict_override_errors | IncompleteType _ -> (* TODO(T42467236): Ungate this when ready to codemod upgrade *) true diff --git a/source/analysis/analysisError.mli b/source/analysis/analysisError.mli index fb3d1c40cf9..06843df811e 100644 --- a/source/analysis/analysisError.mli +++ b/source/analysis/analysisError.mli @@ -601,7 +601,12 @@ val deduplicate : t list -> t list val filter : resolution:GlobalResolution.t -> t list -> t list -val suppress : mode:Source.mode -> ignore_codes:int list -> t -> bool +val suppress + : mode:Source.mode -> + ignore_codes:int list -> + type_check_controls:EnvironmentControls.TypeCheckControls.t -> + t -> + bool val dequalify : Reference.t Reference.Map.t -> resolution:GlobalResolution.t -> t -> t diff --git a/source/analysis/postprocessing.ml b/source/analysis/postprocessing.ml index 85455da13d2..b0f5b7457a3 100644 --- a/source/analysis/postprocessing.ml +++ b/source/analysis/postprocessing.ml @@ -117,10 +117,11 @@ let filter_errors ~mode ~global_resolution ~typecheck_flags:{ Source.TypecheckFlags.ignore_codes; _ } + ~type_check_controls errors_by_define = let filter errors = - let keep_error error = not (Error.suppress ~mode ~ignore_codes error) in + let keep_error error = not (Error.suppress ~mode ~ignore_codes ~type_check_controls error) in List.filter ~f:keep_error errors in List.map errors_by_define ~f:(fun errors -> filter errors |> List.sort ~compare:Error.compare) @@ -151,10 +152,11 @@ let run_on_source in let global_resolution = TypeEnvironment.ReadOnly.global_resolution environment in let controls = TypeEnvironment.ReadOnly.controls environment in + let type_check_controls = EnvironmentControls.type_check_controls controls in let mode = Source.mode ~configuration:(EnvironmentControls.configuration controls) ~local_mode in - filter_errors ~mode ~global_resolution ~typecheck_flags errors_by_define + filter_errors ~mode ~global_resolution ~typecheck_flags ~type_check_controls errors_by_define |> add_local_mode_errors ~define:(Source.top_level_define_node source) source - |> suppress_errors_based_on_comments ~controls:(EnvironmentControls.type_check_controls controls) + |> suppress_errors_based_on_comments ~controls:type_check_controls |> List.map ~f:(Error.dequalify (Preprocessing.dequalify_map source) ~resolution:global_resolution) |> List.sort ~compare:Error.compare diff --git a/source/analysis/test/analysisErrorTest.ml b/source/analysis/test/analysisErrorTest.ml index e9feab4e814..99b15ac294d 100644 --- a/source/analysis/test/analysisErrorTest.ml +++ b/source/analysis/test/analysisErrorTest.ml @@ -698,11 +698,27 @@ let test_filter context = let test_suppress _ = + let type_check_controls = + { + EnvironmentControls.TypeCheckControls.include_type_errors = true; + include_local_annotations = true; + include_readonly_errors = true; + include_strict_override_errors = true; + include_unawaited_awaitable_errors = true; + debug = true; + include_suppressed_errors = true; + no_validation_on_class_lookup_failure = true; + } + in let assert_suppressed mode ?(ignore_codes = []) ?(signature = mock_signature) ?location kind = - assert_equal true (Error.suppress ~mode ~ignore_codes (error ~signature ?location kind)) + assert_equal + true + (Error.suppress ~mode ~ignore_codes ~type_check_controls (error ~signature ?location kind)) in let assert_not_suppressed mode ?(ignore_codes = []) ?(signature = mock_signature) kind = - assert_equal false (Error.suppress ~mode ~ignore_codes (error ~signature kind)) + assert_equal + false + (Error.suppress ~mode ~ignore_codes ~type_check_controls (error ~signature kind)) in (* Test different modes. *) assert_not_suppressed Source.Debug (missing_return Type.Top); diff --git a/source/analysis/test/integration/overrideTest.ml b/source/analysis/test/integration/overrideTest.ml index ae9a432dd10..7921be9b422 100644 --- a/source/analysis/test/integration/overrideTest.ml +++ b/source/analysis/test/integration/overrideTest.ml @@ -8,6 +8,69 @@ open OUnit2 open IntegrationTest +let test_suppression = + test_list + [ + (* suppressed in unsafe mode *) + labeled_test_case __FUNCTION__ __LINE__ + @@ assert_default_type_errors + {| + class C: + def f(self) -> None: + pass + + class D(C): + def f(self) -> None: + pass + |} + []; + (* suppressed in strict mode until we codemod *) + labeled_test_case __FUNCTION__ __LINE__ + @@ assert_strict_type_errors + {| + class C: + def f(self) -> None: + pass + + class D(C): + def f(self) -> None: + pass + |} + []; + (* show error in strict mode if flag is set *) + labeled_test_case __FUNCTION__ __LINE__ + @@ assert_strict_type_errors + ~enable_strict_override_check:true + {| + class C: + def f(self) -> None: + pass + + class D(C): + def f(self) -> None: + pass + |} + [ + "Invalid override [40]: `test.D.f` is not decorated with @override, but overrides a \ + method with the same name in superclasses of `D`."; + ]; + (* do not show error in unsafe mode if flag is set *) + labeled_test_case __FUNCTION__ __LINE__ + @@ assert_default_type_errors + ~enable_strict_override_check:true + {| + class C: + def f(self) -> None: + pass + + class D(C): + def f(self) -> None: + pass + |} + []; + ] + + let test_extra_overriding_parameter = test_list [ @@ -316,4 +379,4 @@ let test_extra_overriding_parameter = ] -let () = "override" >::: [test_extra_overriding_parameter] |> Test.run +let () = "override" >::: [test_suppression; test_extra_overriding_parameter] |> Test.run