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

Do not suggest using a const parameter when there are bounds on an unused type parameter #93400

Conversation

ChayimFriedman2
Copy link
Contributor

The user wrote the bound, so it's obvious they want a type.

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 28, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 28, 2022
@rust-log-analyzer

This comment has been minimized.

@ChayimFriedman2 ChayimFriedman2 marked this pull request as draft January 28, 2022 00:27
@ChayimFriedman2 ChayimFriedman2 force-pushed the dont-suggest-using-const-with-bounds-unused-generic-param branch from a3e7727 to 7c7b5f5 Compare January 28, 2022 00:40
@ChayimFriedman2
Copy link
Contributor Author

Looks like hir::GenericParam::bounds does not include bounds from where clauses. How can check for them too? I can use GenericPredicates but I don't know how to differentiate implicit bounds from explicit bounds.

@michaelwoerister
Copy link
Member

Thanks for the PR, @ChayimFriedman2! Maybe someone from the diagnostics working group can help you.

r? rust-lang/diagnostics

@davidtwco
Copy link
Member

Looks like hir::GenericParam::bounds does not include bounds from where clauses. How can check for them too? I can use GenericPredicates but I don't know how to differentiate implicit bounds from explicit bounds.

You might be able to use explicit_predicates_of (it's been a while but I think this includes all predicates from the user but none of the implicit ones).

@ChayimFriedman2 ChayimFriedman2 force-pushed the dont-suggest-using-const-with-bounds-unused-generic-param branch 2 times, most recently from 28a5d17 to a1df803 Compare February 13, 2022 06:23
@rust-log-analyzer

This comment has been minimized.

@ChayimFriedman2 ChayimFriedman2 force-pushed the dont-suggest-using-const-with-bounds-unused-generic-param branch 2 times, most recently from b58c6bd to 49c5a8d Compare February 13, 2022 11:08
@ChayimFriedman2
Copy link
Contributor Author

Unfortunately, explicit_bounds_of() includes the implicit : Sized predicate:

<dyn AstConv<'_>>::add_implicitly_sized(
&icx,
&mut bounds,
param.bounds,
Some((param.hir_id, ast_generics.where_clause.predicates)),
param.span,
);

I ended with rather ugly (IMHO) code that lazily lowers the types in the where clause (because there is no easy way to tell if a type is a generic parameter using HIR types). I'm not sure this is a good idea, but I don't have a better one. Do you have a better idea? Or do you think there is no reason to do it lazily?

@ChayimFriedman2 ChayimFriedman2 force-pushed the dont-suggest-using-const-with-bounds-unused-generic-param branch from 49c5a8d to 3dcb72c Compare February 13, 2022 11:16
@rust-log-analyzer

This comment has been minimized.

@ChayimFriedman2 ChayimFriedman2 force-pushed the dont-suggest-using-const-with-bounds-unused-generic-param branch from 3dcb72c to eef7ee8 Compare February 13, 2022 11:34
@rust-log-analyzer

This comment has been minimized.

@ChayimFriedman2 ChayimFriedman2 force-pushed the dont-suggest-using-const-with-bounds-unused-generic-param branch from eef7ee8 to 35db1e7 Compare February 13, 2022 11:57
@ChayimFriedman2 ChayimFriedman2 marked this pull request as ready for review February 13, 2022 22:07
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

I think this looks reasonable but I'm not familiar enough with typeck to know for sure, will reassign.

@davidtwco
Copy link
Member

r? @jackh726

@rust-highfive rust-highfive assigned jackh726 and unassigned davidtwco Feb 16, 2022
@bors
Copy link
Contributor

bors commented Feb 25, 2022

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

@ChayimFriedman2 ChayimFriedman2 force-pushed the dont-suggest-using-const-with-bounds-unused-generic-param branch from 35db1e7 to 003cfc2 Compare February 25, 2022 05:32
@rust-log-analyzer

This comment has been minimized.

…used type parameter

The user wrote the bound, so it's obvious they want a type.
@ChayimFriedman2 ChayimFriedman2 force-pushed the dont-suggest-using-const-with-bounds-unused-generic-param branch from 003cfc2 to 4520045 Compare February 25, 2022 05:44
@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 26, 2022

📌 Commit 4520045 has been approved by jackh726

@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 Feb 26, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 26, 2022
…const-with-bounds-unused-generic-param, r=jackh726

Do not suggest using a const parameter when there are bounds on an unused type parameter

The user wrote the bound, so it's obvious they want a type.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2022
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#93400 (Do not suggest using a const parameter when there are bounds on an unused type parameter)
 - rust-lang#93982 (Provide extra note if synthetic type args are specified)
 - rust-lang#94087 (Remove unused `unsound_ignore_borrow_on_drop`)
 - rust-lang#94235 (chalk: Fix wrong debrujin index in opaque type handling.)
 - rust-lang#94306 (Avoid exhausting stack space in dominator compression)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit be9b99b into rust-lang:master Feb 26, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 26, 2022
@ChayimFriedman2 ChayimFriedman2 deleted the dont-suggest-using-const-with-bounds-unused-generic-param branch February 26, 2022 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

8 participants