Skip to content

Commit

Permalink
[flow] More precise dependency analysis in class extends
Browse files Browse the repository at this point in the history
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
  • Loading branch information
SamChou19815 authored and facebook-github-bot committed Mar 29, 2024
1 parent b2a1884 commit a1555d9
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 11 deletions.
17 changes: 17 additions & 0 deletions src/analysis/env_builder/__tests__/name_def_test.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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<Node> };

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() {
Expand Down
20 changes: 19 additions & 1 deletion src/analysis/env_builder/name_def_ordering.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
18 changes: 14 additions & 4 deletions tests/sealed_tvars/class_ext.js
Original file line number Diff line number Diff line change
@@ -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<Node> };
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) {}
}
12 changes: 6 additions & 6 deletions tests/sealed_tvars/sealed_tvars.exp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit a1555d9

Please sign in to comment.