Migration tool fixes

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
 

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:^"
+  }
+}

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"
+}

tests/dependencies/deprecatedlib/src/DeprecatedDep.res

@@ -0,0 +1,7 @@
+@deprecated({
+  reason: "Use `newThing` instead.",
+  migrate: DeprecatedDep.newThing(),
+})
+let oldThing = () => 1
+
+let newThing = () => 2

tests/tools_tests/package.json

@@ -8,6 +8,7 @@
   },
   "dependencies": {
     "@rescript/react": "link:../dependencies/rescript-react",
+    "deprecatedlib": "link:../dependencies/deprecatedlib",
     "rescript": "workspace:^"
   }
 }

tests/tools_tests/rescript.json

@@ -9,5 +9,5 @@
     "in-source": true
   },
   "suffix": ".res.js",
-  "dependencies": ["@rescript/react"]
+  "dependencies": ["@rescript/react", "deprecatedlib"]
 }

tests/tools_tests/src/expected/DependencyDeprecation.res.expected

@@ -0,0 +1,2 @@
+let value = DeprecatedDep.oldThing()
+

tests/tools_tests/src/expected/NoOp.res.expected

@@ -0,0 +1,2 @@
+let meaningOfLife = 42
+

tests/tools_tests/src/expected/StdlibMigration_Js_Undefined.res.expected

@@ -27,10 +27,10 @@ let toOption2 = Nullable.toOption(Nullable.make(3))
 let to_opt1 = Nullable.make(4)->Nullable.toOption
 let to_opt2 = Nullable.toOption(Nullable.make(4))
 
-let test1 = Js.Undefined.empty->Js.Undefined.test
-let test2 = Js.Undefined.test(Js.Undefined.empty)
-let test3 = Js.Undefined.return(5)->Js.Undefined.bind(v => v)->Js.Undefined.test
+let test1 = Nullable.undefined->Js.Undefined.test
+let test2 = Js.Undefined.test(Nullable.undefined)
+let test3 = Nullable.make(5)->Nullable.map(v => v)->Js.Undefined.test
 
-let testAny1 = Js.Undefined.testAny(Js.Undefined.empty)
-let testAny2 = Js.Undefined.empty->Js.Undefined.testAny
+let testAny1 = Js.Undefined.testAny(Nullable.undefined)
+let testAny2 = Nullable.undefined->Js.Undefined.testAny
 

tests/tools_tests/src/migrate/DependencyDeprecation.res

@@ -0,0 +1 @@
+let value = DeprecatedDep.oldThing()

tests/tools_tests/src/migrate/NoOp.res

@@ -0,0 +1 @@
+let meaningOfLife = 42

tests/tools_tests/src/migrate/migrated/Migrated_StdlibMigration_Js_Undefined.res

@@ -29,9 +29,9 @@ let toOption2 = Nullable.toOption(Nullable.make(3))
 let to_opt1 = Nullable.make(4)->Nullable.toOption
 let to_opt2 = Nullable.toOption(Nullable.make(4))
 
-let test1 = Js.Undefined.empty->Js.Undefined.test
-let test2 = Js.Undefined.test(Js.Undefined.empty)
-let test3 = Js.Undefined.return(5)->Js.Undefined.bind(v => v)->Js.Undefined.test
+let test1 = Nullable.undefined->Js.Undefined.test
+let test2 = Js.Undefined.test(Nullable.undefined)
+let test3 = Nullable.make(5)->Nullable.map(v => v)->Js.Undefined.test
 
-let testAny1 = Js.Undefined.testAny(Js.Undefined.empty)
-let testAny2 = Js.Undefined.empty->Js.Undefined.testAny
+let testAny1 = Js.Undefined.testAny(Nullable.undefined)
+let testAny2 = Nullable.undefined->Js.Undefined.testAny

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

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

tools/src/migrate.ml

@@ -3,6 +3,41 @@ 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
+    deprecated_used
+    |> List.filter (fun (d : Cmt_utils.deprecated_used) ->
+           let loc_path = canonical d.source_loc.Location.loc_start.pos_fname in
+           not
+             (FileSet.mem loc_path dependency_paths
+             || FileSet.mem (canonical loc_path) dependency_paths))
+
+let migratable_deprecations (deprecated_used : Cmt_utils.deprecated_used list) =
+  deprecated_used
+  |> List.filter (fun (d : Cmt_utils.deprecated_used) ->
+         Option.is_some d.migration_template
+         || Option.is_some d.migration_in_pipe_chain_template)
 
 (* Public API: migrate ~entryPointFile ~outputMode *)
 
@@ -736,18 +771,14 @@ 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
     | false -> entryPointFile
   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 +787,44 @@ 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}
-        in
-        let astTransformed =
-          apply_transforms.structure apply_transforms astMapped
+        let deprecated_used =
+          filter_deprecations_for_project ~deprecated_used ?package
+            ?dependency_paths path
         in
-        Ok
-          ( Res_printer.print_implementation
-              ~width:Res_printer.default_print_width astTransformed ~comments,
-            source )
+        let migratable_deprecated = migratable_deprecations deprecated_used in
+        if Ext_list.is_empty migratable_deprecated 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 migratable_deprecated 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 +833,30 @@ 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
+        let migratable_deprecated = migratable_deprecations deprecated_used in
+        if Ext_list.is_empty migratable_deprecated 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 migratable_deprecated 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 +865,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")))

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

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"