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.

Sign up for GitHub

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

Jump to bottom

Make IMPLIED_BOUNDS_ENTAILMENT into a hard error from a lint #117984

Merged
merged 2 commits into from
Dec 16, 2023
Merged

Make IMPLIED_BOUNDS_ENTAILMENT into a hard error from a lint #117984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
629d351
Make IMPLIED_BOUNDS_ENTAILMENT into a hard error from a lint
compiler-errors Nov 16, 2023
32907c7
Remove the lint outright
compiler-errors Nov 16, 2023
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
201 changes: 12 additions & 189 deletions compiler/rustc_hir_analysis/src/check/compare_impl_item.rs
View file
Open in desktop
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()
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't tainted_by_errors always be None, given that we return in case select_all_or_error failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

idk, astconv might taint the infcx or something. I didn't want to unwrap here, avoids an unnecessary ICE for no cost.

.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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading