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

Show interior mutability chain in mutable_key_type #13496

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

y21
Copy link
Member

@y21 y21 commented Oct 3, 2024

Fixes #10619

Just ran into this myself and I definitely agree it's not very nice to have to manually go through all the types involved to figure out why this happens and to evaluate if this is really a problem (knowing if the field of a struct is something that a hash impl relies on), so this changes the lint to emit notes for each step involved.

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2024

r? @Alexendoo

rustbot has assigned @Alexendoo.
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 rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 3, 2024
span_lint_and_then(cx, MUTABLE_KEY_TYPE, span, "mutable key type", |diag| {
for ty in chain.iter().rev() {
diag.note(with_forced_trimmed_paths!(format!(
"... because it contains `{ty}`, which has interior mutability"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since type names can be quite long sometimes, wouldn't it be best to use instead of ... here? In monospace font the difference would be two characters wide:

... because it contains `UnsafeCell<usize>`, which has interior mutability
… because it contains `UnsafeCell<usize>`, which has interior mutability

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 went with the three dots because I think it's much more common in diagnostics (in fact, I think I've never actually seen a non-ASCII character like that in a diagnostic - rust-lang/rust#126597 adds unicode block drawing but it requires an extra --error-format=human-unicode).

Some rustc examples
Some cargo examples

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a "…" in a diagnostic issued by clippy_lints/src/methods/or_then_unwrap.rs:

clippy_lints/src/methods/or_then_unwrap.rs
25:        title = "found `.or(Some(…)).unwrap()`";
32:        title = "found `.or(Ok(…)).unwrap()`";

@Alexendoo
Copy link
Member

👍

... is fine with me, but if we decide to use we can change all of them at once

@bors r+

@bors
Copy link
Contributor

bors commented Oct 7, 2024

📌 Commit 19d1358 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 7, 2024

⌛ Testing commit 19d1358 with merge b013e69...

@bors
Copy link
Contributor

bors commented Oct 7, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing b013e69 to master...

@bors bors merged commit b013e69 into rust-lang:master Oct 7, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mutable_key_type doesn't mention the type which makes it confusing
5 participants