diff --git a/CHANGELOG.md b/CHANGELOG.md index c8924a442d..ea0a69780c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,8 @@ - Rewatch: enable `--create-sourcedirs` by default (now deprecated when explicitly used). https://github.com/rescript-lang/rescript/pull/8092 - Rewatch: check if filename case for interface and implementation matches. https://github.com/rescript-lang/rescript/pull/8144 +- Migration tool: Do not rewrite or format files unless there are actual migrations performed. https://github.com/rescript-lang/rescript/pull/8157 +- Migration tool: Do not attempt to run migrations on dependencies. https://github.com/rescript-lang/rescript/pull/8157 #### :house: Internal diff --git a/tests/dependencies/deprecatedlib/package.json b/tests/dependencies/deprecatedlib/package.json new file mode 100644 index 0000000000..4a95698ff7 --- /dev/null +++ b/tests/dependencies/deprecatedlib/package.json @@ -0,0 +1,11 @@ +{ + "name": "deprecatedlib", + "version": "0.0.1", + "scripts": { + "build": "rescript-legacy build", + "clean": "rescript clean" + }, + "dependencies": { + "rescript": "workspace:^" + } +} diff --git a/tests/dependencies/deprecatedlib/rescript.json b/tests/dependencies/deprecatedlib/rescript.json new file mode 100644 index 0000000000..b6367f07e2 --- /dev/null +++ b/tests/dependencies/deprecatedlib/rescript.json @@ -0,0 +1,6 @@ +{ + "name": "deprecatedlib", + "sources": { "dir": "src", "subdirs": true }, + "package-specs": { "module": "commonjs", "in-source": true }, + "suffix": ".res.js" +} diff --git a/tests/dependencies/deprecatedlib/src/DeprecatedDep.res b/tests/dependencies/deprecatedlib/src/DeprecatedDep.res new file mode 100644 index 0000000000..26733f0f0c --- /dev/null +++ b/tests/dependencies/deprecatedlib/src/DeprecatedDep.res @@ -0,0 +1,7 @@ +@deprecated({ + reason: "Use `newThing` instead.", + migrate: DeprecatedDep.newThing(), +}) +let oldThing = () => 1 + +let newThing = () => 2 diff --git a/tests/tools_tests/package.json b/tests/tools_tests/package.json index 46209fd6de..57abb22bbb 100644 --- a/tests/tools_tests/package.json +++ b/tests/tools_tests/package.json @@ -8,6 +8,7 @@ }, "dependencies": { "@rescript/react": "link:../dependencies/rescript-react", + "deprecatedlib": "link:../dependencies/deprecatedlib", "rescript": "workspace:^" } } diff --git a/tests/tools_tests/rescript.json b/tests/tools_tests/rescript.json index 3ed41da988..75312e0b22 100644 --- a/tests/tools_tests/rescript.json +++ b/tests/tools_tests/rescript.json @@ -9,5 +9,5 @@ "in-source": true }, "suffix": ".res.js", - "dependencies": ["@rescript/react"] + "dependencies": ["@rescript/react", "deprecatedlib"] } diff --git a/tests/tools_tests/src/expected/DependencyDeprecation.res.expected b/tests/tools_tests/src/expected/DependencyDeprecation.res.expected new file mode 100644 index 0000000000..acaf9fdc88 --- /dev/null +++ b/tests/tools_tests/src/expected/DependencyDeprecation.res.expected @@ -0,0 +1,2 @@ +let value = DeprecatedDep.oldThing() + diff --git a/tests/tools_tests/src/expected/NoOp.res.expected b/tests/tools_tests/src/expected/NoOp.res.expected new file mode 100644 index 0000000000..01895b7ba3 --- /dev/null +++ b/tests/tools_tests/src/expected/NoOp.res.expected @@ -0,0 +1,2 @@ +let meaningOfLife = 42 + diff --git a/tests/tools_tests/src/migrate/DependencyDeprecation.res b/tests/tools_tests/src/migrate/DependencyDeprecation.res new file mode 100644 index 0000000000..6d8716c8be --- /dev/null +++ b/tests/tools_tests/src/migrate/DependencyDeprecation.res @@ -0,0 +1 @@ +let value = DeprecatedDep.oldThing() diff --git a/tests/tools_tests/src/migrate/NoOp.res b/tests/tools_tests/src/migrate/NoOp.res new file mode 100644 index 0000000000..50802237b0 --- /dev/null +++ b/tests/tools_tests/src/migrate/NoOp.res @@ -0,0 +1 @@ +let meaningOfLife = 42 diff --git a/tests/tools_tests/test.sh b/tests/tools_tests/test.sh index 4e44f42170..c91e22e5d0 100755 --- a/tests/tools_tests/test.sh +++ b/tests/tools_tests/test.sh @@ -36,7 +36,8 @@ done # Test migrate command for file in src/migrate/*.{res,resi}; do output="src/expected/$(basename $file).expected" - ../../_build/install/default/bin/rescript-tools migrate "$file" --stdout > $output + # Capture stderr too so warnings would surface in expected output if they occur. + ../../_build/install/default/bin/rescript-tools migrate "$file" --stdout > $output 2>&1 if [ "$RUNNER_OS" == "Windows" ]; then perl -pi -e 's/\r\n/\n/g' -- $output fi diff --git a/tools/bin/main.ml b/tools/bin/main.ml index cd810b5309..41f2148df2 100644 --- a/tools/bin/main.ml +++ b/tools/bin/main.ml @@ -71,12 +71,14 @@ let main () = | "migrate" :: file :: opts -> ( let isStdout = List.mem "--stdout" opts in let outputMode = if isStdout then `Stdout else `File in - match - (Tools.Migrate.migrate ~entryPointFile:file ~outputMode, outputMode) - with - | Ok content, `Stdout -> print_endline content - | result, `File -> logAndExit result - | Error e, _ -> logAndExit (Error e)) + let base = Filename.basename file in + match Tools.Migrate.migrate ~outputMode file with + | Ok (`Changed content | `Unchanged content) when isStdout -> + print_endline content + | Ok (`Changed _) -> logAndExit (Ok (base ^ ": File migrated successfully")) + | Ok (`Unchanged _) -> + logAndExit (Ok (base ^ ": File did not need migration")) + | Error e -> logAndExit (Error e)) | "migrate-all" :: root :: _opts -> ( let rootPath = if Filename.is_relative root then Unix.realpath root else root @@ -106,24 +108,28 @@ let main () = let total = List.length files in if total = 0 then logAndExit (Ok "No source files found to migrate") else + let canonical p = try Unix.realpath p with _ -> p in + let dependency_paths = + Analysis.SharedTypes.FileSet.fold + (fun p acc -> + acc + |> Analysis.SharedTypes.FileSet.add p + |> Analysis.SharedTypes.FileSet.add (canonical p)) + package.dependenciesFiles Analysis.SharedTypes.FileSet.empty + in let process_one file = - (file, Tools.Migrate.migrate ~entryPointFile:file ~outputMode:`File) + ( file, + Tools.Migrate.migrate ~package ~dependency_paths ~outputMode:`File + file ) in let results = List.map process_one files in let migrated, unchanged, failures = results |> List.fold_left - (fun (migrated, unchanged, failures) (file, res) -> + (fun (migrated, unchanged, failures) (_file, res) -> match res with - | Ok msg -> - let base = Filename.basename file in - if msg = base ^ ": File migrated successfully" then - (migrated + 1, unchanged, failures) - else if msg = base ^ ": File did not need migration" then - (migrated, unchanged + 1, failures) - else - (* Unknown OK message, count as unchanged *) - (migrated, unchanged + 1, failures) + | Ok (`Changed _) -> (migrated + 1, unchanged, failures) + | Ok (`Unchanged _) -> (migrated, unchanged + 1, failures) | Error _ -> (migrated, unchanged, failures + 1)) (0, 0, 0) in diff --git a/tools/src/migrate.ml b/tools/src/migrate.ml index b6251dd246..8d49b89184 100644 --- a/tools/src/migrate.ml +++ b/tools/src/migrate.ml @@ -3,6 +3,44 @@ open Analysis module StringMap = Map.Make (String) module StringSet = Set.Make (String) module IntSet = Set.Make (Int) +module FileSet = SharedTypes.FileSet + +let filter_deprecations_for_project ?dependency_paths ?package ~deprecated_used + entryPointFile = + let canonical p = try Unix.realpath p with _ -> p in + let package = + match package with + | Some p -> Some p + | None -> + let uri = Uri.fromPath entryPointFile in + Packages.getPackage ~uri + in + match package with + | None -> deprecated_used + | Some package -> + let dependency_paths = + match dependency_paths with + | Some paths -> paths + | None -> + FileSet.fold + (fun p acc -> acc |> FileSet.add p |> FileSet.add (canonical p)) + package.dependenciesFiles FileSet.empty + in + let is_dependency_path p = + FileSet.mem p dependency_paths + || FileSet.mem (canonical p) dependency_paths + in + let def_loc_path (d : Cmt_utils.deprecated_used) = + let loc_of_expr e = e.Parsetree.pexp_loc.Location.loc_start.pos_fname in + match d.migration_template with + | Some e -> Some (loc_of_expr e) + | None -> Option.map loc_of_expr d.migration_in_pipe_chain_template + in + deprecated_used + |> List.filter (fun (d : Cmt_utils.deprecated_used) -> + match def_loc_path d with + | Some def_path when is_dependency_path def_path -> false + | _ -> true) (* Public API: migrate ~entryPointFile ~outputMode *) @@ -736,7 +774,7 @@ let makeMapper (deprecated_used : Cmt_utils.deprecated_used list) = in mapper -let migrate ~entryPointFile ~outputMode = +let migrate ?dependency_paths ?package ~outputMode entryPointFile = let path = match Filename.is_relative entryPointFile with | true -> Unix.realpath entryPointFile @@ -744,10 +782,6 @@ let migrate ~entryPointFile ~outputMode = in let result = if Filename.check_suffix path ".res" then - let parser = - Res_driver.parsing_engine.parse_implementation ~for_printer:true - in - let {Res_driver.parsetree; comments; source} = parser ~filename:path in match Cmt.loadCmtInfosFromPath ~path with | None -> Error @@ -756,31 +790,43 @@ let migrate ~entryPointFile ~outputMode = could not be found. try to build the project" path) | Some {cmt_extra_info = {deprecated_used}} -> - let mapper = makeMapper deprecated_used in - let astMapped = mapper.structure mapper parsetree in - (* Second pass: apply any post-migration transforms signaled via @apply.transforms *) - let apply_transforms = - let expr mapper (e : Parsetree.expression) = - let e = Ast_mapper.default_mapper.expr mapper e in - MapperUtils.ApplyTransforms.apply_on_self e - in - {Ast_mapper.default_mapper with expr} + let deprecated_used = + filter_deprecations_for_project ~deprecated_used ?package + ?dependency_paths path in - let astTransformed = - apply_transforms.structure apply_transforms astMapped - in - Ok - ( Res_printer.print_implementation - ~width:Res_printer.default_print_width astTransformed ~comments, - source ) + if Ext_list.is_empty deprecated_used then + match outputMode with + | `Stdout -> + let source = Res_io.read_file ~filename:path in + Ok (`Unchanged source) + | `File -> Ok (`Unchanged "") + else + let parser = + Res_driver.parsing_engine.parse_implementation ~for_printer:true + in + let {Res_driver.parsetree; comments; source} = + parser ~filename:path + in + let mapper = makeMapper deprecated_used in + let astMapped = mapper.structure mapper parsetree in + (* Second pass: apply any post-migration transforms signaled via @apply.transforms *) + let apply_transforms = + let expr mapper (e : Parsetree.expression) = + let e = Ast_mapper.default_mapper.expr mapper e in + MapperUtils.ApplyTransforms.apply_on_self e + in + {Ast_mapper.default_mapper with expr} + in + let astTransformed = + apply_transforms.structure apply_transforms astMapped + in + let contents = + Res_printer.print_implementation + ~width:Res_printer.default_print_width astTransformed ~comments + in + if contents = source then Ok (`Unchanged source) + else Ok (`Changed contents) else if Filename.check_suffix path ".resi" then - let parser = - Res_driver.parsing_engine.parse_interface ~for_printer:true - in - let {Res_driver.parsetree = signature; comments; source} = - parser ~filename:path - in - match Cmt.loadCmtInfosFromPath ~path with | None -> Error @@ -789,9 +835,29 @@ let migrate ~entryPointFile ~outputMode = could not be found. try to build the project" path) | Some {cmt_extra_info = {deprecated_used}} -> - let mapper = makeMapper deprecated_used in - let astMapped = mapper.signature mapper signature in - Ok (Res_printer.print_interface astMapped ~comments, source) + let deprecated_used = + filter_deprecations_for_project ~deprecated_used ?package + ?dependency_paths path + in + if Ext_list.is_empty deprecated_used then + match outputMode with + | `Stdout -> + let source = Res_io.read_file ~filename:path in + Ok (`Unchanged source) + | `File -> Ok (`Unchanged "") + else + let parser = + Res_driver.parsing_engine.parse_interface ~for_printer:true + in + let {Res_driver.parsetree = signature; comments; source} = + parser ~filename:path + in + + let mapper = makeMapper deprecated_used in + let astMapped = mapper.signature mapper signature in + let contents = Res_printer.print_interface astMapped ~comments in + if contents = source then Ok (`Unchanged source) + else Ok (`Changed contents) else Error (Printf.sprintf @@ -800,15 +866,17 @@ let migrate ~entryPointFile ~outputMode = in match result with | Error e -> Error e - | Ok (contents, source) when contents <> source -> ( + | Ok (`Unchanged source) -> ( match outputMode with - | `Stdout -> Ok contents + | `Stdout -> Ok (`Unchanged source) + | `File -> + Ok (`Unchanged (Filename.basename path ^ ": File did not need migration")) + ) + | Ok (`Changed contents) -> ( + match outputMode with + | `Stdout -> Ok (`Changed contents) | `File -> let oc = open_out path in Printf.fprintf oc "%s" contents; close_out oc; - Ok (Filename.basename path ^ ": File migrated successfully")) - | Ok (contents, _) -> ( - match outputMode with - | `Stdout -> Ok contents - | `File -> Ok (Filename.basename path ^ ": File did not need migration")) + Ok (`Changed (Filename.basename path ^ ": File migrated successfully"))) diff --git a/tools/src/migrate.mli b/tools/src/migrate.mli index 1831a90b30..1ff89c244c 100644 --- a/tools/src/migrate.mli +++ b/tools/src/migrate.mli @@ -1,4 +1,13 @@ val migrate : - entryPointFile:string -> + ?dependency_paths:Analysis.SharedTypes.FileSet.t -> + ?package:Analysis.SharedTypes.package -> outputMode:[`File | `Stdout] -> - (string, string) result + string -> + ([`Changed of string | `Unchanged of string], string) result + +val filter_deprecations_for_project : + ?dependency_paths:Analysis.SharedTypes.FileSet.t -> + ?package:Analysis.SharedTypes.package -> + deprecated_used:Cmt_utils.deprecated_used list -> + string -> + Cmt_utils.deprecated_used list diff --git a/yarn.lock b/yarn.lock index 2624cbc989..30123bce08 100644 --- a/yarn.lock +++ b/yarn.lock @@ -783,6 +783,7 @@ __metadata: resolution: "@tests/tools@workspace:tests/tools_tests" dependencies: "@rescript/react": "link:../dependencies/rescript-react" + deprecatedlib: "link:../dependencies/deprecatedlib" rescript: "workspace:^" languageName: unknown linkType: soft @@ -1213,6 +1214,20 @@ __metadata: languageName: node linkType: hard +"deprecatedlib@link:../dependencies/deprecatedlib::locator=%40tests%2Ftools%40workspace%3Atests%2Ftools_tests": + version: 0.0.0-use.local + resolution: "deprecatedlib@link:../dependencies/deprecatedlib::locator=%40tests%2Ftools%40workspace%3Atests%2Ftools_tests" + languageName: node + linkType: soft + +"deprecatedlib@workspace:tests/dependencies/deprecatedlib": + version: 0.0.0-use.local + resolution: "deprecatedlib@workspace:tests/dependencies/deprecatedlib" + dependencies: + rescript: "workspace:^" + languageName: unknown + linkType: soft + "diff@npm:^7.0.0": version: 7.0.0 resolution: "diff@npm:7.0.0"