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

Improve the diagnostics for unused generic params in ty aliases & permit bivariant but constrained params in LTAs #120556

Merged
merged 1 commit into from
Feb 5, 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
6 changes: 3 additions & 3 deletions compiler/rustc_error_codes/src/error_codes/E0091.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
An unnecessary type or const parameter was given in a type alias.
An unnecessary type parameter was given in a type alias.

Erroneous code example:

```compile_fail,E0091
type Foo<T> = u32; // error: type parameter `T` is unused
type Foo<T> = u32; // error: type parameter `T` is never used
// or:
type Foo<A,B> = Box<A>; // error: type parameter `B` is unused
type Foo<A, B> = Box<A>; // error: type parameter `B` is never used
```

Please check you didn't write too many parameters. Example:
Expand Down
11 changes: 11 additions & 0 deletions compiler/rustc_hir_analysis/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,17 @@ hir_analysis_unused_associated_type_bounds =
.note = this associated type has a `where Self: Sized` bound. Thus, while the associated type can be specified, it cannot be used in any way, because trait objects are not `Sized`.
.suggestion = remove this bound

hir_analysis_unused_generic_parameter =
{$param_def_kind} `{$param_name}` is never used
.label = unused {$param_def_kind}
.const_param_help = if you intended `{$param_name}` to be a const parameter, use `const {$param_name}: /* Type */` instead
hir_analysis_unused_generic_parameter_adt_help =
consider removing `{$param_name}`, referring to it in a field, or using a marker such as `{$phantom_data}`
hir_analysis_unused_generic_parameter_adt_no_phantom_data_help =
consider removing `{$param_name}` or referring to it in a field
hir_analysis_unused_generic_parameter_ty_alias_help =
consider removing `{$param_name}` or referring to it in the body of the type alias

hir_analysis_value_of_associated_struct_already_specified =
the value of the associated type `{$item_name}` in trait `{$def_path}` is already specified
.label = re-bound here
Expand Down
77 changes: 54 additions & 23 deletions compiler/rustc_hir_analysis/src/check/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use rustc_trait_selection::traits::outlives_bounds::InferCtxtExt as _;
use rustc_trait_selection::traits::{self, TraitEngine, TraitEngineExt as _};
use rustc_type_ir::fold::TypeFoldable;

use std::cell::LazyCell;
use std::ops::ControlFlow;

pub fn check_abi(tcx: TyCtxt<'_>, hir_id: hir::HirId, span: Span, abi: Abi) {
Expand Down Expand Up @@ -520,9 +521,7 @@ pub(crate) fn check_item_type(tcx: TyCtxt<'_>, def_id: LocalDefId) {
}
}
DefKind::TyAlias => {
let pty_ty = tcx.type_of(def_id).instantiate_identity();
let generics = tcx.generics_of(def_id);
check_type_params_are_used(tcx, generics, pty_ty);
check_type_alias_type_params_are_used(tcx, def_id);
}
DefKind::ForeignMod => {
let it = tcx.hir().expect_item(def_id);
Expand Down Expand Up @@ -1269,28 +1268,51 @@ fn detect_discriminant_duplicate<'tcx>(tcx: TyCtxt<'tcx>, adt: ty::AdtDef<'tcx>)
}
}

pub(super) fn check_type_params_are_used<'tcx>(
tcx: TyCtxt<'tcx>,
generics: &ty::Generics,
ty: Ty<'tcx>,
) {
debug!("check_type_params_are_used(generics={:?}, ty={:?})", generics, ty);

assert_eq!(generics.parent, None);
fn check_type_alias_type_params_are_used<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) {
if tcx.type_alias_is_lazy(def_id) {
// Since we compute the variances for lazy type aliases and already reject bivariant
// parameters as unused, we can and should skip this check for lazy type aliases.
return;
}

let generics = tcx.generics_of(def_id);
if generics.own_counts().types == 0 {
return;
}

let mut params_used = BitSet::new_empty(generics.params.len());

let ty = tcx.type_of(def_id).instantiate_identity();
if ty.references_error() {
// If there is already another error, do not emit
// an error for not using a type parameter.
// If there is already another error, do not emit an error for not using a type parameter.
assert!(tcx.dcx().has_errors().is_some());
return;
}

// Lazily calculated because it is only needed in case of an error.
let bounded_params = LazyCell::new(|| {
tcx.explicit_predicates_of(def_id)
.predicates
.iter()
.filter_map(|(predicate, span)| {
let bounded_ty = match predicate.kind().skip_binder() {
ty::ClauseKind::Trait(pred) => pred.trait_ref.self_ty(),
ty::ClauseKind::TypeOutlives(pred) => pred.0,
_ => return None,
};
if let ty::Param(param) = bounded_ty.kind() {
Some((param.index, span))
} else {
None
}
})
// FIXME: This assumes that elaborated `Sized` bounds come first (which does hold at the
// time of writing). This is a bit fragile since we later use the span to detect elaborated
// `Sized` bounds. If they came last for example, this would break `Trait + /*elab*/Sized`
// since it would overwrite the span of the user-written bound. This could be fixed by
// folding the spans with `Span::to` which requires a bit of effort I think.
Comment on lines +1307 to +1311
Copy link
Member Author

@fmease fmease Feb 1, 2024

Choose a reason for hiding this comment

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

Didn't want to deal with this right now but I can try to do so if you'd like me to. I think it'll be a bit nasty to implement though. Consider a hypothetical set of predicates like [<A as TraitA>, <B as TraitB>, /*elab*/<A as Sized>] (note: we need to merge the spans of the 0th and the 2nd predicate).

Maybe it's actually not that ugly to implement, I just haven't spent any time to think about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

why does it rely on that? the spans differ, so both elaborated and user-written would end up in the hash set

Copy link
Contributor

@oli-obk oli-obk Feb 1, 2024

Choose a reason for hiding this comment

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

🤦 it's a hashmap and this collect relies on the hash map impl overwriting with follow up items of the same key

Copy link
Contributor

Choose a reason for hiding this comment

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

seems fine to rely on this, there are tests after all

.collect::<FxHashMap<_, _>>()
});

let mut params_used = BitSet::new_empty(generics.params.len());
for leaf in ty.walk() {
if let GenericArgKind::Type(leaf_ty) = leaf.unpack()
&& let ty::Param(param) = leaf_ty.kind()
Expand All @@ -1305,15 +1327,24 @@ pub(super) fn check_type_params_are_used<'tcx>(
&& let ty::GenericParamDefKind::Type { .. } = param.kind
{
let span = tcx.def_span(param.def_id);
struct_span_code_err!(
tcx.dcx(),
let param_name = Ident::new(param.name, span);

// The corresponding predicates are post-`Sized`-elaboration. Therefore we
// * check for emptiness to detect lone user-written `?Sized` bounds
// * compare the param span to the pred span to detect lone user-written `Sized` bounds
let has_explicit_bounds = bounded_params.is_empty()
|| (*bounded_params).get(&param.index).is_some_and(|&&pred_sp| pred_sp != span);
let const_param_help = (!has_explicit_bounds).then_some(());

let mut diag = tcx.dcx().create_err(errors::UnusedGenericParameter {
span,
E0091,
"type parameter `{}` is unused",
param.name,
)
.with_span_label(span, "unused type parameter")
.emit();
param_name,
param_def_kind: tcx.def_descr(param.def_id),
help: errors::UnusedGenericParameterHelp::TyAlias { param_name },
const_param_help,
});
diag.code(E0091);
diag.emit();
}
}
}
Expand Down
64 changes: 33 additions & 31 deletions compiler/rustc_hir_analysis/src/check/wfcheck.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
use crate::autoderef::Autoderef;
use crate::constrained_generic_params::{identify_constrained_generic_params, Parameter};
use crate::errors;

use rustc_ast as ast;
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet};
use rustc_errors::{
codes::*, pluralize, struct_span_code_err, Applicability, DiagnosticBuilder, ErrorGuaranteed,
};
use rustc_errors::{codes::*, pluralize, struct_span_code_err, Applicability, ErrorGuaranteed};
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, LocalDefId, LocalModDefId};
use rustc_hir::lang_items::LangItem;
Expand All @@ -21,7 +20,7 @@ use rustc_middle::ty::{
};
use rustc_middle::ty::{GenericArgKind, GenericArgs};
use rustc_session::parse::feature_err;
use rustc_span::symbol::{sym, Ident, Symbol};
use rustc_span::symbol::{sym, Ident};
use rustc_span::{Span, DUMMY_SP};
use rustc_target::spec::abi::Abi;
use rustc_trait_selection::regions::InferCtxtRegionExt;
Expand Down Expand Up @@ -1869,7 +1868,7 @@ fn check_variances_for_type_defn<'tcx>(
hir::ParamName::Error => {}
_ => {
let has_explicit_bounds = explicitly_bounded_params.contains(&parameter);
report_bivariance(tcx, hir_param, has_explicit_bounds);
report_bivariance(tcx, hir_param, has_explicit_bounds, item.kind);
}
}
}
Expand All @@ -1879,30 +1878,38 @@ fn report_bivariance(
tcx: TyCtxt<'_>,
param: &rustc_hir::GenericParam<'_>,
has_explicit_bounds: bool,
item_kind: ItemKind<'_>,
) -> ErrorGuaranteed {
let span = param.span;
let param_name = param.name.ident().name;
let mut err = error_392(tcx, span, param_name);

let suggested_marker_id = tcx.lang_items().phantom_data();
// Help is available only in presence of lang items.
let msg = if let Some(def_id) = suggested_marker_id {
format!(
"consider removing `{}`, referring to it in a field, or using a marker such as `{}`",
param_name,
tcx.def_path_str(def_id),
)
} else {
format!("consider removing `{param_name}` or referring to it in a field")
let param_name = param.name.ident();
Copy link
Member Author

@fmease fmease Feb 1, 2024

Choose a reason for hiding this comment

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

I'm no longer extracting the Symbol from the Ident via param.name.ident().name since the Display impl for Ident accounts for escaping via r#. This way, we properly display r#if in type T<r#if> = (); as r#if instead of if.


let help = match item_kind {
ItemKind::Enum(..) | ItemKind::Struct(..) | ItemKind::Union(..) => {
if let Some(def_id) = tcx.lang_items().phantom_data() {
errors::UnusedGenericParameterHelp::Adt {
param_name,
phantom_data: tcx.def_path_str(def_id),
}
} else {
errors::UnusedGenericParameterHelp::AdtNoPhantomData { param_name }
}
}
ItemKind::TyAlias(..) => errors::UnusedGenericParameterHelp::TyAlias { param_name },
item_kind => bug!("report_bivariance: unexpected item kind: {item_kind:?}"),
};
err.help(msg);

if matches!(param.kind, hir::GenericParamKind::Type { .. }) && !has_explicit_bounds {
err.help(format!(
"if you intended `{param_name}` to be a const parameter, use `const {param_name}: usize` instead"
));
}
err.emit()
let const_param_help =
matches!(param.kind, hir::GenericParamKind::Type { .. } if !has_explicit_bounds)
.then_some(());

let mut diag = tcx.dcx().create_err(errors::UnusedGenericParameter {
span: param.span,
param_name,
param_def_kind: tcx.def_descr(param.def_id.to_def_id()),
help,
const_param_help,
});
diag.code(E0392);
diag.emit()
}

impl<'tcx> WfCheckingCtxt<'_, 'tcx> {
Expand Down Expand Up @@ -1967,11 +1974,6 @@ fn check_mod_type_wf(tcx: TyCtxt<'_>, module: LocalModDefId) -> Result<(), Error
res
}

fn error_392(tcx: TyCtxt<'_>, span: Span, param_name: Symbol) -> DiagnosticBuilder<'_> {
struct_span_code_err!(tcx.dcx(), span, E0392, "parameter `{param_name}` is never used")
.with_span_label(span, "unused parameter")
}

pub fn provide(providers: &mut Providers) {
*providers = Providers { check_mod_type_wf, check_well_formed, ..*providers };
}
24 changes: 24 additions & 0 deletions compiler/rustc_hir_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1511,3 +1511,27 @@ pub struct NotSupportedDelegation<'a> {
#[label]
pub callee_span: Span,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_unused_generic_parameter)]
pub(crate) struct UnusedGenericParameter {
#[primary_span]
#[label]
pub span: Span,
pub param_name: Ident,
pub param_def_kind: &'static str,
#[subdiagnostic]
pub help: UnusedGenericParameterHelp,
#[help(hir_analysis_const_param_help)]
pub const_param_help: Option<()>,
}

#[derive(Subdiagnostic)]
pub(crate) enum UnusedGenericParameterHelp {
#[help(hir_analysis_unused_generic_parameter_adt_help)]
Adt { param_name: Ident, phantom_data: String },
#[help(hir_analysis_unused_generic_parameter_adt_no_phantom_data_help)]
AdtNoPhantomData { param_name: Ident },
#[help(hir_analysis_unused_generic_parameter_ty_alias_help)]
TyAlias { param_name: Ident },
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@ error: expected identifier, found reserved identifier `_`
LL | fn bad_infer_fn<_>() {}
| ^ expected identifier, found reserved identifier

error[E0392]: parameter `_` is never used
error[E0392]: type parameter `_` is never used
Copy link
Member Author

@fmease fmease Feb 1, 2024

Choose a reason for hiding this comment

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

I could add extra code for skipping this error entirely by delaying a bug on _ when checking the “usedness”. Not sure if it'd be worth it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may be very hard to track the expected identifier, found reserved identifier error all the way into the type system. We'd probably have to add a GenericParamDefKind::Error to do that. May be worth exploring, but not in this PR

--> $DIR/infer-arg-test.rs:7:17
|
LL | struct BadInfer<_>;
| ^ unused parameter
| ^ unused type parameter
|
= help: consider removing `_`, referring to it in a field, or using a marker such as `PhantomData`
= help: if you intended `_` to be a const parameter, use `const _: usize` instead
= help: if you intended `_` to be a const parameter, use `const _: /* Type */` instead

error[E0107]: struct takes 2 generic arguments but 3 generic arguments were supplied
--> $DIR/infer-arg-test.rs:18:10
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/const-generics/issue-46511.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ LL | _a: [u8; std::mem::size_of::<&'a mut u8>()]
= note: lifetime parameters may not be used in const expressions
= help: add `#![feature(generic_const_exprs)]` to allow generic const expressions

error[E0392]: parameter `'a` is never used
error[E0392]: lifetime parameter `'a` is never used
--> $DIR/issue-46511.rs:3:12
|
LL | struct Foo<'a>
| ^^ unused parameter
| ^^ unused lifetime parameter
|
= help: consider removing `'a`, referring to it in a field, or using a marker such as `PhantomData`

Expand Down
6 changes: 3 additions & 3 deletions tests/ui/const-generics/issues/issue-67375.min.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ LL | inner: [(); { [|_: &T| {}; 0].len() }],
= note: type parameters may not be used in const expressions
= help: add `#![feature(generic_const_exprs)]` to allow generic const expressions

error[E0392]: parameter `T` is never used
error[E0392]: type parameter `T` is never used
--> $DIR/issue-67375.rs:5:12
|
LL | struct Bug<T> {
| ^ unused parameter
| ^ unused type parameter
|
= help: consider removing `T`, referring to it in a field, or using a marker such as `PhantomData`
= help: if you intended `T` to be a const parameter, use `const T: usize` instead
= help: if you intended `T` to be a const parameter, use `const T: /* Type */` instead

error: aborting due to 2 previous errors

Expand Down
6 changes: 3 additions & 3 deletions tests/ui/const-generics/issues/issue-67945-1.min.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ LL | let b = &*(&x as *const _ as *const S);
= note: type parameters may not be used in const expressions
= help: add `#![feature(generic_const_exprs)]` to allow generic const expressions

error[E0392]: parameter `S` is never used
error[E0392]: type parameter `S` is never used
--> $DIR/issue-67945-1.rs:7:12
|
LL | struct Bug<S> {
| ^ unused parameter
| ^ unused type parameter
|
= help: consider removing `S`, referring to it in a field, or using a marker such as `PhantomData`
= help: if you intended `S` to be a const parameter, use `const S: usize` instead
= help: if you intended `S` to be a const parameter, use `const S: /* Type */` instead

error: aborting due to 3 previous errors

Expand Down
6 changes: 3 additions & 3 deletions tests/ui/const-generics/issues/issue-67945-3.min.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ LL | let x: Option<S> = None;
= note: type parameters may not be used in const expressions
= help: add `#![feature(generic_const_exprs)]` to allow generic const expressions

error[E0392]: parameter `S` is never used
error[E0392]: type parameter `S` is never used
--> $DIR/issue-67945-3.rs:9:12
|
LL | struct Bug<S> {
| ^ unused parameter
| ^ unused type parameter
|
= help: consider removing `S`, referring to it in a field, or using a marker such as `PhantomData`
= help: if you intended `S` to be a const parameter, use `const S: usize` instead
= help: if you intended `S` to be a const parameter, use `const S: /* Type */` instead

error: aborting due to 2 previous errors

Expand Down
6 changes: 3 additions & 3 deletions tests/ui/const-generics/issues/issue-67945-4.min.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ LL | let x: Option<Box<S>> = None;
= note: type parameters may not be used in const expressions
= help: add `#![feature(generic_const_exprs)]` to allow generic const expressions

error[E0392]: parameter `S` is never used
error[E0392]: type parameter `S` is never used
--> $DIR/issue-67945-4.rs:8:12
|
LL | struct Bug<S> {
| ^ unused parameter
| ^ unused type parameter
|
= help: consider removing `S`, referring to it in a field, or using a marker such as `PhantomData`
= help: if you intended `S` to be a const parameter, use `const S: usize` instead
= help: if you intended `S` to be a const parameter, use `const S: /* Type */` instead

error: aborting due to 2 previous errors

Expand Down
Loading
Loading