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

Only ensure solutions are in the same file in cargo fix #6402

Merged
merged 2 commits into from
Dec 10, 2018

Conversation

pietroalbini
Copy link
Member

@pietroalbini pietroalbini commented Dec 8, 2018

This PR changes cargo fix to avoid rejecting machine-applicable lints with multiple suggestions. Instead, now it only checks the solutions are in the same file.

I don't know how to write a test for this, since no lint in rustc relies on the new behavior and the existing cargo fix tests just invoke lints in rustc. Any idea on that would be appreciated.

r? @alexcrichton
cc @killercup
fixes #6401

@pietroalbini
Copy link
Member Author

Trying this with my local rustc branch doesn't work yet, let me fix it.

@pietroalbini
Copy link
Member Author

Nevermind, ran the wrong cargo binary... This patch works fine.

it works!

@pietroalbini

This comment has been minimized.

Copy link
Member

@killercup killercup left a comment

Choose a reason for hiding this comment

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

Looks good. Ideally, we'd add a test here, too, but that is probably blocked on rust-lang/rust#56645

src/cargo/ops/fix.rs Outdated Show resolved Hide resolved
@pietroalbini pietroalbini changed the title Only ensure solutions are in the same file in cargo fix [WIP] Only ensure solutions are in the same file in cargo fix Dec 9, 2018
Co-Authored-By: Pascal Hertleif <killercup@gmail.com>
@pietroalbini pietroalbini changed the title [WIP] Only ensure solutions are in the same file in cargo fix Only ensure solutions are in the same file in cargo fix Dec 9, 2018
@pietroalbini
Copy link
Member Author

Ok, removed the bump to rustfix 0.4.3. This is ready to be reviewed, even though it's not enough on its own for rust-lang/rust#56645 (needs a fix for rust-lang/rust#53934 before I can fix all the unused_imports).

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Dec 10, 2018

📌 Commit a9f64b2 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Dec 10, 2018

⌛ Testing commit a9f64b2 with merge e074068...

bors added a commit that referenced this pull request Dec 10, 2018
Only ensure solutions are in the same file in cargo fix

This PR changes `cargo fix` to avoid rejecting machine-applicable lints with multiple suggestions. Instead, now it only checks the solutions are in the same file.

I don't know how to write a test for this, since no lint in rustc relies on the new behavior and the existing `cargo fix` tests just invoke lints in rustc. Any idea on that would be appreciated.

r? @alexcrichton
cc @killercup
fixes #6401
@bors
Copy link
Contributor

bors commented Dec 10, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing e074068 to master...

@bors bors merged commit a9f64b2 into rust-lang:master Dec 10, 2018
@pietroalbini pietroalbini deleted the fix-cargofix branch December 10, 2018 06:34
@ehuss ehuss added this to the 1.33.0 milestone Feb 6, 2022
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 shouldn't skip suggestions with multiple snippets but only one replacement
5 participants