From a1555d9352f733504f68c92317e7ecc82ee895cb Mon Sep 17 00:00:00 2001 From: Sam Zhou Date: Fri, 29 Mar 2024 13:33:10 -0700 Subject: [PATCH] [flow] More precise dependency analysis in class extends Summary: In the AST, class extends contain an expression, but during type checking, we do not accept any kind of expressions. In fact, we only accept those that are annotation-like, e.g. identifier, members, and casts. For casts, only the anntoation is used to determine the signature, and for all other expressions, the signature will be any. However, our dependency analysis is name_def_ordering is not aware of these rules. We just blindly add everything in the expression as dependency. In the added test, it makes Flow think that there is a legal SCC of ``` Ref --> referencedInClassExtends --> Node -- /\ | |-----------------------------------------\/ ``` which doesn't make sense. It just has not blown up under the current system, due to the use of unresolved tvars in between annotation resolution. This diff fixes that. During the dependency analysis, we will only visit the nodes that are used to determine the signature, mirroring the logic in `mk_extends` in statement.ml. Changelog: [internal] Reviewed By: panagosg7 Differential Revision: D55528626 fbshipit-source-id: 62b7617b596395b133a8d9aea2f5395dc36aa0e8 --- .../env_builder/__tests__/name_def_test.ml | 17 ++++++++++++++++ src/analysis/env_builder/name_def_ordering.ml | 20 ++++++++++++++++++- tests/sealed_tvars/class_ext.js | 18 +++++++++++++---- tests/sealed_tvars/sealed_tvars.exp | 12 +++++------ 4 files changed, 56 insertions(+), 11 deletions(-) diff --git a/src/analysis/env_builder/__tests__/name_def_test.ml b/src/analysis/env_builder/__tests__/name_def_test.ml index e676930a4f5..5d87c51a609 100644 --- a/src/analysis/env_builder/__tests__/name_def_test.ml +++ b/src/analysis/env_builder/__tests__/name_def_test.ml @@ -570,6 +570,23 @@ class D extends C { [%expect {| legal scc: (((2, 6) to (2, 7)); ((5, 6) to (5, 7))) |}] +let%expect_test "class_extends_cast" = + print_order_test {| +type Ref = { children: Array }; + +declare const referencedInClassExtends: Ref; + +declare function f(v: mixed): mixed; + +class Node extends (f(referencedInClassExtends) as any) {} + |}; + [%expect {| + (8, 6) to (8, 10) => + (2, 5) to (2, 8) => + (4, 14) to (4, 38) => + (6, 17) to (6, 18) => + (8, 22) to (8, 46) (Env_api.Make.ExpressionLoc) |}] + let%expect_test "enum" = print_order_test {| function havoced() { diff --git a/src/analysis/env_builder/name_def_ordering.ml b/src/analysis/env_builder/name_def_ordering.ml index db447a12ae4..0562e4820a7 100644 --- a/src/analysis/env_builder/name_def_ordering.ml +++ b/src/analysis/env_builder/name_def_ordering.ml @@ -605,6 +605,24 @@ struct expr ) + method class_extends_sig + ((_, { Ast.Class.Extends.expr; targs; comments = _ }) : ('loc, 'loc) Ast.Class.Extends.t) + : unit = + Base.Option.iter targs ~f:(fun targs -> ignore @@ this#type_args targs); + (* We only visit expressions that is used to generate the class signature. *) + let rec visit_expr_for_sig ((_, expr') as expr) = + let open Ast.Expression in + match expr' with + | Identifier _ -> ignore @@ this#expression expr + | Member { Member._object; property = Member.PropertyIdentifier _; comments = _ } -> + visit_expr_for_sig _object + | AsExpression { AsExpression.expression = _; annot; comments = _ } + | TypeCast { TypeCast.expression = _; annot; comments = _ } -> + ignore @@ this#type_annotation annot + | _ -> () + in + visit_expr_for_sig expr + method class_body_annotated (cls_body : ('loc, 'loc) Ast.Class.Body.t) = let open Ast.Class.Body in let (_, { body; comments = _ }) = cls_body in @@ -878,7 +896,7 @@ struct (fun visitor -> let open Flow_ast_mapper in let _ = visitor#class_body_annotated body in - let _ = map_opt (map_loc visitor#class_extends) extends in + Base.Option.iter ~f:visitor#class_extends_sig extends; let _ = map_opt visitor#class_implements implements in let _ = map_list visitor#class_decorator class_decorators in let _ = map_opt visitor#type_params tparams in diff --git a/tests/sealed_tvars/class_ext.js b/tests/sealed_tvars/class_ext.js index 7111991782c..c6b0d0368c4 100644 --- a/tests/sealed_tvars/class_ext.js +++ b/tests/sealed_tvars/class_ext.js @@ -1,9 +1,19 @@ //@flow -class C extends (42 as any as D).f(x => 42) {} +{ + class C extends (42 as any as D).f(x => 42) {} -class D { - f(x: mixed): any { - return C; + class D { + f(x: mixed): any { + return C; + } } } + +{ + type Ref = { children: Array }; + declare const referencedInClassExtends: Ref; + declare function f(v: mixed): mixed; + // Node does not depend on f and referencedInClassExtends + class Node extends (f(referencedInClassExtends) as any) {} +} diff --git a/tests/sealed_tvars/sealed_tvars.exp b/tests/sealed_tvars/sealed_tvars.exp index c2eba57253e..4f122a4c279 100644 --- a/tests/sealed_tvars/sealed_tvars.exp +++ b/tests/sealed_tvars/sealed_tvars.exp @@ -14,20 +14,20 @@ Unreachable code. [unreachable-code] ^^^^^^^^^^^^^^ -Error ------------------------------------------------------------------------------------------------ class_ext.js:3:17 +Error ------------------------------------------------------------------------------------------------ class_ext.js:4:19 Cannot use `42.f(...)` [1] as a superclass. Only variables and member expressions may be extended [invalid-extends] - 3| class C extends (42 as any as D).f(x => 42) {} - ^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1] + 4| class C extends (42 as any as D).f(x => 42) {} + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1] -Error ------------------------------------------------------------------------------------------------ class_ext.js:3:36 +Error ------------------------------------------------------------------------------------------------ class_ext.js:4:38 An annotation on `x` is required because Flow cannot infer its type from local context. [missing-local-annot] - 3| class C extends (42 as any as D).f(x => 42) {} - ^ + 4| class C extends (42 as any as D).f(x => 42) {} + ^ Error ---------------------------------------------------------------------------------------------------- enums.js:2:17