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

Add forc-migrate tool #6790

Merged
merged 18 commits into from
Jan 20, 2025
Merged

Add forc-migrate tool #6790

merged 18 commits into from
Jan 20, 2025

Conversation

ironcev
Copy link
Member

@ironcev ironcev commented Dec 16, 2024

Description

This PR introduces forc-migrate, a Forc tool for migrating Sway projects to the next breaking change version of Sway.

The tool addresses two points crucial for code updates caused by breaking changes:

  • it informs developers about the breaking changes and assists in planing and executing the migration.
  • it automatically changes source code where possible, reducing the manual effort needed for code changes.

Besides adding the forc-migrate tool, the PR:

The migration for the references feature, migrating ref mut to &mut, is developed only partially, to demonstrate the development and usage of automatic migrations that alter the original source code.

The intended usage of the tool is documented in detail in the "forc migrate" chapter of The Sway Book: Forc reference > Plugins > forc_migrate. (The generated documentation has issues that are caused by the documentation generation bug explained in #6792. These issues will be fixed in a separate PR that will fix it for all the plugins.)

We expect the forc-migrate to evolve based on the developer's feedback. Some of the possible extensions of the tool are:

  • adding additional CLI options, e.g., for executing only specific migration steps, or ignoring them.
  • passing parameters to migration steps from the CLI.
  • not allowing updates by default, if the repository contains modified or untracked files.
  • migrating workspaces.
  • migrating other artifacts, e.g., Forc.toml files or contract IDs.
  • migrating between arbitrary versions of Sway.
  • migrating SDK code.
  • etc.

forc-migrate also showed a clear need for better infrastructure for writing static analyzers and transforming Sway code. The approach used in the implementation of this PR should be seen as a pragmatic beginning, based on the reuse of what we currently have. Some future options are discussed in #6836.

Demo

forc migrate show

Shows the breaking change features and related migration steps. This command can be run anywhere and does not require a Sway project.

Breaking change features:
  - storage_domains    (https://github.com/FuelLabs/sway/issues/6701)
  - references         (https://github.com/FuelLabs/sway/issues/5063)

Migration steps (1 manual and 1 semiautomatic):
storage_domains
  [M] Review explicitly defined slot keys in storage declarations (`in` keywords)

references
  [S] Replace `ref mut` function parameters with `&mut`

Experimental feature flags:
- for Forc.toml:  experimental = { storage_domains = true, references = true }
- for CLI:        --experimental storage_domains,references

forc migrate check

Performs a dry-run of the migration on a concrete Sway project. It outputs all the occurrences in code that need to be reviewed or changed, as well as the migration time effort:

info: [storage_domains] Review explicitly defined slot keys in storage declarations (`in` keywords)
  --> /home/kebradalaonda/Desktop/M Forc migrate tool/src/main.sw:19:10
   |
...
19 |     y in b256::zero(): u64 = 0,
   |          ------------
20 |     z: u64 = 0,
21 |     a in calculate_slot_address(): u64 = 0,
   |          ------------------------
22 |     b in 0x0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20: u64 = 0,
   |          ------------------------------------------------------------------
   |
   = help: If the slot keys used in `in` keywords represent keys generated for `storage` fields
   = help: by the Sway compiler, those keys might need to be recalculated.
   = help:  
   = help: The previous formula for calculating storage field keys was: `sha256("storage.<field name>")`.
   = help: The new formula is:                                          `sha256((0u8, "storage.<field name>"))`.
   = help:  
   = help: For a detailed migration guide see: https://github.com/FuelLabs/sway/issues/6701
____

Migration effort:

storage_domains
  [M] Review explicitly defined slot keys in storage declarations (`in` keywords)
      Occurrences:     3    Migration effort (hh::mm): ~00:06

references
  [S] Replace `ref mut` function parameters with `&mut`
      Occurrences:     0    Migration effort (hh::mm): ~00:00

Total migration effort (hh::mm): ~00:06

forc migrate run

Runs the migration steps and guides developers through the migration process.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@ironcev ironcev self-assigned this Dec 16, 2024
@ironcev ironcev added forc forc-migrate Everything to do with the forc-migrate tool. labels Dec 16, 2024
@ironcev
Copy link
Member Author

ironcev commented Dec 17, 2024

To try out the forc migrate tool locally:

  1. Pull and checkout the ironcev/forc-migrate-tool branch.
  2. Install the tool from the Sway repository: cargo install --path ./forc-plugins/forc-migrate

For the detailed documentation on migration and the usage of the tool see the scripts/mdbook-forc-documenter/examples/forc_migrate.md in this PR.

Copy link
Contributor

@IGI-111 IGI-111 left a comment

Choose a reason for hiding this comment

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

The tool seems fine, but the migration for #6701 is inverted: users won't need to migrate slots that specify their position with the in keyword, however any slot that doesn't specify their position will have to be set to a fixed, old, position to guarantee backwards compatibility.

The lifecycle of migrations is (primarily) for existing codebases that are deployed behind proxies and need to maintain storage compatibility.

The guide in that linked issue should be fixed to that effect too.

tritao
tritao previously approved these changes Jan 7, 2025
Copy link
Contributor

@tritao tritao left a comment

Choose a reason for hiding this comment

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

Overall looks great, nice work, very nicely commented and engineered.

Left just some minor pedantic nits (feel free to ignore!).

I do have a question regarding the overall design.

Given the code to track the manual effort duration for code changes and manual transformation suggestions, I take it (at least for now) the tool will work in a semi-automatic, as opposed to fully automatic?

forc-plugins/forc-migrate/src/cli/commands/run.rs Outdated Show resolved Hide resolved
forc-plugins/forc-doc/src/cli.rs Outdated Show resolved Hide resolved
scripts/mdbook-forc-documenter/examples/forc_migrate.md Outdated Show resolved Hide resolved
forc-plugins/forc-migrate/src/lib.rs Show resolved Hide resolved
forc-plugins/forc-migrate/src/cli/shared.rs Show resolved Hide resolved
forc-plugins/forc-migrate/src/cli/mod.rs Show resolved Hide resolved
@ironcev
Copy link
Member Author

ironcev commented Jan 8, 2025

The lifecycle of migrations is (primarily) for existing codebases that are deployed behind proxies and need to maintain storage compatibility.

@IGI-111 Completely agree. I gave it a deeper thought and have a design that will cover both the rare case which is supported now and also contracts behind proxies. The two will play well together and also with the SRC-14 (no suggestions to check in keywords for SRC-14 fields and fileds that have in keywords for compatibility, etc.).

I am moving the PR to draft until that is implemented.

@ironcev ironcev marked this pull request as draft January 8, 2025 08:40
@ironcev
Copy link
Member Author

ironcev commented Jan 8, 2025

Given the code to track the manual effort duration for code changes and manual transformation suggestions, I take it (at least for now) the tool will work in a semi-automatic, as opposed to fully automatic?

@tritao It depends on the concrete breaking changes and also on the effort we want to put into writing migrations, compared to eventual manual effort. E.g., in the sample case of changing ref mut to &mut the migration can find and change usages etc. and make the step fully automatic.

But in a completely general case, I don't see a fully automatic migration as always possible, even theoretically. Also, the sensitivity of any migration always requires manual supervision. That's why I've opted for the proposed design in which the tool deliberately stops after a single feature is fully migrated and asks for a code review, even if all the migration steps were fully automated.

Having manual supervision and semi-automatic migrations also corresponds to my experience so far with code migrations. I see the goal of the tool in taking over the mundane and repetitive effort and pointing where to look, but programmers should still be aware of the changes and understand their impact.

Copy link

codspeed-hq bot commented Jan 17, 2025

CodSpeed Performance Report

Merging #6790 will not alter performance

Comparing ironcev/forc-migrate-tool (d7e94a2) with master (3f7b5f1)

Summary

✅ 22 untouched benchmarks

@ironcev
Copy link
Member Author

ironcev commented Jan 17, 2025

@IGI-111 @tritao @JoshuaBatty Thanks for reviewing the forc migrate tool. Can I ask you for a quick re-review of the latest changes? I need this PR merged to finalize the next one which brings additional migrations.

The latest changes address your remarks and also add some ~1000 LOC to the anyhow big PR. Still, it should be fairly easy to review.

The majority of the changes goes to infrastructure for matching and modifying trees. I hoped to get the first version of the forc migrate without necessarily developing such infrastructure. But it turned out that the LOC ratio between the code that does manual matching and tree modifications and the actual migration logic went to almost 3:1! This made the migrations error-prone and difficult to maintain. I expect in the future more improvements in this area as already discussed between @tritao and me (see: #6836). But for the time being, based on lot of concrete examples, I see the simple infrastructure provided in this PR as more than sufficient for writing readable and easy-to-write migrations.

@ironcev ironcev marked this pull request as ready for review January 17, 2025 03:22
@JoshuaBatty JoshuaBatty requested review from a team January 19, 2025 20:56
@IGI-111 IGI-111 merged commit 7aa59f9 into master Jan 20, 2025
45 checks passed
@IGI-111 IGI-111 deleted the ironcev/forc-migrate-tool branch January 20, 2025 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forc forc-migrate Everything to do with the forc-migrate tool.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants