Tidy up miscellaneous bounds suggestions #97778

compiler-errors · 2022-06-06T04:49:32Z

Just some small fixes to suggestions

Generalizes Ty::is_suggestable into a TypeVisitor, so that it can be called on things other than Ty
Makes impl Trait in arg position no longer suggestible (generalizing the fix in Fix wrong suggestion for adding where clauses #97640)
Fixes impl Trait not being replaced with fresh type param when it's deeply nested in function signature (fixes Rust diagnostic for impl Trait suggests broken code #97760)
Fixes some poor handling of where clauses with no predicates (also Rust diagnostic for impl Trait suggests broken code #97760)
Uses InferCtxt::resolve_numeric_literals_with_default so we suggest i32 instead of {integer} (fixes Give helpful suggestions for code containing {integer} and other non-concrete types #97677)

Sorry there aren't many tests the fixes. Most of them would just be duplicates of other tests with empty where clauses or impl Trait in arg position instead of generic params. Let me know if you'd want more test coverage.

rust-highfive · 2022-06-06T04:49:36Z

r? @cjgillot

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

cjgillot

This is a good improvement in diagnostics.
I have the impression we could get rid of the string comparisons, to avoid nonsense diagnostics if we have multiple impl trait with the same trait.

compiler/rustc_hir/src/hir.rs

compiler/rustc_middle/src/ty/diagnostics.rs

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

compiler/rustc_middle/src/ty/diagnostics.rs

cjgillot · 2022-06-06T07:45:17Z

compiler/rustc_middle/src/ty/diagnostics.rs

+
+            Param(param) => {
+                if let Some(found_bound_str) =
+                    param.name.as_str().strip_prefix("impl ").map(|s| s.trim_start())


Could we rather work on the DefId?
There may be multiple impl Trait with the same string name.

I would like to do this, but I was having trouble associating a hir::GenericParam, ty::Param, DefId... etc. I don't really know how to convert all of them to each other. Is there anywhere I should look to learn?

def_id == tcx.hir().local_def_id(hir_generic_param.hir_id) == tcx.generics_of(item).type_param(ty_param).def_id
Does this help?

Yup, that probably works. I will try to refactor all of the string stuff to instead use the DefId of the generic instead.

Actually -- for the is_suggestable type visitor (which walks a TypeFoldable kind to see if it references any types that aren't suggestable, i.e. don't make sense to emit in a suggestion when pretty-printed), we don't have ty::Generics available to look up the ty::ParamTy.

All of the other usages are fine, but specifically here, I think it would be kind of hard to refactor to use a def id.

Anyways, I will see what I can do.

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

compiler-errors · 2022-06-07T04:05:12Z

Hopefully addressed comments. I couldn't get rid of all of the string manipulation, but I tried to use def-id where I could.

@rustbot ready

cjgillot · 2022-06-11T10:25:43Z

@bors r+

bors · 2022-06-11T10:25:44Z

📌 Commit 91b9988 has been approved by cjgillot

…dy, r=cjgillot Tidy up miscellaneous bounds suggestions Just some small fixes to suggestions - Generalizes `Ty::is_suggestable` into a `TypeVisitor`, so that it can be called on things other than `Ty` - Makes `impl Trait` in arg position no longer suggestible (generalizing the fix in rust-lang#97640) - Fixes `impl Trait` not being replaced with fresh type param when it's deeply nested in function signature (fixes rust-lang#97760) - Fixes some poor handling of `where` clauses with no predicates (also rust-lang#97760) - Uses `InferCtxt::resolve_numeric_literals_with_default` so we suggest `i32` instead of `{integer}` (fixes rust-lang#97677) Sorry there aren't many tests the fixes. Most of them would just be duplicates of other tests with empty `where` clauses or `impl Trait` in arg position instead of generic params. Let me know if you'd want more test coverage.

Dylan-DPC · 2022-06-11T10:58:46Z

failed in rollup ci

Dylan-DPC · 2022-06-11T10:58:52Z

@bors r-

compiler-errors · 2022-06-11T23:39:27Z

Added TypeSuperFoldable import, which was apparently introduced sometime recently

@bors r=cjgillot

bors · 2022-06-11T23:39:29Z

📌 Commit 5f7474e has been approved by cjgillot

bors · 2022-06-12T00:47:57Z

⌛ Testing commit 5f7474e with merge 37a4225...

bors · 2022-06-12T03:28:33Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 37a4225 to master...

rust-timer · 2022-06-12T04:46:33Z

Finished benchmarking commit (37a4225): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-0.3%	-0.4%	2
Improvements 🎉 (secondary)	-0.7%	-0.8%	6
All 😿🎉 (primary)	-0.3%	-0.4%	2

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	3.8%	3.8%	1
Regressions 😿 (secondary)	3.3%	3.3%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.3%	-2.4%	2
All 😿🎉 (primary)	3.8%	3.8%	1

Cycles

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-3.7%	-5.1%	4
Improvements 🎉 (secondary)	-2.7%	-3.3%	4
All 😿🎉 (primary)	-3.7%	-5.1%	4

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

rust-highfive assigned cjgillot Jun 6, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 6, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2022

compiler-errors changed the title ~~Tidy up miscellaneous bounds suggestion~~ Tidy up miscellaneous bounds suggestions Jun 6, 2022

cjgillot reviewed Jun 6, 2022

View reviewed changes

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 Jun 11, 2022

Dylan-DPC mentioned this pull request Jun 11, 2022

Rollup of 5 pull requests #97988

Closed

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 Jun 11, 2022

compiler-errors added 5 commits June 11, 2022 16:27

Make Ty::is_suggestable use a visitor

8506b7d

Handle empty where-clause better

9c47afe

Make is_suggestable work on all TypeFoldable

55805ab

Properly replace impl Trait in fn args, turn {integer} to i32

38d7e27

Address comments

5f7474e

compiler-errors force-pushed the misc-diagnostics-tidy branch from 91b9988 to 5f7474e Compare June 11, 2022 23:38

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 Jun 11, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 12, 2022

bors merged commit 37a4225 into rust-lang:master Jun 12, 2022

rustbot added this to the 1.63.0 milestone Jun 12, 2022

This was referenced Jun 12, 2022

diagnostics: remove trailing spaces #97892

Merged

Rename the ConstS::val field as kind. #97935

Merged

nnethercote mentioned this pull request Jun 20, 2022

Obligation forest tweaks #97674

Merged

compiler-errors deleted the misc-diagnostics-tidy branch August 11, 2023 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy up miscellaneous bounds suggestions #97778

Tidy up miscellaneous bounds suggestions #97778

compiler-errors commented Jun 6, 2022

rust-highfive commented Jun 6, 2022

cjgillot left a comment

cjgillot Jun 6, 2022

compiler-errors Jun 6, 2022

cjgillot Jun 6, 2022

compiler-errors Jun 6, 2022

compiler-errors Jun 6, 2022

compiler-errors commented Jun 7, 2022 •

edited

Loading

cjgillot commented Jun 11, 2022

bors commented Jun 11, 2022

Dylan-DPC commented Jun 11, 2022

Dylan-DPC commented Jun 11, 2022

compiler-errors commented Jun 11, 2022

bors commented Jun 11, 2022

bors commented Jun 12, 2022

bors commented Jun 12, 2022

rust-timer commented Jun 12, 2022

Tidy up miscellaneous bounds suggestions #97778

Tidy up miscellaneous bounds suggestions #97778

Conversation

compiler-errors commented Jun 6, 2022

rust-highfive commented Jun 6, 2022

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Jun 7, 2022 • edited Loading

cjgillot commented Jun 11, 2022

bors commented Jun 11, 2022

Dylan-DPC commented Jun 11, 2022

Dylan-DPC commented Jun 11, 2022

compiler-errors commented Jun 11, 2022

bors commented Jun 11, 2022

bors commented Jun 12, 2022

bors commented Jun 12, 2022

rust-timer commented Jun 12, 2022

Footnotes

compiler-errors commented Jun 7, 2022 •

edited

Loading