Skip to content

fix: Iteratively apply suggestions from the compiler#5842

Merged
bors merged 1 commit intorust-lang:masterfrom
alexcrichton:fix-lots
Aug 1, 2018
Merged

fix: Iteratively apply suggestions from the compiler#5842
bors merged 1 commit intorust-lang:masterfrom
alexcrichton:fix-lots

Conversation

@alexcrichton
Copy link
Member

This commit updates the cargo fix implementation to iteratively apply fixes
from the compiler instead of only once. Currently the compiler can sometimes
emit overlapping suggestions, such as in the case of transitioning

::foo::<::Bar>();

to ...

crate::foo::<crate::Bar>();

and rustfix rightfully can't handle overlapping suggestions as there's no
clear way of how to disambiguate the fixes. To fix this problem Cargo will now
run rustc and rustfix multiple times, attempting to reach a steady state
where no fixes failed to apply.

Naturally this is a pretty tricky thing to do and we want to be sure that Cargo
doesn't loop forever, for example. A number of safeguards are in place to
prevent Cargo from going off into the weeds when fixing files, notably avoiding
to reattempt fixes if no successful fixes ended up being applied.

Closes #5813
Closes rust-lang/rust#52754

@rust-highfive
Copy link

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

Cargo.toml Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this is waiting on rust-lang/rustfix#140 to be merged and published, so we'll want to hold off on merging this PR until that happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

now published!

@alexcrichton
Copy link
Member Author

r? @ehuss

@alexcrichton
Copy link
Member Author

cc @killercup, you may be interested in this as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

s/othrewise/otherwise

Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to explicitly call iter instead of just &fixes.files? (same below)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm personally not a huge fan of for x in &y as opposed to for x in y or for x in y.iter(), the latter one feels more clear to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

4 is a pretty magic number here, maybe put it in a binding whose name we can search for?

maybe even do env::var("CARGO_FIX_MAX_RETRIES").and_then(|n| n.parse()).unwrap_or(4)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor

Choose a reason for hiding this comment

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

drop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah this is a "poor man's way" of debugging the test if it fails (as the output is swallowed up if successful). That way it allows looking through just the test output to see why the output didn't exist!

Copy link
Contributor

Choose a reason for hiding this comment

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

as the output is swallowed up if successful

TIL. Very neat!

This commit updates the `cargo fix` implementation to iteratively apply fixes
from the compiler instead of only once. Currently the compiler can sometimes
emit overlapping suggestions, such as in the case of transitioning

    ::foo::<::Bar>();

to ...

    crate::foo::<crate::Bar>();

and `rustfix` rightfully can't handle overlapping suggestions as there's no
clear way of how to disambiguate the fixes. To fix this problem Cargo will now
run `rustc` and `rustfix` multiple times, attempting to reach a steady state
where no fixes failed to apply.

Naturally this is a pretty tricky thing to do and we want to be sure that Cargo
doesn't loop forever, for example. A number of safeguards are in place to
prevent Cargo from going off into the weeds when fixing files, notably avoiding
to reattempt fixes if no successful fixes ended up being applied.

Closes rust-lang#5813
Closes rust-lang/rust#52754
@ehuss
Copy link
Contributor

ehuss commented Aug 1, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Aug 1, 2018

📌 Commit 876a503 has been approved by ehuss

@bors
Copy link
Contributor

bors commented Aug 1, 2018

⌛ Testing commit 876a503 with merge 0b80061...

bors added a commit that referenced this pull request Aug 1, 2018
fix: Iteratively apply suggestions from the compiler

This commit updates the `cargo fix` implementation to iteratively apply fixes
from the compiler instead of only once. Currently the compiler can sometimes
emit overlapping suggestions, such as in the case of transitioning

    ::foo::<::Bar>();

to ...

    crate::foo::<crate::Bar>();

and `rustfix` rightfully can't handle overlapping suggestions as there's no
clear way of how to disambiguate the fixes. To fix this problem Cargo will now
run `rustc` and `rustfix` multiple times, attempting to reach a steady state
where no fixes failed to apply.

Naturally this is a pretty tricky thing to do and we want to be sure that Cargo
doesn't loop forever, for example. A number of safeguards are in place to
prevent Cargo from going off into the weeds when fixing files, notably avoiding
to reattempt fixes if no successful fixes ended up being applied.

Closes #5813
Closes rust-lang/rust#52754
@bors
Copy link
Contributor

bors commented Aug 1, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: ehuss
Pushing 0b80061 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cargo-fix: Overlapping suggestions should be discarded and cargo-fix rerun. Lint suggesting crate:: makes overlapping suggestions

7 participants