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.

Sign up for GitHub

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

Jump to bottom

fix: clarify suggestion that &T must refer to T: Sync for &T: Send #87322

Merged
merged 2 commits into from
Jul 23, 2021
Merged

fix: clarify suggestion that &T must refer to T: Sync for &T: Send #87322

merged 2 commits into from
Jul 23, 2021

Conversation

chazkiker2
Copy link
Contributor

@chazkiker2 chazkiker2 commented Jul 20, 2021 •
edited
Loading

Description

  • fix suggestion that &T requires Sync to be Send could be clearer #86507
  • add UI test for relevant code from issue
  • change rustc_trait_selection/src/traits/error_reporting/suggestions.rs to include a more clear suggestion when &T fails to satisfy Send bounds due to the fact that T fails to implement Sync
  • update UI test in Clippy: src/tools/tests/ui/future_not_send.stderr

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@chazkiker2 chazkiker2 changed the title fix: suggestion ref sync send [WIP] fix: clarify suggestion that &T must refer to T: Send + Sync for &T: Send Jul 20, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@chazkiker2 chazkiker2 changed the title [WIP] fix: clarify suggestion that &T must refer to T: Send + Sync for &T: Send [WIP] fix: clarify suggestion that &T must refer to T: Sync for &T: Send Jul 21, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@chazkiker2 chazkiker2 changed the title [WIP] fix: clarify suggestion that &T must refer to T: Sync for &T: Send fix: clarify suggestion that &T must refer to T: Sync for &T: Send Jul 22, 2021
@chazkiker2 chazkiker2 marked this pull request as ready for review July 22, 2021 03:14
estebank
estebank reviewed Jul 22, 2021
View reviewed changes
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Left some small action items. Could you also squash your commits before we merge this?

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
@estebank
Copy link
Contributor

r? @estebank

@chazkiker2
Squash all commits.
831ac19 
add test for issue 86507

add stderr for issue 86507

update issue-86507 UI test

add comment for the expected error in UI test file

add proper 'refers to <ref_type>' in suggestion

update diagnostic phrasing; update test to match new phrasing; re-organize logic for checking T: Sync

evaluate additional obligation to figure out if T is Sync

run './x.py test tidy --bless'

incorporate changes from review; reorganize logic for readability
@chazkiker2 chazkiker2 force-pushed the fix/suggestion-ref-sync-send branch from 04d20ef to 831ac19 Compare July 22, 2021 20:45
@chazkiker2 chazkiker2 requested a review from estebank July 22, 2021 20:49
@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 22, 2021

📌 Commit 831ac19 has been approved by estebank

@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 Jul 22, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 23, 2021
@JohnTitor
Rollup merge of rust-lang#87322 - chazkiker2:fix/suggestion-ref-sync-…
d089e27 
…send, r=estebank

fix: clarify suggestion that `&T` must refer to `T: Sync` for `&T: Send`

### Description

- [x] fix rust-lang#86507
- [x] add UI test for relevant code from issue
- [x] change `rustc_trait_selection/src/traits/error_reporting/suggestions.rs` to include a more clear suggestion when `&T` fails to satisfy `Send` bounds due to the fact that `T` fails to implement `Sync`
@JohnTitor JohnTitor mentioned this pull request Jul 23, 2021
Closed
@JohnTitor
Copy link
Member

Failed on rollup: #87399 (comment)
@bors r-

Needs to update Clippy.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 23, 2021
@chazkiker2
Copy link
Contributor Author

chazkiker2 commented Jul 23, 2021 •
edited
Loading

@chazkiker2 chazkiker2 force-pushed the fix/suggestion-ref-sync-send branch from c9d83c2 to a1518f0 Compare July 23, 2021 17:56
@JohnTitor
Copy link
Member

Thanks!
@bors r=estebank

@bors
Copy link
Contributor

bors commented Jul 23, 2021

📌 Commit a1518f0 has been approved by estebank

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 23, 2021
@JohnTitor JohnTitor mentioned this pull request Jul 23, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 23, 2021
@bors
Auto merge of rust-lang#87413 - JohnTitor:rollup-dht22jk, r=JohnTitor
67b0300 
Rollup of 14 pull requests

Successful merges:

 - rust-lang#86410 (VecMap::get_value_matching should return just one element)
 - rust-lang#86790 (Document iteration order of `retain` functions)
 - rust-lang#87171 (Remove Option from BufWriter)
 - rust-lang#87175 (Stabilize `into_parts()` and `into_error()`)
 - rust-lang#87185 (Fix panics on Windows when the build was cancelled)
 - rust-lang#87191 (Package LLVM libs for the target rather than the build host)
 - rust-lang#87255 (better support for running libcore tests with Miri)
 - rust-lang#87266 (Add testcase for 87076)
 - rust-lang#87283 (Add `--codegen-backends=foo,bar` configure flag)
 - rust-lang#87322 (fix: clarify suggestion that `&T` must refer to `T: Sync` for `&T: Send`)
 - rust-lang#87358 (Fix `--dry-run` when download-ci-llvm is set)
 - rust-lang#87380 (Don't default to `submodules = true` unless the rust repo has a .git directory)
 - rust-lang#87398 (Add test for fonts used for module items)
 - rust-lang#87412 (Add missing article)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3fc79fd into rust-lang:master Jul 23, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 23, 2021
@chazkiker2 chazkiker2 deleted the fix/suggestion-ref-sync-send branch October 14, 2021 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank Awaiting requested review from estebank

Assignees

@estebank estebank

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.

suggestion that &T requires Sync to be Send could be clearer
8 participants
@chazkiker2 @rust-highfive @rust-log-analyzer @estebank @bors @JohnTitor @Mark-Simulacrum @rustbot