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

Suggest using type args directly instead of equality constraint #122591

Merged
merged 1 commit into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 109 additions & 6 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_infer::traits::FulfillmentError;
use rustc_middle::query::Key;
use rustc_middle::ty::GenericParamDefKind;
use rustc_middle::ty::{self, suggest_constraining_type_param};
use rustc_middle::ty::{AdtDef, Ty, TyCtxt, TypeVisitableExt};
use rustc_middle::ty::{Binder, TraitRef};
Expand Down Expand Up @@ -1200,12 +1201,12 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
/// Emits an error regarding forbidden type binding associations
pub fn prohibit_assoc_item_binding(
gurry marked this conversation as resolved.
Show resolved Hide resolved
tcx: TyCtxt<'_>,
span: Span,
segment: Option<(&hir::PathSegment<'_>, Span)>,
binding: &hir::TypeBinding<'_>,
segment: Option<(DefId, &hir::PathSegment<'_>, Span)>,
) -> ErrorGuaranteed {
tcx.dcx().emit_err(AssocTypeBindingNotAllowed {
span,
fn_trait_expansion: if let Some((segment, span)) = segment
let mut err = tcx.dcx().create_err(AssocTypeBindingNotAllowed {
span: binding.span,
fn_trait_expansion: if let Some((_, segment, span)) = segment
&& segment.args().parenthesized == hir::GenericArgsParentheses::ParenSugar
{
Some(ParenthesizedFnTraitExpansion {
Expand All @@ -1215,7 +1216,109 @@ pub fn prohibit_assoc_item_binding(
} else {
None
},
})
});

// Emit a suggestion to turn the assoc item binding into a generic arg
// if the relevant item has a generic param whose name matches the binding name;
// otherwise suggest the removal of the binding.
if let Some((def_id, segment, _)) = segment
&& segment.args().parenthesized == hir::GenericArgsParentheses::No
&& let hir::TypeBindingKind::Equality { term } = binding.kind
{
// Suggests removal of the offending binding
let suggest_removal = |e: &mut Diag<'_>| {
Copy link
Member

@fmease fmease Apr 12, 2024

Choose a reason for hiding this comment

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

Note to self: I still need to review this function. I suspect it can be simplified by a lot but let's see.

let bindings = segment.args().bindings;
let args = segment.args().args;
let binding_span = binding.span;

// Compute the span to remove based on the position
// of the binding. We do that as follows:
// 1. Find the index of the binding in the list of bindings
// 2. Locate the spans preceding and following the binding.
// If it's the first binding the preceding span would be
// that of the last arg
// 3. Using this information work out whether the span
// to remove will start from the end of the preceding span,
// the start of the next span or will simply be the
// span encomassing everything within the generics brackets

let Some(binding_index) = bindings.iter().position(|b| b.hir_id == binding.hir_id)
else {
bug!("a type binding exists but its HIR ID not found in generics");
};

let preceding_span = if binding_index > 0 {
Some(bindings[binding_index - 1].span)
} else {
args.last().map(|a| a.span())
};

let next_span = if binding_index < bindings.len() - 1 {
Some(bindings[binding_index + 1].span)
} else {
None
};

let removal_span = match (preceding_span, next_span) {
(Some(prec), _) => binding_span.with_lo(prec.hi()),
(None, Some(next)) => binding_span.with_hi(next.lo()),
(None, None) => {
let Some(generics_span) = segment.args().span_ext() else {
bug!("a type binding exists but generic span is empty");
};

generics_span
}
};

// Now emit the suggestion
if let Ok(suggestion) = tcx.sess.source_map().span_to_snippet(removal_span) {
e.span_suggestion_verbose(
removal_span,
"consider removing this type binding",
suggestion,
Applicability::MaybeIncorrect,
);
}
};

// Suggest replacing the associated item binding with a generic argument.
// i.e., replacing `<..., T = A, ...>` with `<..., A, ...>`.
let suggest_direct_use = |e: &mut Diag<'_>, sp: Span| {
if let Ok(snippet) = tcx.sess.source_map().span_to_snippet(sp) {
gurry marked this conversation as resolved.
Show resolved Hide resolved
e.span_suggestion_verbose(
binding.span,
format!("to use `{snippet}` as a generic argument specify it directly"),
snippet,
Applicability::MaybeIncorrect,
);
}
};

// Check if the type has a generic param with the
// same name as the assoc type name in type binding
let generics = tcx.generics_of(def_id);
Copy link
Member

Choose a reason for hiding this comment

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

generics_of can panic if the definition (identified by the def_id) can't have generic parameters. Could you check if we can indeed panic here, e.g. if the definition is a type parameter:

fn f<T>() {
    let _: T<X = ()>;
}

If it does, we need to add some checks before trying to use generics_of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this does not ICE

let matching_param =
generics.params.iter().find(|p| p.name.as_str() == binding.ident.as_str());

// Now emit the appropriate suggestion
if let Some(matching_param) = matching_param {
match (&matching_param.kind, term) {
(GenericParamDefKind::Type { .. }, hir::Term::Ty(ty)) => {
suggest_direct_use(&mut err, ty.span);
}
(GenericParamDefKind::Const { .. }, hir::Term::Const(c)) => {
let span = tcx.hir().span(c.hir_id);
suggest_direct_use(&mut err, span);
}
_ => suggest_removal(&mut err),
}
} else {
suggest_removal(&mut err);
}
}

err.emit()
}

pub(crate) fn fn_trait_to_string(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ pub(crate) fn check_generic_arg_count(
if gen_pos != GenericArgPosition::Type
&& let Some(b) = gen_args.bindings.first()
{
prohibit_assoc_item_binding(tcx, b.span, None);
prohibit_assoc_item_binding(tcx, b, None);
}

let explicit_late_bound =
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
ty::BoundConstness::NotConst,
);
if let Some(b) = item_segment.args().bindings.first() {
prohibit_assoc_item_binding(self.tcx(), b.span, Some((item_segment, span)));
prohibit_assoc_item_binding(self.tcx(), b, Some((def_id, item_segment, span)));
}
args
}
Expand Down Expand Up @@ -619,7 +619,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
ty::BoundConstness::NotConst,
);
if let Some(b) = item_segment.args().bindings.first() {
prohibit_assoc_item_binding(self.tcx(), b.span, Some((item_segment, span)));
prohibit_assoc_item_binding(self.tcx(), b, Some((item_def_id, item_segment, span)));
}
args
}
Expand Down Expand Up @@ -764,7 +764,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
constness,
);
if let Some(b) = trait_segment.args().bindings.first() {
prohibit_assoc_item_binding(self.tcx(), b.span, Some((trait_segment, span)));
prohibit_assoc_item_binding(self.tcx(), b, Some((trait_def_id, trait_segment, span)));
}
ty::TraitRef::new(self.tcx(), trait_def_id, generic_args)
}
Expand Down Expand Up @@ -1556,7 +1556,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
for segment in segments {
// Only emit the first error to avoid overloading the user with error messages.
if let Some(b) = segment.args().bindings.first() {
return Err(prohibit_assoc_item_binding(self.tcx(), b.span, None));
return Err(prohibit_assoc_item_binding(self.tcx(), b, None));
}
}

Expand Down
9 changes: 9 additions & 0 deletions tests/rustdoc-ui/invalid_associated_const.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ error[E0229]: associated type bindings are not allowed here
|
LL | type A: S<C<X = 0i32> = 34>;
| ^^^^^^^^ associated type not allowed here
|
help: consider removing this type binding
|
LL | type A: S<C<X = 0i32> = 34>;
| ~~~~~~~~~~

error[E0229]: associated type bindings are not allowed here
--> $DIR/invalid_associated_const.rs:4:17
Expand All @@ -11,6 +16,10 @@ LL | type A: S<C<X = 0i32> = 34>;
| ^^^^^^^^ associated type not allowed here
|
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
help: consider removing this type binding
|
LL | type A: S<C<X = 0i32> = 34>;
| ~~~~~~~~~~

error: aborting due to 2 previous errors

Expand Down
9 changes: 9 additions & 0 deletions tests/rustdoc-ui/issue-102467.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ error[E0229]: associated type bindings are not allowed here
|
LL | type A: S<C<X = 0i32> = 34>;
| ^^^^^^^^ associated type not allowed here
|
help: consider removing this type binding
|
LL | type A: S<C<X = 0i32> = 34>;
| ~~~~~~~~~~

error[E0229]: associated type bindings are not allowed here
--> $DIR/issue-102467.rs:7:17
Expand All @@ -11,6 +16,10 @@ LL | type A: S<C<X = 0i32> = 34>;
| ^^^^^^^^ associated type not allowed here
|
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
help: consider removing this type binding
|
LL | type A: S<C<X = 0i32> = 34>;
| ~~~~~~~~~~

error: aborting due to 2 previous errors

Expand Down
9 changes: 9 additions & 0 deletions tests/ui/associated-consts/issue-102335-const.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ error[E0229]: associated type bindings are not allowed here
|
LL | type A: S<C<X = 0i32> = 34>;
| ^^^^^^^^ associated type not allowed here
|
help: consider removing this type binding
|
LL | type A: S<C<X = 0i32> = 34>;
| ~~~~~~~~~~

error[E0229]: associated type bindings are not allowed here
--> $DIR/issue-102335-const.rs:4:17
Expand All @@ -11,6 +16,10 @@ LL | type A: S<C<X = 0i32> = 34>;
| ^^^^^^^^ associated type not allowed here
|
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
help: consider removing this type binding
|
LL | type A: S<C<X = 0i32> = 34>;
| ~~~~~~~~~~

error: aborting due to 2 previous errors

Expand Down
8 changes: 7 additions & 1 deletion tests/ui/associated-type-bounds/issue-102335-ty.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
trait T {
type A: S<C<i32 = u32> = ()>;
type A: S<C<i32 = u32> = ()>; // Just one erroneous equality constraint
//~^ ERROR associated type bindings are not allowed here
//~| ERROR associated type bindings are not allowed here
}

trait T2 {
type A: S<C<i32 = u32, X = i32> = ()>; // More than one erroneous equality constraints
//~^ ERROR associated type bindings are not allowed here
//~| ERROR associated type bindings are not allowed here
}
Expand Down
38 changes: 35 additions & 3 deletions tests/ui/associated-type-bounds/issue-102335-ty.stderr
Original file line number Diff line number Diff line change
@@ -1,17 +1,49 @@
error[E0229]: associated type bindings are not allowed here
--> $DIR/issue-102335-ty.rs:2:17
|
LL | type A: S<C<i32 = u32> = ()>;
LL | type A: S<C<i32 = u32> = ()>; // Just one erroneous equality constraint
| ^^^^^^^^^ associated type not allowed here
|
help: consider removing this type binding
|
LL | type A: S<C<i32 = u32> = ()>; // Just one erroneous equality constraint
| ~~~~~~~~~~~

error[E0229]: associated type bindings are not allowed here
--> $DIR/issue-102335-ty.rs:2:17
|
LL | type A: S<C<i32 = u32> = ()>;
LL | type A: S<C<i32 = u32> = ()>; // Just one erroneous equality constraint
| ^^^^^^^^^ associated type not allowed here
|
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
help: consider removing this type binding
|
LL | type A: S<C<i32 = u32> = ()>; // Just one erroneous equality constraint
| ~~~~~~~~~~~

error[E0229]: associated type bindings are not allowed here
--> $DIR/issue-102335-ty.rs:8:17
|
LL | type A: S<C<i32 = u32, X = i32> = ()>; // More than one erroneous equality constraints
| ^^^^^^^^^ associated type not allowed here
|
help: consider removing this type binding
|
LL | type A: S<C<i32 = u32, X = i32> = ()>; // More than one erroneous equality constraints
| ~~~~~~~~~~

error[E0229]: associated type bindings are not allowed here
--> $DIR/issue-102335-ty.rs:8:17
|
LL | type A: S<C<i32 = u32, X = i32> = ()>; // More than one erroneous equality constraints
| ^^^^^^^^^ associated type not allowed here
|
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
help: consider removing this type binding
|
LL | type A: S<C<i32 = u32, X = i32> = ()>; // More than one erroneous equality constraints
| ~~~~~~~~~~

error: aborting due to 2 previous errors
error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0229`.
Loading
Loading