Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PR] Implement versioned .js.flow #8135

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions hack/utils/string/string_utils.ml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ let string_ends_with long short =
`haystack`. If not found, returns -1.

An implementation of the Knuth-Morris-Pratt (KMP) algorithm. *)
let substring_index needle =
let substring_index ?(start_pos = 0) needle =
(* see Wikipedia pseudocode *)
let needle_len = String.length needle in
if needle_len = 0 then raise (Invalid_argument needle);
Expand All @@ -55,7 +55,7 @@ let substring_index needle =
done;
fun haystack ->
let len = String.length haystack in
let p = ref 0 in
let p = ref start_pos in
let q = ref 0 in
while !p < len && !q < needle_len do
if haystack.[!p] = needle.[!q] then (
Expand Down
2 changes: 1 addition & 1 deletion hack/utils/string/string_utils.mli
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ val string_starts_with : string -> string -> bool

val string_ends_with : string -> string -> bool

val substring_index : string -> string -> int
val substring_index : ?start_pos:int -> string -> string -> int

val is_substring : string -> string -> bool

Expand Down
65 changes: 64 additions & 1 deletion src/parser_utils/package_json.ml
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,29 @@

module Ast = Flow_ast

type flow_t = { types_versions: string SMap.t }

type t = {
name: string option;
main: string option;
flow: flow_t option;
}

type 'a t_or_error = (t, 'a * string) result

let ( >>= ) = Core_result.( >>= )

let empty = { name = None; main = None }
let empty = { name = None; main = None; flow = None }

let name package = package.name

let main package = package.main

let flow_types_versions package =
match package.flow with
| Some t -> t.types_versions
| None -> SMap.empty

let statement_of_program = function
| (_, [statement], _) -> Ok statement
| (loc, _, _) -> Error (loc, "Expected a single statement.")
Expand All @@ -45,6 +53,46 @@ let properties_of_object = function
| (_, Ast.Expression.Object { Ast.Expression.Object.properties; comments = _ }) -> Ok properties
| (loc, _) -> Error (loc, "Expected an object literal")

let parse_flow properties =
Ast.(
Expression.Object.(
let extract_property (flow : flow_t) = function
| Property
( _,
Property.Init
{
key = Property.Literal (_, { Literal.value = Literal.String key; _ });
value = (_, Expression.Object { Expression.Object.properties; _ });
_;
} ) ->
begin
match key with
| "typesVersions" ->
let value =
List.fold_left
(fun map property ->
match property with
| Property
( _,
Property.Init
{
key = Property.Literal (_, { Literal.value = Literal.String key; _ });
value =
(_, Expression.Literal { Literal.value = Literal.String value; _ });
_;
} ) ->
SMap.add key value map
| _ -> map)
SMap.empty
properties
in
{ types_versions = value }
| _ -> flow
end
| _ -> flow
in
List.fold_left extract_property { types_versions = SMap.empty } properties))

let parse ast : 'a t_or_error =
statement_of_program ast
>>= object_of_statement
Expand All @@ -67,6 +115,21 @@ let parse ast : 'a t_or_error =
| "main" -> { package with main = Some value }
| _ -> package
end
| Property
( _,
Property.Init
{
key = Property.Literal (_, { Literal.value = Literal.String key; _ });
value = (_, Expression.Object { Expression.Object.properties; _ });
_;
} ) ->
begin
match key with
| "flow" ->
let value = parse_flow properties in
{ package with flow = Some value }
| _ -> package
end
| _ -> package
in
Ok (List.fold_left extract_property empty properties)))
2 changes: 2 additions & 0 deletions src/parser_utils/package_json.mli
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,6 @@ val name : t -> string option

val main : t -> string option

val flow_types_versions : t -> string SMap.t

val parse : (Loc.t, Loc.t) Flow_ast.program -> Loc.t t_or_error
76 changes: 76 additions & 0 deletions src/services/inference/module/module_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,69 @@ let module_name_candidates ~options =
in
List.rev (name :: List.fold_left map_name [] mappers))

let types_versions_package_candidates reader =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this function returns a pair of (version, path) but only the path is used by the caller. Could you change it to return only the path?

Module_hashtables.memoize_with_types_versions_candidates_cache ~f:(fun package_filename ->
let package =
match Module_heaps.Reader_dispatcher.get_package ~reader package_filename with
| Some (Ok package) -> package
| _ -> Package_json.empty
goodmind marked this conversation as resolved.
Show resolved Hide resolved
in
let types_versions = Package_json.flow_types_versions package in
let version_path =
SMap.find_first_opt
(fun k ->
try Semver.satisfies ~include_prereleases:true k Flow_version.version
with Semver.Parse_error _ -> false)
types_versions
Comment on lines +105 to +109
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this means is that if there are multiple available typesVersions entries that are satisfied by the current version, we will pick whichever is lowest alphabetically. I think this is a policy which will confuse users. I think we should instead pick the first entry that is satisfied by the current version.

This means that we should represent the types_versions as a list of pairs, in the order that they are written, rather than a Map, but since (a) it's likely to be a small list, and (b) we do an O(n) traversal here anyway, it shouldn't affect performance.

in
version_path)

let types_versions_candidates ~reader dir rel_path dirname =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function and the associated code in node_paths_part.ml really make me nervous. I get worried whenever I see code that manipulates paths directly. There are a lot of opportunities to make mistakes, both for the original author and future maintainers.

I think it would help if I understood the motivation for this code better. What is happening here that couldn't be satisfied by reusing existing code? Fundamentally, it seems to me like we are looking in the package.json already for main to figure out what to resolve the module to, why can't we augment that logic to look at the typesVersions fields as well?

I have some more specific comments about this code, but I'd like to understand the context better before I dig deeper.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to write unit tests for this? I didn't found any runner in repository, but it has some OUnit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have some unit tests, but they are only set up to run with Buck so we haven't open sourced them. @mroch, do you know what would be involved in setting up our unit tests to run with make?

Node_paths_part.(
let parts = get_node_module_path_parts rel_path dirname in
match parts with
| Some parts ->
let package_root =
if parts.package_root_index == -1 then
rel_path
else
String.sub rel_path 0 parts.package_root_index
in
let package_filename = Filename.concat package_root "package.json" in
let package_filename = Files.normalize_path dir package_filename in
let version_path = types_versions_package_candidates reader package_filename in
let filename =
if parts.package_root_index == -1 then
""
else
String.sub
rel_path
(parts.package_root_index + 1)
(String.length rel_path - (parts.package_root_index + 1))
in
let rel_path =
match version_path with
| Some (_, version_path) ->
(* Chop trailing "/" *)
let tail =
if filename = "" then
version_path
else
Filename.concat version_path filename
in
Files.normalize_path package_root tail
| None -> rel_path
in
(* If file points outside of root, resolve current package root *)
let rel_path =
if String_utils.string_starts_with rel_path package_root then
rel_path
else
Files.normalize_path package_root filename
in
Some rel_path
| _ -> None)

let add_package filename = function
| Ok package -> Module_heaps.Package_heap_mutator.add_package_json filename package
| Error _ -> Module_heaps.Package_heap_mutator.add_error filename
Expand Down Expand Up @@ -320,6 +383,19 @@ module Node = struct
let file_options = Options.file_options options in
lazy_seq
[
lazy
( if SSet.mem dir node_modules_containers then
lazy_seq
( Files.node_resolver_dirnames file_options
|> Core_list.map ~f:(fun dirname ->
let rel_path = spf "%s%s%s" dirname Filename.dir_sep r in
let rel_path_opt = types_versions_candidates ~reader dir rel_path dirname in
match rel_path_opt with
| Some rel_path ->
lazy (resolve_relative ~options ~reader loc ?resolution_acc dir rel_path)
| _ -> lazy None) )
else
None );
lazy
( if SSet.mem dir node_modules_containers then
lazy_seq
Expand Down
78 changes: 78 additions & 0 deletions src/services/inference/module/node_paths_part.ml
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
(**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*)

type node_module_path_parts = {
top_level_node_modules_index: int;
top_level_package_name_index: int;
package_root_index: int;
file_name_index: int;
}

type states = {
before_node_modules: int;
node_modules: int;
scope: int;
package_content: int;
}

let states = { before_node_modules = 0; node_modules = 1; scope = 2; package_content = 3 }

(* Example of expected pattern: /base/path/node_modules/[@scope/otherpackage/@otherscope/node_modules/]package/[subdirectory/]file.js *)
(* Returns indices: ^ ^ ^ ^ *)
let get_node_module_path_parts (full_path : string) (node_modules_path_part : string) :
node_module_path_parts option =
let top_level_node_modules_index = ref 0 in
let top_level_package_name_index = ref 0 in
let package_root_index = ref 0 in
let file_name_index = ref 0 in
let part_start = ref 0 in
let part_end = ref 0 in
let state = ref states.before_node_modules in
while !part_end >= 0 do
part_start := !part_end;
part_end :=
String_utils.substring_index ~start_pos:(!part_start + 1) Filename.dir_sep full_path;

if !state == states.before_node_modules then (
let start = !part_start in
let index = String_utils.substring_index ~start_pos:start node_modules_path_part full_path in
if index == start then (
top_level_node_modules_index := start;
top_level_package_name_index := !part_end;
state := states.node_modules
)
) else if !state == states.node_modules || !state == states.scope then
let index = !part_start + 1 in
let ch = full_path.[index] in
let has_scope = ch == '@' in
if !state == states.node_modules && has_scope then
state := states.scope
else (
package_root_index := !part_end;
state := states.package_content
)
else if !state == states.package_content then (
let index =
String_utils.substring_index ~start_pos:!part_start node_modules_path_part full_path
in
if index == !part_start then state := states.node_modules
) else
state := states.package_content
done;

file_name_index := !part_start;

if !state > states.node_modules then
Some
{
top_level_node_modules_index = !top_level_node_modules_index;
top_level_package_name_index = !top_level_package_name_index;
package_root_index = !package_root_index;
file_name_index = !file_name_index;
}
else
None
9 changes: 9 additions & 0 deletions src/state/locals/module/module_hashtables.ml
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,12 @@ let memoize_with_module_name_candidates_cache ~f name =
let result = f name in
Hashtbl.add module_name_candidates_cache name result;
result

let types_versions_candidates_cache = Hashtbl.create 100

let memoize_with_types_versions_candidates_cache ~f name =
try Hashtbl.find types_versions_candidates_cache name
with Not_found ->
let result = f name in
Hashtbl.add types_versions_candidates_cache name result;
result
goodmind marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions src/state/locals/module/module_hashtables.mli
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,6 @@ module All_providers_mutator : sig
end

val memoize_with_module_name_candidates_cache : f:(string -> string list) -> string -> string list

val memoize_with_types_versions_candidates_cache :
f:(string -> (SMap.key * string) option) -> string -> (SMap.key * string) option
7 changes: 7 additions & 0 deletions tests/declaration_files_node_versions/.flowconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[ignore]

[include]

[libs]

[options]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Found 0 errors

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/declaration_files_node_versions/node_modules/foo/ok.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading