Skip to content

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Sep 15, 2025

This commit can be replicated by running cargo update -p rustfix --precise 0.8.7 && x test ui --bless.


The reasons this affects UI tests is as follows:

  • The UI test suite runs rustc with -Z deduplicate-diagnostics=no --error-format=json, which means that rustc emits multiple errors containing identical suggestions. That caused the weird-looking code that had multiple X: Copy suggestions.
  • Those suggestions are interpreted not by rustc itself, but by the rustfix library, maintained by cargo but published as a separate crates.io library and used by compiletest.
  • Sometime between rustfix 0.8.1 and 0.8.7 (probably in Add transactional semantics to rustfix cargo#14747, but it's hard to tell because rustfix's versioning doesn't match cargo's), rustfix got smarter and stopped applying duplicate suggestions.

Update rustfix to match cargo's behavior. Ideally, we would always share a version of rustfix between cargo and rustc (perhaps with a path dependency?), to make sure we are testing the behavior we ship. But for now, just manually update it to match.

Note that the latest version of rustfix published to crates.io is 0.9.1, not 0.8.7. But 0.9.1 is not the version used in cargo, which is 0.9.3. Rather than trying to match versions exactly, I just updated rustfix to the latest in the 0.8 branch.

@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 15, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2025

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot

This comment has been minimized.

This commit can be replicated by running
`cargo update -p rustfix --precise 0.8.7 && x test ui --bless`.

---

The reasons this affects UI tests is as follows:
- The UI test suite runs rustc with
    `-Z deduplicate-diagnostics=no --error-format=json`,
  which means that rustc emits multiple errors containing identical
  suggestions. That caused the weird-looking code that had multiple `X: Copy` suggestions.
- Those suggestions are interpreted not by rustc itself, but by the
  `rustfix` library, maintained by cargo but published as a separate
  crates.io library and used by compiletest.
- Sometime between rustfix 0.8.1 and 0.8.7 (probably in cargo 14747, but
  it's hard to tell because rustfix's versioning doesn't match cargo's),
  rustfix got smarter and stopped applying duplicate suggestions.

Update rustfix to match cargo's behavior. Ideally, we would always share
a version of rustfix between cargo and rustc (perhaps with a path
dependency?), to make sure we are testing the behavior we ship. But for
now, just manually update it to match.

Note that the latest version of rustfix published to crates.io is 0.9.1,
not 0.8.7. But 0.9.1 is not the version used in cargo, which is 0.9.3.
Rather than trying to match versions exactly, I just updated rustfix to
the latest in the 0.8 branch.

#[derive(Debug, Copy, Clone)] //~ ERROR the trait `Copy` cannot be implemented for this type
pub struct AABB<K: Copy + Debug + std::fmt::Debug + std::fmt::Debug + std::fmt::Debug>{
pub struct AABB<K: Copy + Debug + std::fmt::Debug>{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll give you a gold star if you also add the missing space before the {!

Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

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

The de-duplication is nice.

View changes since this review

@nnethercote
Copy link
Contributor

Note that the latest version of rustfix published to crates.io is 0.9.1, not 0.8.7. But 0.9.1 is not the version used in cargo, which is 0.9.3. Rather than trying to match versions exactly, I just updated rustfix to the latest in the 0.8 branch.

Hmm.

  • How does cargo use 0.9.3 if that version isn't on crates.io? (Oh, I see, rustfix is a crate within cargo.)
  • I can understand how cargo might be using a version of rustfix that is one version ahead of what's on crates.io. But two versions? Why isn't 0.9.2 on crates.io? (Pre-existing problem, mostly orthogonal to this PR.)
  • Is it worth updating to 0.9.1 in this PR? Good to get on 0.9 to get closer to Cargo's version, even if it doesn't match exactly?

r=me for the PR as is, because it's a clear improvement, but I'll be happy if you fix the whitespace issue I mentioned above and also got onto 0.9.1 (assuming it doesn't cause extra complications).

@bors delegate=nnethercote

@bors
Copy link
Collaborator

bors commented Sep 15, 2025

✌️ @nnethercote, you can now approve this pull request!

If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote

@jyn514
Copy link
Member Author

jyn514 commented Sep 16, 2025

I'm going to merge this as-is just to minimize conflicts for @clubby789 in #145849, then bump to 0.9.1 in a separate PR.

@bors r=nnethercote

I'll give you a gold star if you also add the missing space before the {!

I don't have time to commit to this right now. I also kinda feel that this should be rustfmt's job, not rustc's, doing it in this case doesn't seem too bad but getting it right in the general case seems quite hard and I'm not sure it makes sense to spend too much time on it.

@bors
Copy link
Collaborator

bors commented Sep 16, 2025

@jyn514: 🔑 Insufficient privileges: Not in reviewers

@jyn514
Copy link
Member Author

jyn514 commented Sep 16, 2025

@bors r=@nnethercote

@bors
Copy link
Collaborator

bors commented Sep 16, 2025

@jyn514: 🔑 Insufficient privileges: Not in reviewers

@jyn514
Copy link
Member Author

jyn514 commented Sep 16, 2025

oh, i think you needed to say delegate=jyn514 for it to work.

@samueltardieu
Copy link
Member

@bors r=nnethercote

@bors
Copy link
Collaborator

bors commented Sep 16, 2025

📌 Commit 2adaa5d has been approved by nnethercote

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 Sep 16, 2025
@ehuss
Copy link
Contributor

ehuss commented Sep 16, 2025

cc @weihanglo just FYI.

I don't really understand why the weekly updates weren't updating rustfix. We were depending on that to avoid them getting too out of sync. Was it possibly due to a pinned dependency?

@jyn514
Copy link
Member Author

jyn514 commented Sep 16, 2025

I don't really understand why the #145189 weren't updating rustfix. We were depending on that to avoid them getting too out of sync. Was it possibly due to a pinned dependency?

Yes, tracing was pinned to an old version, and was just unpinned: #146188

bors added a commit that referenced this pull request Sep 16, 2025
Rollup of 5 pull requests

Successful merges:

 - #146442 (Display ?Sized, const, and lifetime parameters in trait item suggestions across a crate boundary)
 - #146474 (Improve `core::ascii` coverage)
 - #146605 (Bump rustfix 0.8.1 -> 0.8.7)
 - #146611 (bootstrap: emit hint if a config key is used in the wrong section)
 - #146618 (Do not run ui test if options specific to LLVM are used when another codegen backend is used)

r? `@ghost`
`@rustbot` modify labels: rollup
@nnethercote
Copy link
Contributor

Apologies for messing up the delegate syntax.

I'll give you a gold star if you also add the missing space before the {!

I don't have time to commit to this right now. I also kinda feel that this should be rustfmt's job, not rustc's, doing it in this case doesn't seem too bad but getting it right in the general case seems quite hard and I'm not sure it makes sense to spend too much time on it.

The problem is simply that tests/ui/suggestions/missing-bound-in-derive-copy-impl-3.rs is misformatted, and we don't run rustfmt on tests. So it would be a single character change. But it's not important and the other improvements in this PR are good and I see you want it merged quickly for #145849, so I will give you a gold stat anyway: ⭐

@bors bors merged commit b1a7246 into rust-lang:master Sep 17, 2025
10 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Sep 17, 2025
rust-timer added a commit that referenced this pull request Sep 17, 2025
Rollup merge of #146605 - jyn514:update-rustfix, r=nnethercote

Bump rustfix 0.8.1 -> 0.8.7

This commit can be replicated by running `cargo update -p rustfix --precise 0.8.7 && x test ui --bless`.

---

The reasons this affects UI tests is as follows:
- The UI test suite runs rustc with `-Z deduplicate-diagnostics=no --error-format=json`, which means that rustc emits multiple errors containing identical suggestions. That caused the weird-looking code that had multiple `X: Copy` suggestions.
- Those suggestions are interpreted not by rustc itself, but by the `rustfix` library, maintained by cargo but published as a separate crates.io library and used by compiletest.
- Sometime between rustfix 0.8.1 and 0.8.7 (probably in rust-lang/cargo#14747, but it's hard to tell because rustfix's versioning doesn't match cargo's), rustfix got smarter and stopped applying duplicate suggestions.

Update rustfix to match cargo's behavior. Ideally, we would always share a version of rustfix between cargo and rustc (perhaps with a path dependency?), to make sure we are testing the behavior we ship. But for now, just manually update it to match.

Note that the latest version of rustfix published to crates.io is 0.9.1, not 0.8.7. But 0.9.1 is not the version used in cargo, which is 0.9.3. Rather than trying to match versions exactly, I just updated rustfix to the latest in the 0.8 branch.
@jyn514 jyn514 deleted the update-rustfix branch September 24, 2025 14:40
@jyn514
Copy link
Member Author

jyn514 commented Sep 24, 2025

Is it worth updating to 0.9.1 in this PR? Good to get on 0.9 to get closer to Cargo's version, even if it doesn't match exactly?

FYI, I looked into this, and it's not trivial because clippy also depends on rustfix. So it involves:

  • bumping rustfix in ui-test
  • bumping ui-test in clippy
  • bumping rustfix in compiletest

in that order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants