Create `unnecessary_send_constraint` lint for `&(dyn ... + Send)` #110961

john-h-k · 2023-04-28T19:19:19Z

Does not yet handle `Send` being only constraint (will still recommend removing it)

rustbot · 2023-04-28T19:19:29Z

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

rustbot · 2023-04-28T19:19:32Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

library/core/src/cell.rs

john-h-k · 2023-04-28T19:23:55Z

Also, tests/ui/lint tests seem to have really inconsistent naming - when should I use hyphens vs underscores in file names?

john-h-k · 2023-04-28T19:27:44Z

Also, it appears no lint is generated for duplicate auto traits (like &(dyn Any + Any + Send)). Is there a reason for this? If not I can add that to as the logic is similar ish (in a separate PR)

compiler/rustc_lint/src/useless_send_constraint.rs

Co-authored-by: est31 <est31@users.noreply.github.com>

rfcbot · 2023-06-27T19:06:48Z

🔔 This is now entering its final comment period, as per the review above. 🔔

compiler/rustc_lint/src/unnecessary_send_constraint.rs

RalfJung · 2023-06-27T19:46:02Z

compiler/rustc_lint/src/unnecessary_send_constraint.rs

+    /// References cannot be sent across threads unless they have a `Sync` bound, so constraining them to `Send` without `Sync` is unnecessary.
+    pub UNNECESSARY_SEND_CONSTRAINT,
+    Warn,
+    "constraining a reference to `Send` without `Sync` is unnecessary, consider removing it"


Shouldn't the recommendation be to replace Send by Sync? That seems more likely to be what the author intended.

See this comment: #110961 (comment)

In those situations, it stems from a Box<dyn Any + Send> to which a &dyn was made. It's easy to just copy Any + Send, but the Send part should be removed for shared references.

That seems like a fairly special case to me, I would not expect this to be common enough to warrant a general recommendation of dropping the Send. The more symmetric thing to do would be to have &dyn Any+Sync, to get a shared reference that can still be shared across thread boundaries. Of course then you'd need Box<dyn Any+Send+Sync> for the owned case.

So it should recommend replacing Send with Sync instead? just confirming before making the change

RalfJung · 2023-06-27T19:47:23Z

compiler/rustc_lint/messages.ftl

@@ -192,6 +192,12 @@ lint_redundant_semicolons =
        *[false] this semicolon
    }

+lint_unnecessary_send_constraint = constraining a reference to `Send` is meaningless
+    .suggestion = {$only_trait ->
+        [true] replace this with `std::any::Any`


Replacing by Any is a very strange suggestion. Why would type_id machinery be necessary? Can't the 'static bound cause a problem?

I wasn't sure what else to put, this is for the scenario where Send is the only trait specified (so removing it would be invalid)

compiler/rustc_lint/src/unnecessary_send_constraint.rs

rfcbot · 2023-07-07T19:08:42Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

nikomatsakis · 2023-07-18T18:21:05Z

It looks like we agreed to this. I'm going to drop nomination.

@rustbot labels -I-lang-nominated

Explicitly document how Send and Sync relate to references Some of these relations were already mentioned in the text, but that Send is implemented for &mut impl Send was not mentioned, neither did the docs list when &T is Sync. Inspired by the discussion in #110961. [Proof](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ed77bfc3c77ba664400ebc2734f500e6) based on `@lukas-code` 's [example](rust-lang/rust#110961 (comment)).

nikomatsakis · 2023-07-25T18:41:07Z

@rustbot author

The team role is done here, FCP is complete, we are now waiting on the author to address feedback though.

john-h-k · 2023-08-31T13:19:13Z

@rustbot ready

(still 1 part of the review I am unsure about so have asked for clarification, but other comments have been addressed)

steffahn · 2024-01-16T16:43:20Z

library/std/src/io/error.rs

-    pub fn get_mut(&mut self) -> Option<&mut (dyn error::Error + Send + Sync + 'static)> {
+    pub fn get_mut(&mut self) -> Option<&mut (dyn error::Error + Sync + 'static)> {


This is a mutable reference. For &mut … + Send, the Send makes sense, doesn’t it?

steffahn · 2024-01-16T16:51:29Z

library/std/src/io/error.rs

-    pub fn get_ref(&self) -> Option<&(dyn error::Error + Send + Sync + 'static)> {
+    pub fn get_ref(&self) -> Option<&(dyn error::Error + Sync + 'static)> {


dyn Error + Send + Sync has some API that dyn Error + Sync doesn’t have. Like an is method and a downcast_ref method.

Also, in any case, changing these methods is a breaking change the impact of which I’m not sure has been tested anywhere yet.

that dyn Error + Sync doesn’t have. Like an is method and a downcast_ref method.

dyn Error also implements is and downcast_* with exact the same signature.

dyn Error also implements is and downcast_* with exact the same signature.

Oops, my bad. We cannot use dyn Error's methods on dyn Error + Sync, though it seems odd to me.

Dylan-DPC · 2024-09-24T07:07:31Z

@john-h-k if you can rebase and resolve the conflicts, we can push this for a review

alex-semenyuk · 2024-11-05T19:53:25Z

@john-h-k
From wg-triage. Closed this PR due to inactivity. Feel free to reopen or raised new one. Thanks for your efforts.

john-h-k added 4 commits April 28, 2023 14:48

Initial impl of lint_useless_send_constraint

efbdc5b

Does not yet handle `Send` being only constraint (will still recommend removing it)

Fixes

532368c

Trait test and edge case handling

6ed01d9

fmt

a00fd67

rustbot assigned davidtwco Apr 28, 2023

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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 28, 2023

john-h-k commented Apr 28, 2023

View reviewed changes

library/core/src/cell.rs Outdated Show resolved Hide resolved

WIP tests

30af6f0

This comment has been minimized.

Sign in to view

john-h-k added 3 commits April 28, 2023 20:35

Comment round rust-lang#1

cd7bff1

Comment round rust-lang#1

cb9086b

Remove comment

cef8a0f

This comment has been minimized.

Sign in to view

john-h-k added 3 commits April 28, 2023 20:49

Compiling except for the actual lint

c16b474

Remove comment

ebf545f

Remove uses of send

657ec63

est31 reviewed Apr 29, 2023

View reviewed changes

compiler/rustc_lint/src/useless_send_constraint.rs Outdated Show resolved Hide resolved

john-h-k added 3 commits April 29, 2023 08:41

Comments

2321cb1

Style

b914451

Fix sync behaviour

72adaec

This comment has been minimized.

Sign in to view

john-h-k mentioned this pull request Apr 29, 2023

Add lint_duplicate_trait for duplicate specified traits in a trait object #110991

Closed

est31 reviewed Apr 29, 2023

View reviewed changes

compiler/rustc_lint/src/useless_send_constraint.rs Outdated Show resolved Hide resolved

Update compiler/rustc_lint/src/useless_send_constraint.rs

63b0988

Co-authored-by: est31 <est31@users.noreply.github.com>

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jun 27, 2023

RalfJung reviewed Jun 27, 2023

View reviewed changes

compiler/rustc_lint/src/unnecessary_send_constraint.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jun 27, 2023

View reviewed changes

m-ou-se reviewed Jun 27, 2023

View reviewed changes

compiler/rustc_lint/src/unnecessary_send_constraint.rs Outdated Show resolved Hide resolved

oxalica reviewed Jun 29, 2023

View reviewed changes

compiler/rustc_lint/src/unnecessary_send_constraint.rs Outdated Show resolved Hide resolved

compiler/rustc_lint/src/unnecessary_send_constraint.rs Outdated Show resolved Hide resolved

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 7, 2023

rfcbot added the to-announce Announce this issue on triage meeting label Jul 7, 2023

apiraino removed the to-announce Announce this issue on triage meeting label Jul 13, 2023

rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jul 18, 2023

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 25, 2023

joshtriplett removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Aug 8, 2023

Address review comments

df19546

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 31, 2023

steffahn reviewed Jan 16, 2024

View reviewed changes

Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2024

alex-semenyuk closed this Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `unnecessary_send_constraint` lint for `&(dyn ... + Send)` #110961

Create `unnecessary_send_constraint` lint for `&(dyn ... + Send)` #110961

john-h-k commented Apr 28, 2023

rustbot commented Apr 28, 2023

rustbot commented Apr 28, 2023

This comment has been minimized.

john-h-k commented Apr 28, 2023

john-h-k commented Apr 28, 2023

This comment has been minimized.

This comment has been minimized.

rfcbot commented Jun 27, 2023

RalfJung Jun 27, 2023 •

edited

Loading

m-ou-se Jun 27, 2023

RalfJung Jun 27, 2023

john-h-k Aug 31, 2023

RalfJung Jun 27, 2023

john-h-k Aug 31, 2023

rfcbot commented Jul 7, 2023

nikomatsakis commented Jul 18, 2023

nikomatsakis commented Jul 25, 2023

john-h-k commented Aug 31, 2023

steffahn Jan 16, 2024

steffahn Jan 16, 2024 •

edited

Loading

steffahn Jan 16, 2024

oxalica Jan 22, 2024

oxalica Jan 22, 2024

Dylan-DPC commented Sep 24, 2024

alex-semenyuk commented Nov 5, 2024

		pub fn get_mut(&mut self) -> Option<&mut (dyn error::Error + Send + Sync + 'static)> {
		pub fn get_mut(&mut self) -> Option<&mut (dyn error::Error + Sync + 'static)> {

		pub fn get_ref(&self) -> Option<&(dyn error::Error + Send + Sync + 'static)> {
		pub fn get_ref(&self) -> Option<&(dyn error::Error + Sync + 'static)> {

Create unnecessary_send_constraint lint for &(dyn ... + Send) #110961

Create unnecessary_send_constraint lint for &(dyn ... + Send) #110961

Conversation

john-h-k commented Apr 28, 2023

rustbot commented Apr 28, 2023

rustbot commented Apr 28, 2023

This comment has been minimized.

john-h-k commented Apr 28, 2023

john-h-k commented Apr 28, 2023

This comment has been minimized.

This comment has been minimized.

rfcbot commented Jun 27, 2023

RalfJung Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfcbot commented Jul 7, 2023

nikomatsakis commented Jul 18, 2023

nikomatsakis commented Jul 25, 2023

john-h-k commented Aug 31, 2023

Choose a reason for hiding this comment

steffahn Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dylan-DPC commented Sep 24, 2024

alex-semenyuk commented Nov 5, 2024

Create `unnecessary_send_constraint` lint for `&(dyn ... + Send)` #110961

Create `unnecessary_send_constraint` lint for `&(dyn ... + Send)` #110961

RalfJung Jun 27, 2023 •

edited

Loading

steffahn Jan 16, 2024 •

edited

Loading