Skip to content

Commit

Permalink
Back out "[flow][get-def] Use the same heuristic for get-def in requi…
Browse files Browse the repository at this point in the history
…re expression"

Summary:
This breaks some find references test, because the change has more blast radius than I thought.

Previously, I think it will only affect the get-def results in the following parts of a require:

```
const Foo = require('bar');
//          ^^^^^^^^^^^^^^
```

However, it will affect `Foo` as well, since get-def on `Foo` will jump to the `require(...)`, which then jumps to the updated location with the heuristics added in D67875259. Since it now jumps to an identifier, it now affects find-ref results, and broke some tests (see github ci).

I couldn't find an easy way to fix forward, so I'm reverting the diff for now.

Changelog: [internal]

Reviewed By: panagosg7

Differential Revision: D67913190

fbshipit-source-id: c12fb75eee091f2a2afc08e35dcd4c098a04de6d
  • Loading branch information
SamChou19815 authored and facebook-github-bot committed Jan 7, 2025
1 parent ddcde52 commit 5052a16
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 46 deletions.
10 changes: 2 additions & 8 deletions src/services/get_def/get_def_process_location.ml
Original file line number Diff line number Diff line change
Expand Up @@ -463,18 +463,12 @@ class virtual ['T] searcher _cx ~is_local_use ~is_legit_require ~covers_target ~
Call.callee = (_, Identifier (_, { Identifier.name = "require"; _ }));
arguments =
( _,
{
ArgList.arguments =
[Expression (source_annot, (StringLiteral _ | TemplateLiteral _))];
comments = _;
}
{ ArgList.arguments = [Expression (source_annot, StringLiteral _)]; comments = _ }
);
_;
}
when this#is_legit_require source_annot ->
let annot =
(this#loc_of_annot source_annot, this#type_from_enclosing_node source_annot)
in
let annot = (this#loc_of_annot annot, this#type_from_enclosing_node annot) in
this#request (Get_def_request.Type { annot; name = None })
| _ -> super#expression (annot, expr)
else
Expand Down
32 changes: 17 additions & 15 deletions src/typing/statement.ml
Original file line number Diff line number Diff line change
Expand Up @@ -3655,7 +3655,21 @@ module Make
comments;
}
)
)
) ->
let (_def_loc_opt, t) =
Import_export.get_module_t
cx
(source_loc, module_name)
~perform_platform_validation:true
~import_kind_for_untyped_import_validation:(Some ImportValue)
|> Import_export.cjs_require_type
cx
(mk_reason (RModule module_name) loc)
~namespace_symbol:(mk_module_symbol ~name:module_name ~def_loc:loc)
~standard_cjs_esm_interop:false
~legacy_interop:false
in
(t, (args_loc, { ArgList.arguments = [Expression (expression cx lit_exp)]; comments }))
| ( None,
( args_loc,
{
Expand Down Expand Up @@ -3685,7 +3699,7 @@ module Make
}
)
) ->
let (def_loc_opt, require_t) =
let (_def_loc_opt, t) =
Import_export.get_module_t
cx
(source_loc, module_name)
Expand All @@ -3698,19 +3712,7 @@ module Make
~standard_cjs_esm_interop:false
~legacy_interop:false
in
let lit_exp =
let ((l, t), e) = expression cx lit_exp in
let t =
TypeUtil.mod_reason_of_t
(fun _ ->
match def_loc_opt with
| Some def_loc -> mk_reason (RModule module_name) def_loc |> repos_reason loc
| None -> TypeUtil.reason_of_t require_t |> repos_reason loc)
t
in
((l, t), e)
in
(require_t, (args_loc, { ArgList.arguments = [Expression lit_exp]; comments }))
(t, (args_loc, { ArgList.arguments = [Expression (expression cx lit_exp)]; comments }))
| (Some _, arguments) ->
ignore (arg_list cx arguments);
Flow.add_output
Expand Down
36 changes: 16 additions & 20 deletions tests/get_def/get_def.exp
Original file line number Diff line number Diff line change
Expand Up @@ -224,71 +224,67 @@ require.js:12:10
Flags:
[LIB] libs/lib.js:18:27,18:32

require.js:15:11
require.js:16:9
Flags:
[LIB] libs/lib.js:2:22,2:24

require.js:19:9
require.js:18:9
Flags:
[LIB] libs/lib.js:2:22,2:24

require.js:21:9
require.js:20:14
Flags:
[LIB] libs/lib.js:2:22,2:24

require.js:23:14
Flags:
[LIB] libs/lib.js:2:22,2:24

require.js:25:9
require.js:22:9
Flags:
[LIB] libs/lib.js:3:22,3:24

require.js:27:9
require.js:24:9
Flags:
[LIB] libs/lib.js:3:22,3:24

require.js:29:14
require.js:26:14
Flags:
[LIB] libs/lib.js:3:22,3:24

require.js:31:15
require.js:28:15
Flags:
[LIB] libs/lib.js:3:28,3:30

require.js:33:20
require.js:30:20
Flags:
[LIB] libs/lib.js:3:28,3:30

require.js:38:9
require.js:35:9
Flags:
[LIB] libs/lib.js:8:5,8:7

require.js:40:9
require.js:37:9
Flags:
[LIB] libs/lib.js:8:5,8:7

require.js:42:14
require.js:39:14
Flags:
[LIB] libs/lib.js:8:5,8:7

require.js:44:9
require.js:41:9
Flags:
[LIB] libs/lib.js:9:5,9:7

require.js:46:9
require.js:43:9
Flags:
[LIB] libs/lib.js:9:5,9:7

require.js:48:14
require.js:45:14
Flags:
[LIB] libs/lib.js:9:5,9:7

require.js:50:15
require.js:47:15
Flags:
[LIB] libs/lib.js:9:11,9:13

require.js:52:20
require.js:49:20
Flags:
[LIB] libs/lib.js:9:11,9:13

Expand Down
3 changes: 0 additions & 3 deletions tests/get_def/require.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ require("Untyped");
require("Untyped_LibDeclared");
// ^

require('test_lib');
// ^

{
const {foo} = require('test_lib');
// ^
Expand Down

0 comments on commit 5052a16

Please sign in to comment.