Skip to content

Wrap dyn type with parentheses in suggestion #120929

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 23, 2024
Merged

Wrap dyn type with parentheses in suggestion #120929

merged 1 commit into from
Apr 23, 2024

Conversation

long-long-float
Copy link
Contributor

Close #120223

Fix wrong suggestion that is grammatically incorrect.
Specifically, I added parentheses to dyn types that need lifetime bound.

help: consider adding an explicit lifetime bound
  |
4 |     executor: impl FnOnce(T) -> (dyn Future<Output = ()>) + 'static,
  |                                 +                       +++++++++++

@rustbot
Copy link
Collaborator

rustbot commented Feb 11, 2024

r? @fmease

rustbot has assigned @fmease.
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 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. labels Feb 11, 2024
@rust-log-analyzer

This comment has been minimized.

# to view
@rust-log-analyzer

This comment has been minimized.

# to view
@long-long-float
Copy link
Contributor Author

@fmease Could you review this PR? Thank you.

@fmease
Copy link
Member

fmease commented Feb 18, 2024 •
edited
Loading

Sure, I plan on reviewing this later today. Sorry for taking a bit, I'm assigned to several PRs and I continuously work on PRs myself ^^'

@bors
Copy link
Collaborator

bors commented Feb 24, 2024

☔ The latest upstream changes (presumably #121549) made this pull request unmergeable. Please resolve the merge conflicts.

fmease
fmease requested changes Mar 6, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry for leaving you hanging like that! I have a couple of suggestions for how to structure this a bit differently and some correctness things.

At the moment your changes only trigger in very few cases. I think we should generalize it to apply to more + XXX-style suggestions.

First and foremost, I would check the other 3 usage sites bounds_span_for_suggestion whether they could benefit from this, too (I even suggest you to merge bounds_span_for_suggestion_with_parentheses into it in one of my review comments). You could even consider grep'ing through tests/ui/ to check if there are any other incorrect consider adding an explicit lifetime bound suggestions.

In summary, I'm a fan of fixing the incorrect diagnostic suggestion but I'd like to make sure to tap the full potential.

@rustbot rustbot 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 Mar 6, 2024
@long-long-float
Copy link
Contributor Author

@fmease Thank you for detailed review! I will fix them and check for more patterns in a couple of days.

@long-long-float long-long-float force-pushed the wrap-dyn-in-suggestion branch from 62a7989 to 2b03887 Compare March 17, 2024 06:37
@rust-log-analyzer

This comment has been minimized.

# to view
@long-long-float
Copy link
Contributor Author

I will fix testing failure at first.

@fmease
Copy link
Member

fmease commented Mar 23, 2024

@rustbot review

@rustbot 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 Mar 23, 2024
@long-long-float long-long-float force-pushed the wrap-dyn-in-suggestion branch from 67e91e9 to c1ccb05 Compare March 23, 2024 06:02
@long-long-float
Copy link
Contributor Author

@fmease

First and foremost, I would check the other 3 usage sites bounds_span_for_suggestion whether they could benefit from this, too.

I checked them:

  • compiler/rustc_hir_typeck/src/method/suggest.rs
    • tests/ui/suggestions/impl-trait-with-missing-trait-bounds-in-arg.rs
      • The impl Trait accepts impl Foo + Bar so we don't need parenthesis.
      • I think this suggestion is for type parameters (impl Trait in arguments), not dyn.
  • compiler/rustc_middle/src/ty/diagnostics.rs
    • Same as above?
  • compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs
    • ?Sized cannot be used in trait object types (dyn).

You could even consider grep'ing through tests/ui/ to check if there are any other incorrect consider adding an explicit lifetime bound suggestions.

I searched consider adding an explicit lifetime bound under tests/ui and couldn't find any incorrect suggestions.

I think incorrect suggestions which need parentheses happen with only dyn (I'm not familiar with Rust, so it may incorrect).

fmease
fmease reviewed Mar 30, 2024
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

# to view
fmease
fmease requested changes Apr 19, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

First and foremost, I would check the other 3 usage sites bounds_span_for_suggestion whether they could benefit from this, too.

I could find an example for each usage site. Please make them use open_paren_sp as well, thanks! :)

After that, this PR should be good to go! One more thing, could you squash the commits into one (or fewer) commits?

@rustbot rustbot 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 Apr 19, 2024
@long-long-float long-long-float force-pushed the wrap-dyn-in-suggestion branch from 9e0d856 to f0114a1 Compare April 21, 2024 02:47
@long-long-float
Copy link
Contributor Author

@fmease
Thank you for reviewing! I fixed to use open_paren_sp in all sites, added tests, and squashed commits. Could you check them?

@long-long-float
Copy link
Contributor Author

@rustbot ready

@rustbot 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 Apr 21, 2024
@rust-log-analyzer

This comment has been minimized.

# to view
@fmease
Copy link
Member

fmease commented Apr 21, 2024

#120929 (comment)

CI still has errors, please fix them. If you have any questions regarding the failures, don't hesitate to reach out and ask them!

@fmease fmease 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 Apr 21, 2024
@rust-log-analyzer

This comment has been minimized.

# to view
@long-long-float long-long-float force-pushed the wrap-dyn-in-suggestion branch from 40b99a9 to 31e581e Compare April 22, 2024 15:15
@long-long-float
Copy link
Contributor Author

@fmease Thank you! All tests are passed.

@fmease
Copy link
Member

fmease commented Apr 23, 2024

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 23, 2024

📌 Commit 31e581e has been approved by fmease

It is now in the queue for this repository.

@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 Apr 23, 2024
@fmease fmease mentioned this pull request Apr 23, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
@bors
Auto merge of rust-lang#124295 - fmease:rollup-i3apkc6, r=fmease
c2f2db7 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#120929 (Wrap dyn type with parentheses in suggestion)
 - rust-lang#122591 (Suggest using type args directly instead of equality constraint)
 - rust-lang#122598 (deref patterns: lower deref patterns to MIR)
 - rust-lang#123048 (alloc::Layout: explicitly document size invariant on the type level)
 - rust-lang#123993 (Do `check_coroutine_obligations` once per typeck root)
 - rust-lang#124218 (Allow nesting subdiagnostics in #[derive(Subdiagnostic)])
 - rust-lang#124285 (Mark ``@RUSTC_BUILTIN`` search path usage as unstable)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 80f2b91 into rust-lang:master Apr 23, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
@rust-timer
Unrolled build for rust-lang#120929
888a0ad 
Rollup merge of rust-lang#120929 - long-long-float:wrap-dyn-in-suggestion, r=fmease

Wrap dyn type with parentheses in suggestion

Close rust-lang#120223

Fix wrong suggestion that is grammatically incorrect.
Specifically, I added parentheses to dyn types that need lifetime bound.

```
help: consider adding an explicit lifetime bound
  |
4 |     executor: impl FnOnce(T) -> (dyn Future<Output = ()>) + 'static,
  |                                 +                       +++++++++++
```
@fmease fmease mentioned this pull request Dec 10, 2024
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@fmease fmease fmease approved these changes

Assignees

@fmease fmease

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.

E0310 - Invalid suggestion when type is a function with a dyn return type
5 participants
@long-long-float @rustbot @rust-log-analyzer @fmease @bors