-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
Unconstrained parameter fix #148788
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
base: main
Are you sure you want to change the base?
Unconstrained parameter fix #148788
Changes from all commits
0ade480
a8a7e19
12340a8
69165e1
e7d4c8a
1065e84
3d49089
bf798a3
179940d
9d1025b
c735d38
8200d5c
71ecd9b
5d77bb2
48c4e3c
78b460f
d5406c4
2461e5d
1cdc8cb
0b83de8
7664723
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1095,6 +1095,93 @@ impl<'hir> Generics<'hir> { | |||||
| bound_span.with_lo(bounds[bound_pos - 1].span().hi()) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Computes the span representing the removal of a generic parameter at `param_index`. | ||||||
| /// | ||||||
| /// This function identifies the correct slice of source code to delete so that the | ||||||
| /// remaining generic list remains syntactically valid (handling commas and brackets). | ||||||
| /// | ||||||
| /// ### Examples | ||||||
| /// | ||||||
| /// 1. **With a following parameter:** (Includes the trailing comma) | ||||||
| /// - Input: `<T, U>` (index 0) | ||||||
| /// - Produces span for: `T, ` | ||||||
| /// | ||||||
| /// 2. **With a previous parameter:** (Includes the leading comma and bounds) | ||||||
| /// - Input: `<T: Clone, U>` (index 1) | ||||||
| /// - Produces span for: `, U` | ||||||
| /// | ||||||
| /// 3. **The only parameter:** (Includes the angle brackets) | ||||||
| /// - Input: `<T>` (index 0) | ||||||
| /// - Produces span for: `<T>` | ||||||
| /// | ||||||
| /// 4. **Parameter with where-clause bounds:** | ||||||
| /// - Input: `fn foo<T, U>() where T: Copy` (index 0) | ||||||
| /// - Produces span for: `T, ` (The where-clause remains for other logic to handle). | ||||||
| pub fn span_for_param_removal(&self, param_index: usize) -> Span { | ||||||
| if param_index >= self.params.len() { | ||||||
| return self.span.shrink_to_hi(); | ||||||
| } | ||||||
|
|
||||||
| let is_impl_generic = |par: &&GenericParam<'_>| match par.kind { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| GenericParamKind::Type { .. } | ||||||
| | GenericParamKind::Const { .. } | ||||||
| | GenericParamKind::Lifetime { kind: LifetimeParamKind::Explicit } => true, | ||||||
| _ => false, | ||||||
| }; | ||||||
|
|
||||||
| // Find the span of the type parameter. | ||||||
| if let Some(next) = self.params[param_index + 1..].iter().find(is_impl_generic) { | ||||||
| self.params[param_index].span.until(next.span) | ||||||
| } else if let Some(prev) = self.params[..param_index].iter().rfind(is_impl_generic) { | ||||||
| let mut prev_span = prev.span; | ||||||
| // Consider the span of the bounds with the previous generic parameter when there is. | ||||||
| if let Some(prev_bounds_span) = self.span_for_param_bounds(prev) { | ||||||
| prev_span = prev_span.to(prev_bounds_span); | ||||||
| } | ||||||
|
|
||||||
| // Consider the span of the bounds with the current generic parameter when there is. | ||||||
| prev_span.shrink_to_hi().to( | ||||||
| if let Some(cur_bounds_span) = self.span_for_param_bounds(&self.params[param_index]) | ||||||
| { | ||||||
| cur_bounds_span | ||||||
| } else { | ||||||
| self.params[param_index].span | ||||||
| }, | ||||||
| ) | ||||||
| } else { | ||||||
| // Remove also angle brackets <> when there is just ONE generic parameter. | ||||||
| self.span | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Returns the span of the `WherePredicate` associated with the given `GenericParam`, if any. | ||||||
| /// | ||||||
| /// This looks specifically for predicates in the `where` clause that were generated | ||||||
| /// from the parameter definition (e.g., `T` in `where T: Bound`). | ||||||
| /// | ||||||
| /// ### Example | ||||||
| /// | ||||||
| /// - Input: `param` representing `T` | ||||||
| /// - Context: `where T: Clone + Default, U: Copy` | ||||||
| /// - Returns: Span of `T: Clone + Default` | ||||||
| fn span_for_param_bounds(&self, param: &GenericParam<'hir>) -> Option<Span> { | ||||||
TomtheCoder2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| self.predicates | ||||||
| .iter() | ||||||
| .find(|pred| { | ||||||
| if let WherePredicateKind::BoundPredicate(WhereBoundPredicate { | ||||||
| origin: PredicateOrigin::GenericParam, | ||||||
| bounded_ty, | ||||||
| .. | ||||||
| }) = pred.kind | ||||||
| { | ||||||
| bounded_ty.span == param.span | ||||||
| } else { | ||||||
| false | ||||||
| } | ||||||
| }) | ||||||
| .map(|pred| pred.span) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// A single predicate in a where-clause. | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,217 @@ | ||||||
| use std::ops::ControlFlow; | ||||||
|
|
||||||
| use rustc_errors::{Applicability, Diag}; | ||||||
| use rustc_hir::def::DefKind; | ||||||
| use rustc_hir::def_id::{DefId, LocalDefId}; | ||||||
| use rustc_hir::intravisit::{self, Visitor, walk_lifetime}; | ||||||
| use rustc_hir::{GenericArg, HirId, LifetimeKind, Path, QPath, TyKind}; | ||||||
| use rustc_middle::hir::nested_filter::All; | ||||||
| use rustc_middle::ty::{GenericParamDef, GenericParamDefKind, TyCtxt}; | ||||||
|
|
||||||
| use crate::hir::def::Res; | ||||||
|
|
||||||
| /// Use a Visitor to find usages of the type or lifetime parameter | ||||||
| struct ParamUsageVisitor<'tcx> { | ||||||
| tcx: TyCtxt<'tcx>, | ||||||
| /// The `DefId` of the generic parameter we are looking for. | ||||||
| param_def_id: DefId, | ||||||
| found: bool, | ||||||
| } | ||||||
|
|
||||||
| impl<'tcx> Visitor<'tcx> for ParamUsageVisitor<'tcx> { | ||||||
| type NestedFilter = All; | ||||||
|
|
||||||
| fn maybe_tcx(&mut self) -> Self::MaybeTyCtxt { | ||||||
| self.tcx | ||||||
| } | ||||||
|
|
||||||
| type Result = ControlFlow<()>; | ||||||
|
|
||||||
| fn visit_path(&mut self, path: &Path<'tcx>, _id: HirId) -> Self::Result { | ||||||
| if let Some(res_def_id) = path.res.opt_def_id() { | ||||||
| if res_def_id == self.param_def_id { | ||||||
| self.found = true; | ||||||
| return ControlFlow::Break(()); | ||||||
| } | ||||||
| } | ||||||
| intravisit::walk_path(self, path) | ||||||
| } | ||||||
|
|
||||||
| fn visit_lifetime(&mut self, lifetime: &'tcx rustc_hir::Lifetime) -> Self::Result { | ||||||
| if let LifetimeKind::Param(id) = lifetime.kind { | ||||||
| if let Some(local_def_id) = self.param_def_id.as_local() { | ||||||
| if id == local_def_id { | ||||||
| self.found = true; | ||||||
| return ControlFlow::Break(()); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| walk_lifetime(self, lifetime) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Adds a suggestion to a diagnostic to either remove an unused generic parameter, or use it. | ||||||
| /// | ||||||
| /// # Examples | ||||||
| /// | ||||||
| /// - `impl<T> Struct { ... }` where `T` is unused -> suggests removing `T` or using it. | ||||||
| /// - `impl<'a> Struct { ... }` where `'a` is unused -> suggests removing `'a`. | ||||||
| /// - `impl<T> Struct { // T used in here }` where `T` is used in the body but not in the self type -> suggests adding `T` to the self type and struct definition. | ||||||
| /// - `impl<T> Struct { ... }` where the struct has a generic parameter with a default -> suggests adding `T` to the self type. | ||||||
| pub(crate) fn suggest_to_remove_or_use_generic( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add your own test file that tests all of these cases specifically, as well as where the type is defined in another crate, or where the type is generated from a macro. |
||||||
| tcx: TyCtxt<'_>, | ||||||
| diag: &mut Diag<'_>, | ||||||
| impl_def_id: LocalDefId, | ||||||
| param: &GenericParamDef, | ||||||
| is_lifetime: bool, | ||||||
| ) { | ||||||
| let node = tcx.hir_node_by_def_id(impl_def_id); | ||||||
| let hir_impl = node.expect_item().expect_impl(); | ||||||
|
|
||||||
| let Some((index, _)) = hir_impl | ||||||
| .generics | ||||||
| .params | ||||||
| .iter() | ||||||
| .enumerate() | ||||||
| .find(|(_, par)| par.def_id.to_def_id() == param.def_id) | ||||||
| else { | ||||||
| return; | ||||||
| }; | ||||||
|
|
||||||
| // Get the Struct/ADT definition ID from the self type | ||||||
| let struct_def_id = if let TyKind::Path(QPath::Resolved(_, path)) = hir_impl.self_ty.kind | ||||||
| && let Res::Def(DefKind::Struct | DefKind::Enum | DefKind::Union, def_id) = path.res | ||||||
| { | ||||||
| def_id | ||||||
| } else { | ||||||
| return; | ||||||
| }; | ||||||
|
|
||||||
| // Count how many generic parameters are defined in the struct definition | ||||||
| let generics = tcx.generics_of(struct_def_id); | ||||||
| let total_params = generics | ||||||
| .own_params | ||||||
| .iter() | ||||||
| .filter(|p| { | ||||||
| if is_lifetime { | ||||||
| matches!(p.kind, GenericParamDefKind::Lifetime) | ||||||
| } else { | ||||||
| matches!(p.kind, GenericParamDefKind::Type { .. }) | ||||||
| } | ||||||
| }) | ||||||
| .count(); | ||||||
|
|
||||||
| // Count how many arguments are currently provided in the impl | ||||||
| let mut provided_params = 0; | ||||||
| let mut last_segment_args = None; | ||||||
|
|
||||||
| if let TyKind::Path(QPath::Resolved(_, path)) = hir_impl.self_ty.kind | ||||||
| && let Some(seg) = path.segments.last() | ||||||
| && let Some(args) = seg.args | ||||||
| { | ||||||
| last_segment_args = Some(args); | ||||||
| provided_params = args | ||||||
| .args | ||||||
| .iter() | ||||||
| .filter(|arg| match arg { | ||||||
| GenericArg::Lifetime(_) => is_lifetime, | ||||||
| GenericArg::Type(_) => !is_lifetime, | ||||||
| _ => false, | ||||||
| }) | ||||||
| .count(); | ||||||
| } | ||||||
|
|
||||||
| let mut visitor = ParamUsageVisitor { tcx, param_def_id: param.def_id, found: false }; | ||||||
| for item_ref in hir_impl.items { | ||||||
| let _ = visitor.visit_impl_item_ref(item_ref); | ||||||
| if visitor.found { | ||||||
| break; | ||||||
| } | ||||||
| } | ||||||
| let is_param_used = visitor.found; | ||||||
|
|
||||||
| let mut suggestions = vec![]; | ||||||
|
|
||||||
| // Option A: Remove (Only if not used in body) | ||||||
| if !is_param_used { | ||||||
| suggestions.push((hir_impl.generics.span_for_param_removal(index), String::new())); | ||||||
| } | ||||||
|
|
||||||
| // Option B: Suggest adding only if there's an available parameter in the struct definition | ||||||
| // or the parameter is already used somewhere, then we suggest adding to the impl struct and the struct definition | ||||||
| if provided_params < total_params || is_param_used { | ||||||
| if let Some(args) = last_segment_args { | ||||||
| // Struct already has <...>, append to it | ||||||
| suggestions.push((args.span().unwrap().shrink_to_hi(), format!(", {}", param.name))); | ||||||
| } else if let TyKind::Path(QPath::Resolved(_, path)) = hir_impl.self_ty.kind { | ||||||
| // Struct has no <...> yet, add it | ||||||
| let seg = path.segments.last().unwrap(); | ||||||
| suggestions.push((seg.ident.span.shrink_to_hi(), format!("<{}>", param.name))); | ||||||
| } | ||||||
| if is_param_used { | ||||||
| // If the parameter is used in the body, we also want to suggest adding it to the struct definition if it's not already there | ||||||
| let struct_span = tcx.def_span(struct_def_id); | ||||||
| let struct_generic_params = tcx | ||||||
| .hir_node_by_def_id(struct_def_id.expect_local()) | ||||||
| .expect_item() | ||||||
| .expect_struct() | ||||||
| .1 | ||||||
| .params; | ||||||
| if struct_generic_params.len() > 0 { | ||||||
| let last_param = struct_generic_params.last().unwrap(); | ||||||
| suggestions.push((last_param.span.shrink_to_hi(), format!(", {}", param.name))); | ||||||
| } else { | ||||||
| suggestions.push((struct_span.shrink_to_hi(), format!("<{}>", param.name))); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if suggestions.is_empty() { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| let parameter_type = if is_lifetime { "lifetime" } else { "type" }; | ||||||
| if is_param_used { | ||||||
| let msg = format!( | ||||||
| "make use of the {} parameter `{}` in the `self` type", | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Can you get the name of the |
||||||
| parameter_type, param.name | ||||||
| ); | ||||||
| diag.span_suggestion( | ||||||
| suggestions[0].0, | ||||||
| msg, | ||||||
| suggestions[0].1.clone(), | ||||||
| Applicability::MaybeIncorrect, | ||||||
| ); | ||||||
| let msg2 = format!( | ||||||
| "and add it to the struct definition of {} as well since it's used in the body of the impl", | ||||||
| tcx.def_path_str(struct_def_id) | ||||||
| ); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should probably use a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And change the message to "use this type parameter in the type definition" |
||||||
| diag.span_suggestion( | ||||||
| suggestions[1].0, | ||||||
| msg2, | ||||||
| suggestions[1].1.clone(), | ||||||
| Applicability::MaybeIncorrect, | ||||||
| ); | ||||||
| } else { | ||||||
| let msg = if suggestions.len() == 2 { | ||||||
| format!("either remove the unused {} parameter `{}`", parameter_type, param.name) | ||||||
| } else { | ||||||
| format!("remove the unused {} parameter `{}`", parameter_type, param.name) | ||||||
| }; | ||||||
| diag.span_suggestion( | ||||||
| suggestions[0].0, | ||||||
| msg, | ||||||
| suggestions[0].1.clone(), | ||||||
| Applicability::MaybeIncorrect, | ||||||
| ); | ||||||
| if suggestions.len() == 2 { | ||||||
| let msg = format!("or make use of it"); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| diag.span_suggestion( | ||||||
| suggestions[1].0, | ||||||
| msg, | ||||||
| suggestions[1].1.clone(), | ||||||
| Applicability::MaybeIncorrect, | ||||||
| ); | ||||||
| } | ||||||
| }; | ||||||
| } | ||||||
Uh oh!
There was an error while loading. Please reload this page.