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

Correct the bug report for cargo clippy --fix #11882

Merged
merged 3 commits into from
Apr 14, 2023

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Mar 24, 2023

What does this PR try to resolve?

close #11877

Correct the bug report for cargo clippy --fix.

How should we test and review this PR?

See the unit tests.

@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2023

r? @weihanglo

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

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-diagnostics Area: Error and warning messages generated by Cargo itself. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 24, 2023
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

I am trying to add a test for it. But it seems to be a bit difficult. If you have any suggestions please let me now. Thanks!

@weihanglo
Copy link
Member

Some tests like check_fixable_warning_for_clippy do similar stuff I believe.

@Rustin170506
Copy link
Member Author

Rustin170506 commented Mar 25, 2023

Some tests like check_fixable_warning_for_clippy do similar stuff I believe.

Yeah, I am trying to combine it with broken_fixes_backed_out. Thanks!

@Rustin170506 Rustin170506 marked this pull request as ready for review March 25, 2023 12:52
@Rustin170506

This comment was marked as outdated.

@Rustin170506
Copy link
Member Author

@weihanglo Friendly ping~
Could you please take a look when you have time? Thanks!

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I don't think we'll have more tools in a near future, so this hard-coded solution effectively fixes the issue IMO.

Only some minor suggestions.

src/cargo/core/compiler/job_queue/mod.rs Outdated Show resolved Hide resolved
tests/testsuite/fix.rs Outdated Show resolved Hide resolved
tests/testsuite/fix.rs Outdated Show resolved Hide resolved
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@rustbot rustbot added the A-testing-cargo-itself Area: cargo's tests label Apr 14, 2023
@Rustin170506
Copy link
Member Author

Only some minor suggestions.

Thanks for your review! 💚 💙 💜 💛 ❤️ All addressed!

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Thanks for the update!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 14, 2023

📌 Commit 08169fd has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 14, 2023
@bors
Copy link
Contributor

bors commented Apr 14, 2023

⌛ Testing commit 08169fd with merge b0742b2...

@bors
Copy link
Contributor

bors commented Apr 14, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing b0742b2 to master...

@bors bors merged commit b0742b2 into rust-lang:master Apr 14, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 16, 2023
…ehuss

Make cargo a workspace

8 commits in 7bf43f028ba5eb1f4d70d271c2546c38512c9875..39116ccc9b420a883a98a960f0597f9cf87414b8
2023-04-10 16:01:41 +0000 to 2023-04-15 20:24:15 +0000

- Make cargo a workspace (rust-lang/cargo#11851)
- Fix flaky not_found_permutations test. (rust-lang/cargo#11976)
- Use restricted Damerau-Levenshtein algorithm (rust-lang/cargo#11963)
- Correct the bug report for `cargo clippy --fix` (rust-lang/cargo#11882)
- Stabilize `cargo logout` (rust-lang/cargo#11950)
- Add more information to HTTP errors to help with debugging. (rust-lang/cargo#11878)
- Use registry.default for login/logout (rust-lang/cargo#11949)
- Change -C to be unstable (rust-lang/cargo#11960)

---

### What does this PR try to resolve?

Making cargo a workspace.

Why doing this?

* `rustc-workspace-hack` is primarily for sharing dependencies between rls and cargo, as rls previously depends on cargo. After rls retired, it is no longer the case sharing dependencies.
* It's q bit painful that cargo needs to deal with some dependency and licensing complexities. For example, rust-lang#108665 failed because of the interaction bewteen `windows-sys` and `raw-dylib`. It currenctly blocks cargo's feature `-Zgitxodie` from moving forward.
* See rust-lang/cargo#11851

### Benchmark result

I've done a simple benchmark on both keeping or removing entire `rustc-workspace-hack`. It had no significant difference. Both took ~2m30s to finish `./x.py build -j8 src/tools/cargo src/tools/rls src/tools/clippy src/tools/miri src/tools/rustfmt`. Environment info:

```
host: aarch64-apple-darwin
os: Mac OS 13.2.1 [64-bit]
```

A sophisticated benchmark may be needed.

### Additional information

This depends on prior works from `@Muscraft` and `@ehuss.` Credits to them!
@ehuss ehuss added this to the 1.71.0 milestone Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-diagnostics Area: Error and warning messages generated by Cargo itself. A-testing-cargo-itself Area: cargo's tests S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo fix hard-codes the bug report URL
5 participants