Adding helper to report_unused_parameter by jkvargas · Pull Request #108252 · rust-lang/rust

jkvargas · 2023-02-20T02:30:08Z

Fixes issue: #107295

rustbot · 2023-02-20T02:30:16Z

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

Please see the contribution instructions for more information.

Noratrieb · 2023-02-20T06:58:57Z

compiler/rustc_hir_analysis/src/impl_wf_check.rs

+    err.span_suggestion(
+        span,
+        format!(
+            "Either remove the type parameter '{}' or make use of it, for example ` for S<{}>`.",


"Make use of it" is a little misleading, the user might already use it in a where clause. I'm not sure what the best wording for this would be.

maybe just removing the "make" and leave it Either remove the type parameter or use it?
I am not sure on the words as well... =(

yea that sounds better

Noratrieb · 2023-02-20T07:00:51Z

compiler/rustc_hir_analysis/src/impl_wf_check.rs

+            "Either remove the type parameter '{}' or make use of it, for example ` for S<{}>`.",
+            name, name
+        ),
+        "",


I don't think it's a good idea to give a suggestion to remove it. Simply removing the parameter is usually not the solution to this error in my experience.

I based myself on the "desired output" on #107295
What would be a good explanation? :)

oli-obk · 2023-02-22T08:53:28Z

r? @oli-obk

oli-obk

I don't think we have enough information to actually report a good message here. We'll need to add more information to the report_unused_parameter function, but first we need to figure out what the ideal output even is. It may be better to

only add another note in this PR
in a follow up PR: turn this into a structured error (https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-structs.html)
add more information (type, trait, full span for generic param) and report more targeted errors

the last step is very handwavy on purpose, as turning this diagnostic into a really good one is hard, so it'll need a lot of experimentation, ideally initially restricted to just a few cases, possibly avoiding a removal suggestion entirely

oli-obk · 2023-02-22T11:06:43Z

tests/ui/const-generics/issues/issue-68366.full.stderr

+help: Either remove the type parameter 'N' or make use of it, for example ` for S<N>`.
+   |
+LL - impl <const N: usize> Collatz<{Some(N)}> {}
+LL + impl <> Collatz<{Some(N)}> {}
+   |


in this case the suggestion is wrong, removing it is never the right thing if it is used, but in a way that isn't legal.

since we know it in this case (we are emitting the two notes above after all), you can re-use the condition for that and not emit the suggestion in that case

oli-obk · 2023-02-22T11:07:56Z

tests/ui/duplicate/duplicate-type-parameter.stderr

+help: Either remove the type parameter 'T' or make use of it, for example ` for S<T>`.
+   |
+LL - impl<T,T> Qux<T,T> for Option<T> {}
+LL + impl<T,> Qux<T,T> for Option<T> {}


stray comma in the suggestion

oli-obk · 2023-02-22T11:08:42Z

tests/ui/error-codes/E0207.stderr

+help: Either remove the type parameter 'T' or make use of it, for example ` for S<T>`.
+   |
+LL - impl<T: Default> Foo {
+LL + impl<: Default> Foo {


Need to remove the bounds, too

oli-obk · 2023-02-22T11:08:58Z

tests/ui/const-generics/issues/issue-68366.min.stderr

+help: Either remove the type parameter 'N' or make use of it, for example ` for S<N>`.
+   |
+LL - impl<const N: usize> Foo {}
+LL + impl<> Foo {}


should remove the generic parameters entirely instead of leaving empty ones around

oli-obk · 2023-02-22T11:09:13Z

tests/ui/generic-associated-types/bugs/issue-88526.stderr

+help: Either remove the type parameter 'I' or make use of it, for example ` for S<I>`.
+   |
+LL - impl<'q, Q, I, F> A for TestB<Q, F>
+LL + impl<'q, Q, , F> A for TestB<Q, F>


stray comma

oli-obk · 2023-02-22T11:10:19Z

compiler/rustc_hir_analysis/src/impl_wf_check.rs

+    err.span_suggestion(
+        span,
+        format!(
+            "Either remove the type parameter '{}' or make use of it, for example ` for S<{}>`.",


yea that sounds better

oli-obk · 2023-02-22T11:10:29Z

compiler/rustc_hir_analysis/src/impl_wf_check.rs

+    err.span_suggestion(
+        span,
+        format!(
+            "Either remove the type parameter '{}' or make use of it, for example ` for S<{}>`.",


nit: our messages start lowercase

oli-obk · 2023-02-22T11:11:34Z

compiler/rustc_hir_analysis/src/impl_wf_check.rs

+    err.span_suggestion(
+        span,
+        format!(
+            "Either remove the type parameter '{}' or make use of it, for example ` for S<{}>`.",


we should not just suggest S<T>. Either load the trait or type name and append the generics to that or write prosa explaining what's going on without naming the trait or type.

apiraino · 2023-03-29T12:49:25Z

I think think can switch to waiting on author to incorporate changes and design suggestions. Feel free to request a review with @rustbot ready, thanks!

@rustbot author

Dylan-DPC · 2023-05-20T06:33:09Z

@jkvargas any updates on this?

jkvargas · 2023-05-23T05:04:42Z

@jkvargas any updates on this?

Hi,
I have been passing through some personal issues and I couldn't sit down yet to solve this yet.

bors · 2023-07-27T07:51:20Z

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

Dylan-DPC · 2023-08-06T14:56:43Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

Adding helper to report_unused_parameter

cc811d7

rustbot assigned nagisa Feb 20, 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. labels Feb 20, 2023

This comment has been minimized.

Sign in to view

fixing tests

c76da7f

Noratrieb reviewed Feb 20, 2023

View reviewed changes

jkvargas requested a review from Noratrieb February 20, 2023 08:06

rustbot assigned oli-obk and unassigned nagisa Feb 22, 2023

oli-obk requested changes Feb 22, 2023

View reviewed changes

Merge branch 'master' into 107295

446e330

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 29, 2023

Dylan-DPC closed this Aug 6, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 6, 2023

Lol3rrr mentioned this pull request Aug 26, 2023

Add more information to error message for unconstrained_parameter E0207 #115245

Closed

Uh oh!

Conversation

jkvargas commented Feb 20, 2023

Uh oh!

rustbot commented Feb 20, 2023

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Feb 22, 2023

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apiraino commented Mar 29, 2023

Uh oh!

Dylan-DPC commented May 20, 2023

Uh oh!

jkvargas commented May 23, 2023

Uh oh!

bors commented Jul 27, 2023

Uh oh!

Dylan-DPC commented Aug 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants