-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Suggest using Arc
on !Send
/!Sync
types
#88936
Conversation
r? @yaahc (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
LL | | // Empty | ||
LL | | } | ||
| |_^ required by this bound in `Sync` | ||
= note: consider using `std::sync::Arc<<<Self as Case1>::C as Iterator>::Item>`; for more information visit <https://doc.rust-lang.org/book/ch16-03-shared-state.html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now seems to suggest to use Arc<..::Item>
instead of ..::Item
. It might be useful to say consider using Arc instead of Rc: ...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But That's exactly what this is suggesting: we didn't have an Rc
here. Ideally we should be able to tell what context this error is being emitted on, but sadly rustc_on_unimplemented
doesn't have that filtering today.
r? rust-lang/compiler |
library/core/src/marker.rs
Outdated
@@ -32,8 +32,11 @@ use crate::hash::Hasher; | |||
#[stable(feature = "rust1", since = "1.0.0")] | |||
#[cfg_attr(not(test), rustc_diagnostic_item = "send_trait")] | |||
#[rustc_on_unimplemented( | |||
on(_Self = "std::rc::Rc<T>", note = "use `std::sync::Arc` instead"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of Arc
is not always applicable, as both Send
and Sync
impls for Arc<T>
require T: Send + Sync
. Unless we can verify T
bounds here, I don't think this on
clause adds any more clarity over the default note
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats fair, but was concerned the default would be too verbose for a case where we could be more straightforward. I can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we can always change this later too. r=me if you don't want to change it.
message = "`{Self}` cannot be sent between threads safely", | ||
label = "`{Self}` cannot be sent between threads safely" | ||
label = "`{Self}` cannot be sent between threads safely", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what's the effect of this label
clause here? I was thinking that this text would appear near the span underline, but tests indicate that nothing really changed in that particular aspect, at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the primary span text.
@bors r+ |
📌 Commit e2cffd73c83c54c629ed88b30bb7f8a486d0b1ec has been approved by |
⌛ Testing commit e2cffd73c83c54c629ed88b30bb7f8a486d0b1ec with merge e5fb2513a52b92052d5e17ff7414a5e291ba205d... |
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors r=nagisa |
📌 Commit afd717c52628de0c39329c648e56abecf28b9e8f has been approved by |
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
A bors sync made this re-approved, sorry hehe @bors r- |
Closing this as it is inactive |
Squashed, rebased on top of current main and re-blessed the tests. No other changes since the prior approval. @bors r=nagisa |
@@ -2743,6 +2743,12 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> { | |||
} | |||
ObligationCauseCode::BindingObligation(item_def_id, span) | |||
| ObligationCauseCode::ExprBindingObligation(item_def_id, span, ..) => { | |||
if self.tcx.is_diagnostic_item(sym::Send, item_def_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why isn't this commented? seems like something that could use a justification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a comment.
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
Finished benchmarking commit (27a43f0): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 632.651s -> 632.289s (-0.06%) |
Revert "Suggest using `Arc` on `!Send`/`!Sync` types" Closes rust-lang#114687. This is a clean revert of rust-lang#88936 + rust-lang#115210. The suggestion to Arc\<{Self}\> when Self does not implement Send is *always* wrong. rust-lang#114842 is considering a way to make a more refined suggestion.
No description provided.