Suggest `impl Trait` for References to Bare Trait in Function Header #127692

veera-sivarajan · 2024-07-13T17:18:34Z

This PR suggests impl Trait when &Trait is found as a function parameter type or return type. This makes use of existing diagnostics by adding peel_refs() when checking for type equality.

Additionaly, it makes a few other improvements:

Checks if functions inside impl blocks have bare trait in their headers.
Introduces a trait NextLifetimeParamName similar to the existing NextTypeParamName for suggesting a lifetime name. Also, abstracts out the common logic between the two trait impls.

Related Issues

I ran into a bunch of related diagnostic issues but couldn't fix them within the scope of this PR. So, I have created the following issues:

r? @estebank since you implemented #119148

rustbot · 2024-07-13T17:18:42Z

r? @oli-obk

rustbot has assigned @oli-obk.
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 · 2024-07-13T17:18:44Z

HIR ty lowering was modified

cc @fmease

compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs

veera-sivarajan · 2024-07-15T00:56:58Z

r? @estebank

compiler/rustc_hir/src/hir.rs

oli-obk · 2024-07-15T10:02:59Z

we really should move these diagnostics to using the translatable diagnostics infrastructure and ideally to structured diagnostics, too. Preexisting, but adding more complexity to these diagnostics will make that work harder in the future

veera-sivarajan · 2024-07-17T22:45:48Z

Ah yeah, I tried to move everything to structured diagnostics but it feels quite complicated. Should it be a separate PR or included in this PR itself?

oli-obk · 2024-07-18T06:31:07Z

Oh no, definitely a separate PR. Just saying it will be harder, so not a blocker, but my personal preference would be to first move to structured diagnostics before adding more to it. I'll let esteban handle this, as I'll be out for a few months soon

compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs

compiler/rustc_hir/src/hir.rs

compiler-errors · 2024-08-16T13:16:12Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs

+                generics,
+                owner_id,
+                ..
+            }) => (sig, generics, Some(tcx.parent(owner_id.to_def_id()))),


This isn't wrong, but I'm not sure why this is necessary to add for these changes. Can you explain what's being changed by adding this arm?

Added this for consistency so that we show the same suggestions in E0782 for function items and function inside trait blocks and impl blocks.

I'm asking in what specific case you discovered this? It's really hard to determine what changes affect which diagnostics when it's all together like this.

Oh, E0782 suggests impl Trait when Trait is found as function parameter or return type. I noticed that this suggestion is not being made for functions inside impl block and decided to add this arm for consistency. The original author probably forgot to add it.

It does not add anything to the diagnostic. It just ensures that the existing diagnostics apply to functions inside impl blocks as well.

compiler-errors · 2024-08-16T13:18:26Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs

@@ -201,14 +220,54 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
                "{pre}use `impl {trait_name}` to return an opaque type, as long as you return a \
                 single underlying type",
            );
-            diag.multipart_suggestion_verbose(msg, impl_sugg, Applicability::MachineApplicable);
+
+            // Six different cases to consider when suggesting `impl Trait` for a return type:


These special cases seem kind of orthogonal... Why is the mutability of the reference important here for special casing suggestions?

Are you sure that this handles elided lifetimes correctly? I'd rather we stop special casing an elided lifetime at all, and keep this suggestion simple even if it's slightly wrong.

I special cased &mut because the current suggestion for fn code() -> &mut Trait looks like:

7 | fn code() -> &'static mut Trait { | +++++++ 7 | fn code() -> &mut impl Trait { | ++++

(Note: The 'static lifetime suggestion is from E0106 and it's complicated to fix that because it happens in rustc_resolve.)

I thought the current suggestion might be very confusing for someone new to Rust than this proposed suggestion:

error[E0782]: cannot return a mutable reference to a bare trait --> $DIR/reference-to-bare-trait-in-fn-inputs-and-outputs-issue-125139.rs:44:25 | LL | fn parrot() -> &mut Trait { | ^^^^^ | help: use `impl Trait` to return an opaque type, as long as you return a single underlying type | LL | fn parrot() -> impl Trait { | ~~~~~~~~~~

But I agree that special casing for lifetime is more complicated than necessary. Maybe I can keep the case 6 branch and remove the cases 4 and 5 branch?

I don't think mutability has anything to do with this special case being confusing. Can't I just create the same exact case with an immutable reference?

Ah yeah, it would be the same for immutable reference as well.

I have removed all special casing for return type.

compiler-errors · 2024-08-26T21:58:41Z

sorry, I really haven't had the time to review this :(

r? diagnostics

estebank · 2024-09-02T16:27:32Z

@bors r+

bors · 2024-09-02T16:27:35Z

📌 Commit 12de141 has been approved by estebank

It is now in the queue for this repository.

…stebank Suggest `impl Trait` for References to Bare Trait in Function Header Fixes rust-lang#125139 This PR suggests `impl Trait` when `&Trait` is found as a function parameter type or return type. This makes use of existing diagnostics by adding `peel_refs()` when checking for type equality. Additionaly, it makes a few other improvements: 1. Checks if functions inside impl blocks have bare trait in their headers. 2. Introduces a trait `NextLifetimeParamName` similar to the existing `NextTypeParamName` for suggesting a lifetime name. Also, abstracts out the common logic between the two trait impls. ### Related Issues I ran into a bunch of related diagnostic issues but couldn't fix them within the scope of this PR. So, I have created the following issues: 1. [Misleading Suggestion when Returning a Reference to a Bare Trait from a Function](rust-lang#127689) 2. [Verbose Error When a Function Takes a Bare Trait as Parameter](rust-lang#127690) 3. [Incorrect Suggestion when Returning a Bare Trait from a Function](rust-lang#127691) r? `@estebank` since you implemented rust-lang#119148

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#101339 (enable -Zrandomize-layout in debug CI builds ) - rust-lang#127021 (Add target support for RTEMS Arm) - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header) - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks ) - rust-lang#128871 (bypass linker configuration and cross target check for specific commands) - rust-lang#129624 (Adjust `memchr` pinning and run `cargo update`) - rust-lang#129706 (Rename dump of coroutine by-move-body to be more consistent, fix ICE in dump_mir) - rust-lang#129789 (rustdoc: use strategic boxing to shrink `clean::Item`) r? `@ghost` `@rustbot` modify labels: rollup

…stebank Suggest `impl Trait` for References to Bare Trait in Function Header Fixes rust-lang#125139 This PR suggests `impl Trait` when `&Trait` is found as a function parameter type or return type. This makes use of existing diagnostics by adding `peel_refs()` when checking for type equality. Additionaly, it makes a few other improvements: 1. Checks if functions inside impl blocks have bare trait in their headers. 2. Introduces a trait `NextLifetimeParamName` similar to the existing `NextTypeParamName` for suggesting a lifetime name. Also, abstracts out the common logic between the two trait impls. ### Related Issues I ran into a bunch of related diagnostic issues but couldn't fix them within the scope of this PR. So, I have created the following issues: 1. [Misleading Suggestion when Returning a Reference to a Bare Trait from a Function](rust-lang#127689) 2. [Verbose Error When a Function Takes a Bare Trait as Parameter](rust-lang#127690) 3. [Incorrect Suggestion when Returning a Bare Trait from a Function](rust-lang#127691) r? ``@estebank`` since you implemented rust-lang#119148

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header) - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks ) - rust-lang#128934 (Non-exhaustive structs may be empty) - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`) - rust-lang#129706 (Rename dump of coroutine by-move-body to be more consistent, fix ICE in dump_mir) - rust-lang#129896 (do not attempt to prove unknowable goals) - rust-lang#129926 (Move `SanityCheck` and `MirPass`) - rust-lang#129928 (rustc_driver_impl: remove some old dead logic) - rust-lang#129930 (include 1.80.1 release notes on master) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header) - rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks ) - rust-lang#128934 (Non-exhaustive structs may be empty) - rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`) - rust-lang#129863 (update comment regarding TargetOptions.features) - rust-lang#129896 (do not attempt to prove unknowable goals) - rust-lang#129926 (Move `SanityCheck` and `MirPass`) - rust-lang#129928 (rustc_driver_impl: remove some old dead logic) - rust-lang#129930 (include 1.80.1 release notes on master) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#127692 - veera-sivarajan:bugfix-125139, r=estebank Suggest `impl Trait` for References to Bare Trait in Function Header Fixes rust-lang#125139 This PR suggests `impl Trait` when `&Trait` is found as a function parameter type or return type. This makes use of existing diagnostics by adding `peel_refs()` when checking for type equality. Additionaly, it makes a few other improvements: 1. Checks if functions inside impl blocks have bare trait in their headers. 2. Introduces a trait `NextLifetimeParamName` similar to the existing `NextTypeParamName` for suggesting a lifetime name. Also, abstracts out the common logic between the two trait impls. ### Related Issues I ran into a bunch of related diagnostic issues but couldn't fix them within the scope of this PR. So, I have created the following issues: 1. [Misleading Suggestion when Returning a Reference to a Bare Trait from a Function](rust-lang#127689) 2. [Verbose Error When a Function Takes a Bare Trait as Parameter](rust-lang#127690) 3. [Incorrect Suggestion when Returning a Bare Trait from a Function](rust-lang#127691) r? ```@estebank``` since you implemented rust-lang#119148

Update Tests

102997a

rustbot assigned oli-obk Jul 13, 2024

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 Jul 13, 2024

veera-sivarajan mentioned this pull request Jul 13, 2024

Misleading Suggestion when Returning a Reference to a Bare Trait from a Function #127689

Closed

veera-sivarajan commented Jul 13, 2024

View reviewed changes

compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs Show resolved Hide resolved

rustbot assigned estebank and unassigned oli-obk Jul 15, 2024

oli-obk reviewed Jul 15, 2024

View reviewed changes

compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved

veera-sivarajan force-pushed the bugfix-125139 branch from 15499b1 to 57b79f7 Compare July 17, 2024 22:44

This comment was marked as resolved.

Sign in to view

compiler-errors requested changes Aug 16, 2024

View reviewed changes

compiler-errors assigned compiler-errors and unassigned estebank Aug 16, 2024

veera-sivarajan force-pushed the bugfix-125139 branch 2 times, most recently from e845028 to 0021213 Compare August 19, 2024 19:11

This comment has been minimized.

Sign in to view

Suggest impl Trait for References to Bare Trait in Function Header

12de141

veera-sivarajan force-pushed the bugfix-125139 branch from 0021213 to 12de141 Compare August 19, 2024 19:19

rustbot assigned estebank and unassigned compiler-errors Aug 26, 2024

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 Sep 2, 2024

matthiaskrgr mentioned this pull request Sep 2, 2024

Rollup of 8 pull requests #129900

Closed

matthiaskrgr mentioned this pull request Sep 3, 2024

Rollup of 9 pull requests #129933

Closed

matthiaskrgr mentioned this pull request Sep 3, 2024

Rollup of 9 pull requests #129936

Merged

bors merged commit f75a195 into rust-lang:master Sep 5, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Sep 5, 2024

veera-sivarajan deleted the bugfix-125139 branch September 5, 2024 01:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest `impl Trait` for References to Bare Trait in Function Header #127692

Suggest `impl Trait` for References to Bare Trait in Function Header #127692

veera-sivarajan commented Jul 13, 2024 •

edited

Loading

rustbot commented Jul 13, 2024

rustbot commented Jul 13, 2024

veera-sivarajan commented Jul 15, 2024

oli-obk commented Jul 15, 2024

veera-sivarajan commented Jul 17, 2024

oli-obk commented Jul 18, 2024

This comment was marked as resolved.

compiler-errors Aug 16, 2024

veera-sivarajan Aug 19, 2024

compiler-errors Aug 19, 2024

veera-sivarajan Aug 19, 2024

compiler-errors Aug 16, 2024

veera-sivarajan Aug 19, 2024

compiler-errors Aug 19, 2024

veera-sivarajan Aug 19, 2024 •

edited

Loading

This comment has been minimized.

compiler-errors commented Aug 26, 2024

estebank commented Sep 2, 2024

bors commented Sep 2, 2024

Suggest impl Trait for References to Bare Trait in Function Header #127692

Suggest impl Trait for References to Bare Trait in Function Header #127692

Conversation

veera-sivarajan commented Jul 13, 2024 • edited Loading

Related Issues

rustbot commented Jul 13, 2024

rustbot commented Jul 13, 2024

veera-sivarajan commented Jul 15, 2024

oli-obk commented Jul 15, 2024

veera-sivarajan commented Jul 17, 2024

oli-obk commented Jul 18, 2024

This comment was marked as resolved.

compiler-errors Aug 16, 2024

Choose a reason for hiding this comment

veera-sivarajan Aug 19, 2024

Choose a reason for hiding this comment

compiler-errors Aug 19, 2024

Choose a reason for hiding this comment

veera-sivarajan Aug 19, 2024

Choose a reason for hiding this comment

compiler-errors Aug 16, 2024

Choose a reason for hiding this comment

veera-sivarajan Aug 19, 2024

Choose a reason for hiding this comment

compiler-errors Aug 19, 2024

Choose a reason for hiding this comment

veera-sivarajan Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

compiler-errors commented Aug 26, 2024

estebank commented Sep 2, 2024

bors commented Sep 2, 2024

Suggest `impl Trait` for References to Bare Trait in Function Header #127692

Suggest `impl Trait` for References to Bare Trait in Function Header #127692

veera-sivarajan commented Jul 13, 2024 •

edited

Loading

veera-sivarajan Aug 19, 2024 •

edited

Loading