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

Automatic migration of breaking library updates #30

Open
kangalio opened this issue Nov 23, 2022 · 6 comments
Open

Automatic migration of breaking library updates #30

kangalio opened this issue Nov 23, 2022 · 6 comments
Labels
A-user-story Area: A user story or a related issue

Comments

@kangalio
Copy link

Lint explanation

In poise, we recently pushed a release that renames a few common functions and struct fields. Applying those changes is an annoying and mechanical task for users that could have been automated well:

  • .user_data_setup() -> .setup()
  • api_function(ctx.discord()) -> api_function(ctx)
  • other_function(ctx.discord()) -> other_function(ctx.serenity_context())
  • listener: |event| {} -> event_handler: |event| {}

In serenity, there's a huge breaking release (0.12) coming up that reworks all instances of the builder pattern from this (example):

api_function(|builder| builder
    .required_field(req_value)
    .optional_field(opt_value)
    .nested_buider(|nested_builder| nested_builder
        .optional_field_2(opt_value_2)
    )
)

to

api_function(CreateBuilder::new(req_value)
    .optional_field(opt_value)
    .nested_buider(CreateNestedBuilder::new()
        .optional_field_2(opt_value_2)
    )
)

We're all kinda afraid of pushing out this release because of how much churn it will cause. It would be great to have a way of automatically migrating users' code.

Example code

See above

Notes

This issue is technically not about linting but about code rewriting. However, Rust's code rewriting infrastructure, cargo fix, is based on lints. Here's how I'd imagine serenity to use custom lints to aid in migration:

Publish a serenity-migration crate which lints all instances of the builder pattern and suggests the rewritten form. The user would run these lints while still on the old serenity version 0.11 (otherwise, the code would fail to parse/type-check). Now the code has been rewritten for serenity 0.12. Finally, the user bumps the serenity crate version to 0.12.

@kangalio kangalio added the A-user-story Area: A user story or a related issue label Nov 23, 2022
@xFrednet
Copy link
Member

Thank you very much for the user story. Including suggestions with an applicability is definitely part of my goals. Using it to migrate libraries was something I haven't thought about so far. It makes a lot of sense to use it for that as well, like rustc already does for editions. Awesome input!

I'm currently actively working on this project, but it'll still take time until this can be accomplished and then tested on a use case like this. Best of luck and let's hope that we can make this automatic migration happen some day!

@kangalio
Copy link
Author

kangalio commented Nov 24, 2022

Looking at cargo fix options:

Screenshot_20221124_140531

Its edition migration works the same way I imagined dependency migration to work: run cargo fix on the old version which upgrades the code to the next version. In order to be valid code, you then manually upgrade the version.

Perhaps cargo fix could add a --dependency/--dep flag which runs and fixes the lints from the specified dependency? For example cargo fix --dep serenity_migration (we can't use --package because cargo fix already uses that to mean the workspace package to apply the fixes to)

@xFrednet
Copy link
Member

rustfix is build on top of rustc's error reporting. The current architecture is intended to support different drivers, like rustc and maybe/hopefully rust-analyzer one day. If we report the lints over rustc, rustfix could handle them immediately. Clippy does the same. The dream is to later run cargo linter --fix just like it's possible for Clippy rn

@kangalio
Copy link
Author

kangalio commented Nov 25, 2022

After seeing how easy it is to run a rustc wrapper using rustc_driver, I spent the day coding up a custom binary that migrates serenity closure-style builders: https://github.com/kangalioo/serenity_migration (not 100% working yet)

I learned many complexities and idiosyncrasies of rustc's private API. In the long term, rust-linting should probably evolve so that serenity_migration could have been written using rust-linting, which should make the code much more straightforward.

@xFrednet
Copy link
Member

That's a good solution for now, and welcome to rustc's internals. They're not always the intuitive.

Just a small warning, since I'm currently working on that part of marker. Rustc's api is instable, and your crate is bound to a specific nightly. cargo install ignores the rust-toolchian file AFAIK. So, you might have to add extra instructions, that users must use cargo +nightly-yyyy-mm-dd install XYZ or some other form to run your project.

@shepmaster
Copy link

I did something similar for SNAFU where I ran the compiler as a whole (via cargo check), parsed the output using --message-format json, then edited the resulting files with regex and other string manipulation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-user-story Area: A user story or a related issue
Projects
None yet
Development

No branches or pull requests

3 participants