I had trouble earlier where cargo fix --edition resulted in a lot of mangled code, such as use ::foo::bar; -> usecrate::foo::barr; with the first space being omitted and last character duplicated.

I tracked this into the way span offsets and replacements work. As far as I could gather, the span offsets count both LF and CRLF as a single character, this means each CRLF makes span offsets off by one.

However I'm a bit confused on this for two reasons:

• "Does not work with CRLF" would seem like a problem that most people on Windows would encounter, yet I couldn't find any references to this issue anywhere. The 2018 edition has been out for almost a year so I would have expected to find reports of this elsewhere.
• The tests/parse_and_replace.rs has a not(windows) cfg with a comment to fix these tests on Windows; So at some point a problem with the tests has been recognized, but no further issue has been raised - so I'm assuming the breakage has only affected tests and the fixes themselves have been working just fine on Windows.

Is the span change a recent breakage by rustc/cargo or is this something specific to my environment? I'm testing this with a recent nightly (2019-09-30).

Previously rustfix was treating the highlight indices as byte offsets. This caused it to crash when a highlight index was pointing inside a multi-byte character.

This fix makes sure to index into the characters of the string instead of the bytes.

Fixes https://github.com/rust-lang/rust/issues/59660

Closes #75 Closes #74

I don't like the error messages, and the dirty directory change can be annoying while testing new functionality before committing.

Any ideas how to test this?

This adds a first test for edition lints (cf. #62). I'll open another PR for filtering by machine applicability in a minute.

Note: Requires custom rustc built with https://github.com/rust-lang/rust/pull/50454

cc @Manishearth

Clippy is mentioned in the README, but the README contains no information on how to use cargo-fix and clippy together.

cargo fix --help does not contain information about Clippy integration either.

In the Clippy's README and help there is nothing about rustfix either.

i am using VSCode configured to run cargo clippy on save which shows me lot of warning particularly unused imports. If I run rustfix, I get I don't have any suggestions for you right now. Check back later!.

How can I force rustfix to run and fix suggestion provided by clippy? I even tried to delete the target directory but always get the same message.

Hello,

Apologies if this isn't the right place to put this, it seemed to be rustfix functionality rather than cargo. The snippet below is a fairly oft-occurring error, and one with an obvious solution. Is there a reason rustfix doesn't add the derive attribute?

Sample code:

enum MyEnum {
    A,
    B
}

#[derive(Debug)]
struct MyStruct{
    a: u8,
    e: MyEnum
}

fn main() {
    println!("Hello, world!");
}

Output:

$ cargo fix --broken-code
    Checking fix-test v0.1.0 (...\fix-test)
error[E0277]: `MyEnum` doesn't implement `Debug`
  --> src\main.rs:10:5
   |
7  | #[derive(Debug)]
   |          ----- in this derive macro expansion
...
10 |     e: MyEnum
   |     ^^^^^^^^^ `MyEnum` cannot be formatted using `{:?}`
   |
   = help: the trait `Debug` is not implemented for `MyEnum`
   = note: add `#[derive(Debug)]` to `MyEnum` or manually `impl Debug for MyEnum`
   = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider annotating `MyEnum` with `#[derive(Debug)]`
   |
2  | #[derive(Debug)]
   |

For more information about this error, try `rustc --explain E0277`.
error: could not compile `fix-test` due to previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `fix-test` due to previous error

The rustfix tool is going to be a banner feature of the 2018 edition coming up later this year, so I think we'll want to UI to be as good as we can get it to ensure that everyone's got a smooth experience with applying the tool and updating code.

How does rustfix work today?

I've been reading the code as-is to better understand how it works today, and I'll try to summarize it here for the unfamiliar but please correct me if I'm wrong! Today you can either execute rustfix or cargo fix and they'll do the same thing. The CLI takes no arguments and has a few flags that change its operation. Just executing rustfix means that it'll instead run cargo rustc -- --message-format json.

Once Cargo/rustc is executed rustfix slurps up all the output of rustc. The stderr stream is then parsed into a list of suggestions and replacements. All suggestions are then iterated over and by default presented to the user to ask whether the fix should be applied or not. If accepted then it's queued up to be applied later. This then happens for all suggestions found, and there's a flag as well to say "don't query me, just apply everything".

Finally after all this the suggestions will be applied one-by-one, updating the files on the filesystem.

What do we want the UI for the edition to feel like?

I think we're largely undecided on this in the sense that we haven't concretely nailed this down (although I may have missed it!). I'll put forth a strawman though for how I could see this working.

First off before we release the edition we'll want to test out lints, language features, and rustfix. To do that you'll first add #![warn(rust_2018_migration)] to the crate you'd like to update. This will include src/lib.rs, src/main.rs, tests/*.rs, examples/*.rs, etc. They'll all need this annotation. After that's applied you'll execute cargo fix. This'll then apply as many fixes as it can (no user interaction), but also continue to emit normal compiler warnings about things that couldn't be fixed.

The second scenario is when we've actually released the edition itself. The only difference here is that instead of #![warn(..)] all over the place you'll instead just say rust = '2018' in your Cargo.toml.

How to implement this?

Rustfix is shaping up to be a pretty simple tool (yay!). It's largely just taking suggestions from the compiler and actually applying them on the filesystem, assuming the compiler is correctly informing rustfix of changes that need to be made.

The actual integration though is going to be pretty tricky. For a the best-quality experience we'll want to make sure that rustfix gracefully handles things like workspaces, multiple crates/targets in a workspace, cross-compilation, etc. I think that we can get all this done with a "hack" which has worked out quite well for rustbuild historically, override rustc.

A new CLI

I'm envisioning a CLI that looks like this for rustfix:

# Works like `cargo check --all-targets`, fixes all warnings that `cargo check --all-targets`
# would otherwise produce. This involves updating libraries, binaries, examples,
# tests, etc.
$ cargo fix

# Fixes all warning that `cargo build` would otherwise produce
$ cargo fix build

# Fixes all warning that `cargo test` would otherwise produce
$ cargo test

# Fixes all warnings specifically for Windows
$ cargo fix --target x86_64-pc-windows-msvc

# Fixes all warnings for just one example and the library, if any:
$ cargo fix build --example foo

The general idea is that cargo fix is the main entry point, and it otherwise mirrors Cargo's normal invocations. It will operate "as if" some other cargo command is executing, only a bunch of warnings are fixed along the way.

If a subcommand to fix isn't specified it's assumed to be check. If nothing is passed it's the special case check --all-targets to fix as much as possible. Otherwise flags are all forwarded to cargo subcommands as usual to execute various compilations.

This should have the benefit of working on workspaces, working on all targets, and hopefully even being future compatible with future Cargo features!

Overriding rustc

To actually implement the above interface I think we'll want to execute cargo (the CLI) with the RUSTC environment variable set to rustfix's binary itself. That way it'll get reexecuted and have control over the compilation.

In this way we've now provided a hook to all rustc compilations, allowing us to intercept error messages and such to see what's going on. I think the logic of the script will look like:

• Find an argument that looks like a filename. If it doesn't exist exec rustc.
• If the filename looks like it's from a crates.io crate or otherwise not relevant to the workspace we're working in, exec rustc.
• Now that we know this is a local crate, we fix all lints
  • The compiler is executed in --emit metadata mode (injected if not already present). All output is slurped up with --error-format=json as well.
  • All fixes are parsed and applied
    • This can possibly involve IPC back to the main parent process if we want interactivity
  • TODO: what if the compilation here fails?
• Next we actually compile the crate
  • The compiler is invoked with the original arguments passed to the script (no output capture)
  • This is now executing with fixed code, and the user never saw warnings that ended up being fixed
  • Warnings that weren't automatically fixed will still be emitted
  • TODO: what if compilation fails here?

And I think that's enough to basically get the feeling of "automatically fix all the code" while also assuming as little as possible about Cargo itself.

Some possible caveats are:

• There won't be an easy way to roll back fixes once we've gone past a crate. For example if a crate successfully compiles but fails other downstream compilations, it'll be hard to go back and revert the original crate.
• If the fixed code fails to compile, we may have a bad UI presented here. We can go back and revert all the fixes we applied and tell the user what happened, but they run the risk of being confused.
• We want to force Cargo to not actually cache anything and recompile local crates, so somehow we'll want to make repeated invocations of cargo fix actually run over code that may need fixing.

If we were to do this it's a pretty major rewrite/rethinking of the current CLI, so I'd like to get others' opinions on this! I'm curious if we can improve various aspects or otherwise make us better suited for the edition release!

When dealing with Reduce unused variable span, the fix keeps failing.

src:

let (mut var, unused_var) = (1, 2);

suggestion:

warning: unused variable: `var`
 --> src/lib.rs:2:10
  |
2 |     let (mut var, unused_var) = (1, 2);
  |          ^^^^^^^ help: consider using `_var` instead
  |
  = note: #[warn(unused_variables)] on by default

warning: unused variable: `unused_var`
 --> src/lib.rs:2:19
  |
2 |     let (mut var, unused_var) = (1, 2);
  |                   ^^^^^^^^^^ help: consider using `_unused_var` instead

warning: variable does not need to be mutable
 --> src/lib.rs:2:10
  |
2 |     let (mut var, unused_var) = (1, 2);
  |          ----^^^
  |          |
  |          help: remove this `mut`
  |
  = note: #[warn(unused_mut)] on by default

warning: function is never used: `main`
 --> src/lib.rs:1:1
  |
1 | fn main() {
  | ^^^^^^^^^
  |
  = note: #[warn(dead_code)] on by default

fixed:

let (_varus_unused_var(1, 2);

NOTE: the failed fix occurs both in current unreduce_var_span and reduced_var_span after the PR.

Finally -- we have a way to indicate that we trust a lint! This is currently an unstable feature in rustc and thus requires nightly.

The first lints that are currently as machine applicable land with https://github.com/rust-lang/rust/pull/50454, so adding tests will have to wait for a bit -- see #95.

(this is currently based on #96)

Currently, running cargo fix ends up printing a bunch of "Checking …" lines from the internal cargo check calls. In the step where we apply suggestions, I've now added "Fixing …" lines -- one per file. The style is the same as cargo's (it's using the same crates and color detection).

based on #82 concerns #76

Right now, there are two main interfaces for running fixes:

1. `cargo fix [--tests|--benches|--lib]
2. cargo clippy --fix

It's weird that they are so different - I would expect this to be a cargo fix --clippy option. Would it be possible for rustfix to add support so we can unify the interface?

cc @rust-lang/clippy

I have a PR to add a new suggestion to rustc: https://github.com/rust-lang/rust/pull/88672. It uses Diagnostic::multipart_suggestions since there are multiple ways for the user to change their code. These multiple suggestions are exclusive since they modify the same code.

However, rustfix is panicking when I use run-rustfix in a test: "Cannot replace slice of data that was already replaced".

The relevant code in my PR is here: https://github.com/rust-lang/rust/blob/6a89e97ab14cd4c6be70b47fd139c87fd90150b7/compiler/rustc_parse/src/parser/diagnostics.rs#L226-L234 and here: https://github.com/rust-lang/rust/blob/6a89e97ab14cd4c6be70b47fd139c87fd90150b7/compiler/rustc_parse/src/parser/diagnostics.rs#L1272-L1277

I'm not sure how to fix this problem in rustfix, but I'm willing to try with some mentoring :)

Sometimes a specific lint is the bulk of the suggestions. It would be cool if rustfix supported filtering by lint, enabling cargo clippy --fix=absurd-extreme-comparisons or whatever.

When the compiler or clippy produce suggestions, some of them are marked as MaybeIncorrect; i.e. the suggestion should probably not be automatically applied. rustfix currently doesn't touch these.

This leads to confusion like in https://github.com/rust-lang/cargo/issues/8806, and is somewhat frustrating. We should at the very least be clear that such suggestions were not applied.

Ideally, we should provide a mode where you can apply them anyway (cargo clippy --fix --apply-all-suggestions?), or apply them using a "pick and choose" mode. We can then mention it when people have suggestions that were not applied.

See also: https://github.com/rust-lang/rustfix/issues/163

The following example:

macro_rules! foo {
    () => {
        &1;
    };
}

fn main() {
    foo!();
    foo!();
}

generates a suggestion (on beta 1.55) to insert a fix like this:

let _ = &1;

However, since this diagnostic is triggered from macros, rustc emits two separate machine-applicable suggestions at the exact same spot. That causes rustfix to end up changing the code to:

let _ = let _ = &1;

which fails to compile.

#131 added detection to avoid applying duplicate suggestions, but I believe it does not handle the case when it is an insert_only suggestion.

Some suggestions include overlapping regions. These cause rustfix to fail with an error like:

Could not replace range 22...47 in file -- maybe parts of it were already replaced?

An example that triggers this is the following where multiple trait bounds are to be removed:

pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;

This results in the following suggestion:

warning: bounds on generic parameters are not enforced in type aliases
 --> src/main.rs:1:25
  |
1 | pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;
  |                         ^^^^^^^^^^^^^^^^^   ^^^^   ^^^^
  |
  = note: `#[warn(type_alias_bounds)]` on by default
help: the bound will not be checked when the type alias is used, and should be removed
  |
1 - pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;
1 + pub type MyResult<T, E> = Result<T, E>;
  |

With the suggested JSON:

{
    "reason": "compiler-message",
    "package_id": "z2 0.1.0 (path+file:///foo)",
    "manifest_path": "/foo/Cargo.toml",
    "target":
    {
        "kind":
        [
            "bin"
        ],
        "crate_types":
        [
            "bin"
        ],
        "name": "z2",
        "src_path": "/foo/src/main.rs",
        "edition": "2018",
        "doc": true,
        "doctest": false,
        "test": true
    },
    "message":
    {
        "rendered": "warning: bounds on generic parameters are not enforced in type aliases\n --> src/main.rs:1:25\n  |\n1 | pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;\n  |                         ^^^^^^^^^^^^^^^^^   ^^^^   ^^^^\n  |\n  = note: `#[warn(type_alias_bounds)]` on by default\nhelp: the bound will not be checked when the type alias is used, and should be removed\n  |\n1 - pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;\n1 + pub type MyResult<T, E> = Result<T, E>;\n  | \n\n",
        "children":
        [
            {
                "children":
                [],
                "code": null,
                "level": "note",
                "message": "`#[warn(type_alias_bounds)]` on by default",
                "rendered": null,
                "spans":
                []
            },
            {
                "children":
                [],
                "code": null,
                "level": "help",
                "message": "the bound will not be checked when the type alias is used, and should be removed",
                "rendered": null,
                "spans":
                [
                    {
                        "byte_end": 41,
                        "byte_start": 22,
                        "column_end": 42,
                        "column_start": 23,
                        "expansion": null,
                        "file_name": "src/main.rs",
                        "is_primary": true,
                        "label": null,
                        "line_end": 1,
                        "line_start": 1,
                        "suggested_replacement": "",
                        "suggestion_applicability": "MachineApplicable",
                        "text":
                        [
                            {
                                "highlight_end": 42,
                                "highlight_start": 23,
                                "text": "pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;"
                            }
                        ]
                    },
                    {
                        "byte_end": 48,
                        "byte_start": 22,
                        "column_end": 49,
                        "column_start": 23,
                        "expansion": null,
                        "file_name": "src/main.rs",
                        "is_primary": true,
                        "label": null,
                        "line_end": 1,
                        "line_start": 1,
                        "suggested_replacement": "",
                        "suggestion_applicability": "MachineApplicable",
                        "text":
                        [
                            {
                                "highlight_end": 49,
                                "highlight_start": 23,
                                "text": "pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;"
                            }
                        ]
                    },
                    {
                        "byte_end": 55,
                        "byte_start": 22,
                        "column_end": 56,
                        "column_start": 23,
                        "expansion": null,
                        "file_name": "src/main.rs",
                        "is_primary": true,
                        "label": null,
                        "line_end": 1,
                        "line_start": 1,
                        "suggested_replacement": "",
                        "suggestion_applicability": "MachineApplicable",
                        "text":
                        [
                            {
                                "highlight_end": 56,
                                "highlight_start": 23,
                                "text": "pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;"
                            }
                        ]
                    }
                ]
            }
        ],
        "code":
        {
            "code": "type_alias_bounds",
            "explanation": null
        },
        "level": "warning",
        "message": "bounds on generic parameters are not enforced in type aliases",
        "spans":
        [
            {
                "byte_end": 41,
                "byte_start": 24,
                "column_end": 42,
                "column_start": 25,
                "expansion": null,
                "file_name": "src/main.rs",
                "is_primary": true,
                "label": null,
                "line_end": 1,
                "line_start": 1,
                "suggested_replacement": null,
                "suggestion_applicability": null,
                "text":
                [
                    {
                        "highlight_end": 42,
                        "highlight_start": 25,
                        "text": "pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;"
                    }
                ]
            },
            {
                "byte_end": 48,
                "byte_start": 44,
                "column_end": 49,
                "column_start": 45,
                "expansion": null,
                "file_name": "src/main.rs",
                "is_primary": true,
                "label": null,
                "line_end": 1,
                "line_start": 1,
                "suggested_replacement": null,
                "suggestion_applicability": null,
                "text":
                [
                    {
                        "highlight_end": 49,
                        "highlight_start": 45,
                        "text": "pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;"
                    }
                ]
            },
            {
                "byte_end": 55,
                "byte_start": 51,
                "column_end": 56,
                "column_start": 52,
                "expansion": null,
                "file_name": "src/main.rs",
                "is_primary": true,
                "label": null,
                "line_end": 1,
                "line_start": 1,
                "suggested_replacement": null,
                "suggestion_applicability": null,
                "text":
                [
                    {
                        "highlight_end": 56,
                        "highlight_start": 52,
                        "text": "pub type MyResult<T, E: std::error::Error + Send + Sync> = Result<T, E>;"
                    }
                ]
            }
        ]
    }
}

This has three overlapping spans that suggest a replacement of an empty string:

• 22..41
• 22..48
• 22..55

I think it might be feasible to support this. At least, the new diff output from rustc is able to handle it, and clicking the "fix" suggestions in my editor is able to handle it (the regions are "smart" and know when the text within it is modified).

Note: This is relatively new behavior introduced by #195.