Migration tool fixes #8157
Migration tool fixes #8157
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 650d0c6da2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tools/src/migrate.ml
Outdated
| 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)) |
There was a problem hiding this comment.
The dependency filter is keyed off d.source_loc, which is the location of the deprecated use (recorded via Builtin_attributes.check_deprecated at the call site). That means when a project file calls a deprecated API from a dependency, source_loc still points to the project file, so this filter won’t drop it and the migration will still rewrite user code. This contradicts the intended “skip dependency migrations” behavior (e.g., a project’s DeprecatedDep.oldThing() call will still be migrated). To suppress dependency-originated migrations you need to filter by where the deprecated item is defined, not the usage location.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
How would you suggest fixing this?
There was a problem hiding this comment.
@codex verify that this has been fixed now please.
|
@codex review again please. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d98a2244d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tools/src/migrate.ml
Outdated
| 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 |
There was a problem hiding this comment.
The short-circuit only checks whether deprecated_used is empty, but that list is populated for any @deprecated use, even when both migration_template and migration_in_pipe_chain_template are None. In that case the code still parses/prints the file and will mark it as Changed if the printer normalizes formatting, even though no migration can be applied. This reintroduces the “format-only” rewrite the change is trying to prevent for files that only hit non‑migratable deprecations; consider filtering to entries that actually carry a migration template before deciding to parse/print.
Useful? React with 👍 / 👎.
tests/tools_tests/test.sh
Outdated
| # Capture stderr too so warnings would surface in expected output if they occur. | ||
| ../../_build/install/default/bin/rescript-tools migrate "$file" --stdout 2>&1 > $output |
There was a problem hiding this comment.
The redirection order 2>&1 > $output redirects stderr to the original stdout before stdout is sent to the file, so stderr still goes to the terminal and will not appear in the expected output. This contradicts the comment and means warnings won’t be captured by the test; use > $output 2>&1 (or an equivalent pipe) to combine the streams.
Useful? React with 👍 / 👎.
|
@codex review again please. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cef32da1d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| |> 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)) |
There was a problem hiding this comment.
This filter uses d.source_loc to decide whether a deprecation comes from a dependency, but source_loc is recorded at the use site (see Builtin_attributes.check_deprecated → record_deprecated_used), not at the deprecated definition. For a project file that calls a deprecated API from a dependency (e.g. DeprecatedDep.oldThing()), source_loc is the project file path, so it will never match dependency_paths and the migration still runs. That defeats the new goal of skipping dependency migrations and will keep rewriting dependency-sourced deprecations. Consider threading the deprecated definition’s origin/module or path into deprecated_used and filtering on that instead.
Useful? React with 👍 / 👎.
This fixes so that the migration tool a) does not touch files it does not apply any migrations to (it used to format them), and b) does not attempt to do migrations on dependencies.
Closes #8152, #8096