Skip to content

Commit

Permalink
Auto merge of #117984 - compiler-errors:implied-bounds-entailment, r=…
Browse files Browse the repository at this point in the history 
…lcnr

Make `IMPLIED_BOUNDS_ENTAILMENT` into a hard error from a lint

closes #105572

Removes the `IMPLIED_BOUNDS_ENTAILMENT` and makes the `compare_method_predicate_entailment` logic just run once.

r? lcnr
  • Loading branch information
@bors
bors committed Dec 16, 2023
2 parents ca5c68a + 32907c7 commit 5c927ab
Show file tree
Hide file tree
Showing 9 changed files with 95 additions and 299 deletions.
201 changes: 12 additions & 189 deletions compiler/rustc_hir_analysis/src/check/compare_impl_item.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@ use super::potentially_plural_count;
use crate::errors::LifetimesOrBoundsMismatchOnTrait;
use hir::def_id::{DefId, LocalDefId};
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet};
use rustc_errors::{
pluralize, struct_span_err, Applicability, DiagnosticId, ErrorGuaranteed, MultiSpan,
};
use rustc_errors::{pluralize, struct_span_err, Applicability, DiagnosticId, ErrorGuaranteed};
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::intravisit;
Expand Down Expand Up @@ -50,13 +48,7 @@ pub(super) fn compare_impl_method<'tcx>(

let _: Result<_, ErrorGuaranteed> = try {
check_method_is_structurally_compatible(tcx, impl_m, trait_m, impl_trait_ref, false)?;
compare_method_predicate_entailment(
tcx,
impl_m,
trait_m,
impl_trait_ref,
CheckImpliedWfMode::Check,
)?;
compare_method_predicate_entailment(tcx, impl_m, trait_m, impl_trait_ref)?;
refine::check_refining_return_position_impl_trait_in_trait(
tcx,
impl_m,
Expand Down Expand Up @@ -170,7 +162,6 @@ fn compare_method_predicate_entailment<'tcx>(
impl_m: ty::AssocItem,
trait_m: ty::AssocItem,
impl_trait_ref: ty::TraitRef<'tcx>,
check_implied_wf: CheckImpliedWfMode,
) -> Result<(), ErrorGuaranteed> {
let trait_to_impl_args = impl_trait_ref.args;

Expand Down Expand Up @@ -317,7 +308,7 @@ fn compare_method_predicate_entailment<'tcx>(
return Err(emitted);
}

if check_implied_wf == CheckImpliedWfMode::Check && !(impl_sig, trait_sig).references_error() {
if !(impl_sig, trait_sig).references_error() {
// Select obligations to make progress on inference before processing
// the wf obligation below.
// FIXME(-Znext-solver): Not needed when the hack below is removed.
Expand All @@ -333,8 +324,9 @@ fn compare_method_predicate_entailment<'tcx>(
// trigger the lint. Instead, let's only consider type outlives and
// region outlives obligations.
//
// FIXME(-Znext-solver): Try removing this hack again once
// the new solver is stable.
// FIXME(-Znext-solver): Try removing this hack again once the new
// solver is stable. We should just be able to register a WF pred for
// the fn sig.
let mut wf_args: smallvec::SmallVec<[_; 4]> =
unnormalized_impl_sig.inputs_and_output.iter().map(|ty| ty.into()).collect();
// Annoyingly, asking for the WF predicates of an array (with an unevaluated const (only?))
Expand All @@ -357,7 +349,7 @@ fn compare_method_predicate_entailment<'tcx>(
// We need to register Projection oblgiations too, because we may end up with
// an implied `X::Item: 'a`, which gets desugared into `X::Item = ?0`, `?0: 'a`.
// If we only register the region outlives obligation, this leads to an unconstrained var.
// See `implied_bounds_entailment_alias_var` test.
// See `implied_bounds_entailment_alias_var.rs` test.
ty::PredicateKind::Clause(
ty::ClauseKind::RegionOutlives(..)
| ty::ClauseKind::TypeOutlives(..)
Expand All @@ -378,26 +370,8 @@ fn compare_method_predicate_entailment<'tcx>(
// version.
let errors = ocx.select_all_or_error();
if !errors.is_empty() {
match check_implied_wf {
CheckImpliedWfMode::Check => {
let impl_m_hir_id = tcx.local_def_id_to_hir_id(impl_m_def_id);
return compare_method_predicate_entailment(
tcx,
impl_m,
trait_m,
impl_trait_ref,
CheckImpliedWfMode::Skip,
)
.map(|()| {
// If the skip-mode was successful, emit a lint.
emit_implied_wf_lint(infcx.tcx, impl_m, impl_m_hir_id, vec![]);
});
}
CheckImpliedWfMode::Skip => {
let reported = infcx.err_ctxt().report_fulfillment_errors(errors);
return Err(reported);
}
}
let reported = infcx.err_ctxt().report_fulfillment_errors(errors);
return Err(reported);
}

// Finally, resolve all regions. This catches wily misuses of
Expand All @@ -408,119 +382,14 @@ fn compare_method_predicate_entailment<'tcx>(
);
let errors = infcx.resolve_regions(&outlives_env);
if !errors.is_empty() {
// FIXME(compiler-errors): This can be simplified when IMPLIED_BOUNDS_ENTAILMENT
// becomes a hard error (i.e. ideally we'd just call `resolve_regions_and_report_errors`
let impl_m_hir_id = tcx.local_def_id_to_hir_id(impl_m_def_id);
match check_implied_wf {
CheckImpliedWfMode::Check => {
return compare_method_predicate_entailment(
tcx,
impl_m,
trait_m,
impl_trait_ref,
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(tcx, impl_m, impl_m_hir_id, bad_args);
});
}
CheckImpliedWfMode::Skip => {
if infcx.tainted_by_errors().is_none() {
infcx.err_ctxt().report_region_errors(impl_m_def_id, &errors);
}
return Err(tcx
.sess
.span_delayed_bug(rustc_span::DUMMY_SP, "error should have been emitted"));
}
}
return Err(infcx
.tainted_by_errors()
.unwrap_or_else(|| infcx.err_ctxt().report_region_errors(impl_m_def_id, &errors)));
}

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).skip_binder().bound_vars(),
tcx.fn_sig(impl_m.def_id).skip_binder().bound_vars(),
)
.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>,
Expand All @@ -544,52 +413,6 @@ impl<'tcx> TypeFolder<TyCtxt<'tcx>> for RemapLateBound<'_, 'tcx> {
}
}

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,
);
}
},
);
}

#[derive(Debug, PartialEq, Eq)]
enum CheckImpliedWfMode {
/// Checks implied well-formedness of the impl method. If it fails, we will
/// re-check with `Skip`, and emit a lint if it succeeds.
Check,
/// Skips checking implied well-formedness of the impl method, but will emit
/// a lint if the `compare_method_predicate_entailment` succeeded. This means that
/// the reason that we had failed earlier during `Check` was due to the impl
/// having stronger requirements than the trait.
Skip,
}

fn compare_asyncness<'tcx>(
tcx: TyCtxt<'tcx>,
impl_m: ty::AssocItem,
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_lint/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,11 @@ fn register_builtins(store: &mut LintStore) {
"converted into hard error, see PR #104616 \
<https://github.com/rust-lang/rust/pull/104616> for more information",
);
store.register_removed(
"implied_bounds_entailment",
"converted into hard error, see PR #117984 \
<https://github.com/rust-lang/rust/pull/117984> for more information",
);
}

fn register_internals(store: &mut LintStore) {
Expand Down
42 changes: 0 additions & 42 deletions compiler/rustc_lint_defs/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ declare_lint_pass! {
HIDDEN_GLOB_REEXPORTS,
ILL_FORMED_ATTRIBUTE_INPUT,
ILLEGAL_FLOATING_POINT_LITERAL_PATTERN,
IMPLIED_BOUNDS_ENTAILMENT,
INCOMPLETE_INCLUDE,
INDIRECT_STRUCTURAL_MATCH,
INEFFECTIVE_UNSTABLE_TRAIT_IMPL,
Expand Down Expand Up @@ -4235,47 +4234,6 @@ declare_lint! {
"named arguments in format used positionally"
}

declare_lint! {
/// The `implied_bounds_entailment` lint detects cases where the arguments of an impl method
/// have stronger implied bounds than those from the trait method it's implementing.
///
/// ### Example
///
/// ```rust,compile_fail
/// #![deny(implied_bounds_entailment)]
///
/// trait Trait {
/// fn get<'s>(s: &'s str, _: &'static &'static ()) -> &'static str;
/// }
///
/// impl Trait for () {
/// fn get<'s>(s: &'s str, _: &'static &'s ()) -> &'static str {
/// s
/// }
/// }
///
/// let val = <() as Trait>::get(&String::from("blah blah blah"), &&());
/// println!("{}", val);
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Neither the trait method, which provides no implied bounds about `'s`, nor the impl,
/// requires the main function to prove that 's: 'static, but the impl method is allowed
/// to assume that `'s: 'static` within its own body.
///
/// This can be used to implement an unsound API if used incorrectly.
pub IMPLIED_BOUNDS_ENTAILMENT,
Deny,
"impl method assumes more implied bounds than its corresponding trait method",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps,
reference: "issue #105572 <https://github.com/rust-lang/rust/issues/105572>",
};
}

declare_lint! {
/// The `byte_slice_in_packed_struct_with_derive` lint detects cases where a byte slice field
/// (`[u8]`) or string slice field (`str`) is used in a `packed` struct that derives one or
Expand Down
5 changes: 1 addition & 4 deletions tests/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
#![deny(implied_bounds_entailment)]

trait Project {
type Ty;
}
Expand All @@ -11,8 +9,7 @@ trait Trait {
}
impl Trait for () {
fn get<'s>(s: &'s str, _: <&'static &'s () as Project>::Ty) -> &'static str {
//~^ ERROR impl method assumes more implied bounds than the corresponding trait method
//~| WARN this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
//~^ ERROR cannot infer an appropriate lifetime for lifetime parameter 's in generic type due to conflicting requirements
s
}
}
Expand Down
Loading

0 comments on commit 5c927ab

Please sign in to comment.