-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Modify existing bounds if they exist #107555
Modify existing bounds if they exist #107555
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good other than perhaps some of the logic can be simplified. Could you do that? Otherwise r=me, should be fine to land regardless.
Ping me if you give up on trying to simplify stuff and I can approve.
.map(|id| (id, hir.local_def_id_to_hir_id(id))) | ||
.and_then(|(local_id, hir_id)| tcx.hir().find_parent(hir_id).map(|node| (local_id, node))); | ||
if let Some((local_id, generics)) = parent.as_ref() | ||
.and_then(|(local_id, node)| node.generics().map(|generics| (local_id, generics))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's gotta be a way to simplify this, perhaps by avoiding these combinators... Could you maybe make this logic a bit simpler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't get rid of an and_then
, but does this look cleaner to you?
7dad185
to
6dc7d7a
Compare
compiler/rustc_infer/src/infer/error_reporting/note_and_explain.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_infer/src/infer/error_reporting/note_and_explain.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_infer/src/infer/error_reporting/note_and_explain.rs
Outdated
Show resolved
Hide resolved
.filter_map(|bound| { | ||
let bound_trait_path = bound.trait_ref()?.path; | ||
let def_id = bound_trait_path.res.opt_def_id()?; | ||
let generic_args = bound_trait_path.segments.iter().last().map(|path| path.args()); | ||
(def_id == trait_ref.def_id).then_some((bound_trait_path.span, generic_args)) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we have like...
T: Trait<A> + Trait<B>
and then we suggest to restrict T: Trait<B, Output = C>
What will that do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the associated type prevent multiple trait impls on the same type anyways? So Trait<A> + Trait<B>
would error for other reasons before suggesting to constrain the bounds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g.
trait Foo<T> {
type Output;
fn bar() -> Self::Output;
}
fn test<T>() -> i32 where T: Foo<()>, T: Foo<i32> {
<T as Foo<()>>::bar()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I guess that already suggests .. , Output = i32>
correctly
6dc7d7a
to
af5a37e
Compare
Changes:
|
Anywho @bors r+ rollup |
…suggestion, r=compiler-errors Modify existing bounds if they exist Fixes rust-lang#107335. This implementation is kinda gross but I don't really see a better way to do it. This primarily does two things: Modifies `suggest_constraining_type_param` to accept a new parameter that indicates a span to be replaced instead of added, if presented, and limit the additive suggestions to either suggest a new bound on an existing bound (see newly added unit test) or add the generics argument if a generics argument wasn't found. The former change is required to retain the capability to add an entirely new bounds if it was entirely omitted. r? `@compiler-errors`
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#100599 (Add compiler error E0523 long description and test) - rust-lang#107471 (rustdoc: do not include empty default-settings tag in HTML) - rust-lang#107555 (Modify existing bounds if they exist) - rust-lang#107662 (Turn projections into copies in CopyProp.) - rust-lang#107695 (Add test for Future inflating arg size to 3x ) - rust-lang#107700 (Run the tools builder on all PRs) - rust-lang#107706 (Mark 'atomic_mut_ptr' methods const) - rust-lang#107709 (Fix problem noticed in PR106859 with char -> u8 suggestion) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #107335.
This implementation is kinda gross but I don't really see a better way to do it.
This primarily does two things: Modifies
suggest_constraining_type_param
to accept a new parameter that indicates a span to be replaced instead of added, if presented, and limit the additive suggestions to either suggest a new bound on an existing bound (see newly added unit test) or add the generics argument if a generics argument wasn't found.The former change is required to retain the capability to add an entirely new bounds if it was entirely omitted.
r? @compiler-errors