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

Allow conditional Send futures in future_not_send #13590

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

y21
Copy link
Member

@y21 y21 commented Oct 23, 2024

Closes #6947

This changes the lint to allow futures which are not Send as a result of a generic type parameter not having a Send bound and only lint futures that are always !Send for any type, which I believe is the more useful behavior (like the comments in the linked issue explain).

This is still only a heuristic (I'm not sure if there's a more general way to do this), but it should cover the common cases I could think of (including the code examples in the linked issue)

changelog: [future_not_send]: allow conditional Send futures

@rustbot
Copy link
Collaborator

rustbot commented Oct 23, 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 23, 2024
@Alexendoo
Copy link
Member

Do we think people will enable this config? I can't come up with a good case it would catch

@y21
Copy link
Member Author

y21 commented Nov 2, 2024

Not sure, probably not. The only thing I could think of is potentially better errors at the definition/call site directly if everything is required to be Send, instead of having the !Sendness propagate to the final future passed to tokio::spawn(), but it's probably not worth it

Copy link
Member Author

Choose a reason for hiding this comment

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

The ICE path isn't really reachable anymore for this test since the function is generic so I'm not sure it's still useful to have, and if I understand the original ICE correctly it would have happened for all the regular tests anyway (anything that led to the .err_ctxt() call)?

@y21
Copy link
Member Author

y21 commented Nov 2, 2024

I left the config option in as a commit for now in case it's still useful but I'll squash it away before the merge

@Alexendoo
Copy link
Member

Looks good! Can r=me after the squash

@y21 y21 changed the title Allow conditional Send futures by default in future_not_send Allow conditional Send futures in future_not_send Nov 10, 2024
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.

Generic async function not_send
3 participants