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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump IMPLIED_BOUNDS_ENTAILMENT to Deny + ReportNow #106465

Merged
merged 2 commits into from Jan 14, 2023
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
160 changes: 148 additions & 12 deletions compiler/rustc_hir_analysis/src/check/compare_impl_item.rs
Expand Up @@ -2,7 +2,9 @@ use super::potentially_plural_count;
use crate::errors::LifetimesOrBoundsMismatchOnTrait;
use hir::def_id::{DefId, LocalDefId};
use rustc_data_structures::fx::{FxHashMap, FxIndexSet};
use rustc_errors::{pluralize, struct_span_err, Applicability, DiagnosticId, ErrorGuaranteed};
use rustc_errors::{
pluralize, struct_span_err, Applicability, DiagnosticId, ErrorGuaranteed, MultiSpan,
};
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::intravisit;
Expand Down Expand Up @@ -320,15 +322,6 @@ fn compare_method_predicate_entailment<'tcx>(
ty::Binder::dummy(ty::PredicateKind::WellFormed(unnormalized_impl_fty.into())),
));
}
let emit_implied_wf_lint = || {
infcx.tcx.struct_span_lint_hir(
rustc_session::lint::builtin::IMPLIED_BOUNDS_ENTAILMENT,
impl_m_hir_id,
infcx.tcx.def_span(impl_m.def_id),
"impl method assumes more implied bounds than the corresponding trait method",
|lint| lint,
);
};

// Check that all obligations are satisfied by the implementation's
// version.
Expand All @@ -346,7 +339,7 @@ fn compare_method_predicate_entailment<'tcx>(
)
.map(|()| {
// If the skip-mode was successful, emit a lint.
emit_implied_wf_lint();
emit_implied_wf_lint(infcx.tcx, impl_m, impl_m_hir_id, vec![]);
});
}
CheckImpliedWfMode::Skip => {
Expand Down Expand Up @@ -382,8 +375,16 @@ fn compare_method_predicate_entailment<'tcx>(
CheckImpliedWfMode::Skip,
)
.map(|()| {
let bad_args = extract_bad_args_for_implies_lint(
tcx,
&errors,
(trait_m, trait_sig),
// Unnormalized impl sig corresponds to the HIR types written
(impl_m, unnormalized_impl_sig),
impl_m_hir_id,
);
// If the skip-mode was successful, emit a lint.
emit_implied_wf_lint();
emit_implied_wf_lint(tcx, impl_m, impl_m_hir_id, bad_args);
});
}
CheckImpliedWfMode::Skip => {
Expand All @@ -400,6 +401,141 @@ fn compare_method_predicate_entailment<'tcx>(
Ok(())
}

fn extract_bad_args_for_implies_lint<'tcx>(
tcx: TyCtxt<'tcx>,
errors: &[infer::RegionResolutionError<'tcx>],
(trait_m, trait_sig): (&ty::AssocItem, ty::FnSig<'tcx>),
(impl_m, impl_sig): (&ty::AssocItem, ty::FnSig<'tcx>),
hir_id: hir::HirId,
) -> Vec<(Span, Option<String>)> {
let mut blame_generics = vec![];
for error in errors {
// Look for the subregion origin that contains an input/output type
let origin = match error {
infer::RegionResolutionError::ConcreteFailure(o, ..) => o,
infer::RegionResolutionError::GenericBoundFailure(o, ..) => o,
infer::RegionResolutionError::SubSupConflict(_, _, o, ..) => o,
infer::RegionResolutionError::UpperBoundUniverseConflict(.., o, _) => o,
};
// Extract (possible) input/output types from origin
match origin {
infer::SubregionOrigin::Subtype(trace) => {
if let Some((a, b)) = trace.values.ty() {
blame_generics.extend([a, b]);
}
}
infer::SubregionOrigin::RelateParamBound(_, ty, _) => blame_generics.push(*ty),
infer::SubregionOrigin::ReferenceOutlivesReferent(ty, _) => blame_generics.push(*ty),
_ => {}
}
}

let fn_decl = tcx.hir().fn_decl_by_hir_id(hir_id).unwrap();
let opt_ret_ty = match fn_decl.output {
hir::FnRetTy::DefaultReturn(_) => None,
hir::FnRetTy::Return(ty) => Some(ty),
};

// Map late-bound regions from trait to impl, so the names are right.
let mapping = std::iter::zip(
tcx.fn_sig(trait_m.def_id).bound_vars(),
tcx.fn_sig(impl_m.def_id).bound_vars(),
)
Comment on lines +440 to +443
Copy link
Contributor

Choose a reason for hiding this comment

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

that's wrong in subtle ways because impls aren't required to order their bound vars in the same way. That seems fine considering that this lint will only exist for a few releases and works correctly for all the issues we've found via crater.

.filter_map(|(impl_bv, trait_bv)| {
if let ty::BoundVariableKind::Region(impl_bv) = impl_bv
&& let ty::BoundVariableKind::Region(trait_bv) = trait_bv
{
Some((impl_bv, trait_bv))
} else {
None
}
})
.collect();

// For each arg, see if it was in the "blame" of any of the region errors.
// If so, then try to produce a suggestion to replace the argument type with
// one from the trait.
let mut bad_args = vec![];
for (idx, (ty, hir_ty)) in
std::iter::zip(impl_sig.inputs_and_output, fn_decl.inputs.iter().chain(opt_ret_ty))
.enumerate()
{
let expected_ty = trait_sig.inputs_and_output[idx]
.fold_with(&mut RemapLateBound { tcx, mapping: &mapping });
if blame_generics.iter().any(|blame| ty.contains(*blame)) {
let expected_ty_sugg = expected_ty.to_string();
bad_args.push((
hir_ty.span,
// Only suggest something if it actually changed.
(expected_ty_sugg != ty.to_string()).then_some(expected_ty_sugg),
));
}
}

bad_args
}

struct RemapLateBound<'a, 'tcx> {
tcx: TyCtxt<'tcx>,
mapping: &'a FxHashMap<ty::BoundRegionKind, ty::BoundRegionKind>,
}

impl<'tcx> TypeFolder<'tcx> for RemapLateBound<'_, 'tcx> {
fn tcx(&self) -> TyCtxt<'tcx> {
self.tcx
}

fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> {
if let ty::ReFree(fr) = *r {
self.tcx.mk_region(ty::ReFree(ty::FreeRegion {
bound_region: self
.mapping
.get(&fr.bound_region)
.copied()
.unwrap_or(fr.bound_region),
..fr
}))
} else {
r
}
}
}

fn emit_implied_wf_lint<'tcx>(
tcx: TyCtxt<'tcx>,
impl_m: &ty::AssocItem,
hir_id: hir::HirId,
bad_args: Vec<(Span, Option<String>)>,
) {
let span: MultiSpan = if bad_args.is_empty() {
tcx.def_span(impl_m.def_id).into()
} else {
bad_args.iter().map(|(span, _)| *span).collect::<Vec<_>>().into()
};
tcx.struct_span_lint_hir(
rustc_session::lint::builtin::IMPLIED_BOUNDS_ENTAILMENT,
hir_id,
span,
"impl method assumes more implied bounds than the corresponding trait method",
|lint| {
let bad_args: Vec<_> =
bad_args.into_iter().filter_map(|(span, sugg)| Some((span, sugg?))).collect();
if !bad_args.is_empty() {
lint.multipart_suggestion(
format!(
"replace {} type{} to make the impl signature compatible",
pluralize!("this", bad_args.len()),
pluralize!(bad_args.len())
),
bad_args,
Applicability::MaybeIncorrect,
);
}
lint
},
);
}

#[derive(Debug, PartialEq, Eq)]
enum CheckImpliedWfMode {
/// Checks implied well-formedness of the impl method. If it fails, we will
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_lint_defs/src/builtin.rs
Expand Up @@ -4033,10 +4033,10 @@ declare_lint! {
///
/// This can be used to implement an unsound API if used incorrectly.
pub IMPLIED_BOUNDS_ENTAILMENT,
Warn,
Deny,
"impl method assumes more implied bounds than its corresponding trait method",
@future_incompatible = FutureIncompatibleInfo {
reference: "issue #105572 <https://github.com/rust-lang/rust/issues/105572>",
reason: FutureIncompatibilityReason::FutureReleaseError,
reason: FutureIncompatibilityReason::FutureReleaseErrorReportNow,
};
}
@@ -1,8 +1,8 @@
error: impl method assumes more implied bounds than the corresponding trait method
--> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:13:5
--> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:13:31
|
LL | fn get<'s>(s: &'s str, _: <&'static &'s () as Project>::Ty) -> &'static str {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace this type to make the impl signature compatible: `()`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #105572 <https://github.com/rust-lang/rust/issues/105572>
Expand All @@ -14,3 +14,18 @@ LL | #![deny(implied_bounds_entailment)]

error: aborting due to previous error

Future incompatibility report: Future breakage diagnostic:
error: impl method assumes more implied bounds than the corresponding trait method
--> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:13:31
|
LL | fn get<'s>(s: &'s str, _: <&'static &'s () as Project>::Ty) -> &'static str {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace this type to make the impl signature compatible: `()`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #105572 <https://github.com/rust-lang/rust/issues/105572>
note: the lint level is defined here
--> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:1:9
|
LL | #![deny(implied_bounds_entailment)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^

19 changes: 17 additions & 2 deletions tests/ui/implied-bounds/impl-implied-bounds-compatibility.stderr
@@ -1,8 +1,8 @@
error: impl method assumes more implied bounds than the corresponding trait method
--> $DIR/impl-implied-bounds-compatibility.rs:14:5
--> $DIR/impl-implied-bounds-compatibility.rs:14:35
|
LL | fn listeners<'b>(&'b self) -> &'a MessageListeners<'b> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace this type to make the impl signature compatible: `&'b MessageListeners<'b>`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #105572 <https://github.com/rust-lang/rust/issues/105572>
Expand All @@ -14,3 +14,18 @@ LL | #![deny(implied_bounds_entailment)]

error: aborting due to previous error

Future incompatibility report: Future breakage diagnostic:
error: impl method assumes more implied bounds than the corresponding trait method
--> $DIR/impl-implied-bounds-compatibility.rs:14:35
|
LL | fn listeners<'b>(&'b self) -> &'a MessageListeners<'b> {
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace this type to make the impl signature compatible: `&'b MessageListeners<'b>`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #105572 <https://github.com/rust-lang/rust/issues/105572>
note: the lint level is defined here
--> $DIR/impl-implied-bounds-compatibility.rs:1:9
|
LL | #![deny(implied_bounds_entailment)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^