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

Add a new linting pass for obligations #96092

Closed
wants to merge 1 commit into from
Closed
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
30 changes: 28 additions & 2 deletions compiler/rustc_data_structures/src/obligation_forest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ pub trait ObligationProcessor {
fn process_obligation(
&mut self,
obligation: &mut Self::Obligation,
tracer: impl Fn() -> Vec<Self::Obligation>,
) -> ProcessResult<Self::Obligation, Self::Error>;

/// As we do the cycle check, we invoke this callback when we
Expand Down Expand Up @@ -436,7 +437,11 @@ impl<O: ForestObligation> ObligationForest<O> {
// be computed with the initial length, and we would miss the appended
// nodes. Therefore we use a `while` loop.
let mut index = 0;
while let Some(node) = self.nodes.get_mut(index) {
while index < self.nodes.len() {
let (nodes, [node, ..]) = self.nodes.split_at_mut(index) else {
todo!("FIXME(skippy)")
};

// `processor.process_obligation` can modify the predicate within
// `node.obligation`, and that predicate is the key used for
// `self.active_cache`. This means that `self.active_cache` can get
Expand All @@ -447,7 +452,28 @@ impl<O: ForestObligation> ObligationForest<O> {
continue;
}

match processor.process_obligation(&mut node.obligation) {
let parent_index = if node.has_parent { Some(node.dependents[0]) } else { None };
let tracer = || {
let mut trace = vec![];
if let Some(mut index) = parent_index {
loop {
let node = &nodes[index];
trace.push(node.obligation.clone());
if node.has_parent {
// The first dependent is the parent, which is treated
// specially.
index = node.dependents[0];
} else {
// No parent; treat all dependents non-specially.
break;
}
}
}

trace
};

match processor.process_obligation(&mut node.obligation, tracer) {
ProcessResult::Unchanged => {
// No change in state.
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ where
fn process_obligation(
&mut self,
obligation: &mut Self::Obligation,
_tracer: impl Fn() -> Vec<Self::Obligation>,
) -> ProcessResult<Self::Obligation, Self::Error> {
(self.process_obligation)(obligation)
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_infer/src/infer/at.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
err_count_on_creation: self.err_count_on_creation,
in_snapshot: self.in_snapshot.clone(),
universe: self.universe.clone(),
query_mode: self.query_mode.clone(),
}
}
}
Expand Down
38 changes: 33 additions & 5 deletions compiler/rustc_infer/src/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,21 @@ pub struct InferCtxt<'a, 'tcx> {
/// when we enter into a higher-ranked (`for<..>`) type or trait
/// bound.
universe: Cell<ty::UniverseIndex>,

pub query_mode: TraitQueryMode,
}

/// The mode that trait queries run in.
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum TraitQueryMode {
/// Standard/un-canonicalized queries get accurate
/// spans etc. passed in and hence can do reasonable
/// error reporting on their own.
Standard,
/// Canonicalized queries get dummy spans and hence
/// must generally propagate errors to
/// pre-canonicalization callsites.
Canonical,
}

/// See the `error_reporting` module for more details.
Expand Down Expand Up @@ -615,14 +630,26 @@ impl<'tcx> InferCtxtBuilder<'tcx> {
where
T: TypeFoldable<'tcx>,
{
self.enter(|infcx| {
let (value, subst) =
infcx.instantiate_canonical_with_fresh_inference_vars(span, canonical);
f(infcx, value, subst)
})
self.enter_with_query_mode(
|infcx| {
let (value, subst) =
infcx.instantiate_canonical_with_fresh_inference_vars(span, canonical);
f(infcx, value, subst)
},
TraitQueryMode::Canonical,
)
}

pub fn enter<R>(&mut self, f: impl for<'a> FnOnce(InferCtxt<'a, 'tcx>) -> R) -> R {
self.enter_with_query_mode(f, TraitQueryMode::Standard)
}

// FIXME(skippy) specifically needs reviewer feedback (see query_mode field)
fn enter_with_query_mode<R>(
&mut self,
f: impl for<'a> FnOnce(InferCtxt<'a, 'tcx>) -> R,
query_mode: TraitQueryMode,
) -> R {
let InferCtxtBuilder { tcx, defining_use_anchor, ref fresh_typeck_results } = *self;
let in_progress_typeck_results = fresh_typeck_results.as_ref();
f(InferCtxt {
Expand All @@ -640,6 +667,7 @@ impl<'tcx> InferCtxtBuilder<'tcx> {
in_snapshot: Cell::new(false),
skip_leak_check: Cell::new(false),
universe: Cell::new(ty::UniverseIndex::ROOT),
query_mode,
})
}
}
Expand Down
18 changes: 12 additions & 6 deletions compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use crate::infer::canonical::OriginalQueryValues;
use crate::infer::InferCtxt;
use crate::traits::error_reporting::warnings::InferCtxtExt as _;
use crate::traits::query::NoSolution;
use crate::traits::{
ChalkEnvironmentAndGoal, FulfillmentError, FulfillmentErrorCode, ObligationCause,
Expand Down Expand Up @@ -109,12 +110,17 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> {
&orig_values,
&response,
) {
Ok(infer_ok) => next_round.extend(
infer_ok.obligations.into_iter().map(|obligation| {
assert!(!infcx.is_in_snapshot());
infcx.resolve_vars_if_possible(obligation)
}),
),
Ok(infer_ok) => {
infcx.check_obligation_lints(&obligation, || {
vec![obligation.clone()]
});
next_round.extend(infer_ok.obligations.into_iter().map(
|obligation| {
assert!(!infcx.is_in_snapshot());
infcx.resolve_vars_if_possible(obligation)
},
))
}

Err(_err) => errors.push(FulfillmentError {
obligation: obligation.clone(),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
pub mod on_unimplemented;
pub mod suggestions;
pub mod warnings;

use super::{
DerivedObligationCause, EvaluationResult, FulfillmentContext, FulfillmentError,
Expand Down Expand Up @@ -2067,7 +2068,7 @@ impl<'a, 'tcx> InferCtxtPrivExt<'a, 'tcx> for InferCtxt<'a, 'tcx> {
);
let mut selcx = SelectionContext::with_query_mode(
&self,
crate::traits::TraitQueryMode::Standard,
crate::infer::TraitQueryMode::Standard,
);
match selcx.select_from_obligation(&obligation) {
Err(SelectionError::Ambiguous(impls)) if impls.len() > 1 => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
use super::PredicateObligation;

use crate::infer::InferCtxt;

use hir::{ItemKind, TyKind};
use rustc_hir as hir;
use rustc_hir::Node;
use rustc_infer::infer::TraitQueryMode;
use rustc_lint_defs::builtin::DEPRECATED_IN_FUTURE;
use rustc_middle::ty::{self, Binder};

pub trait InferCtxtExt<'tcx> {
fn check_obligation_lints(
&self,
obligation: &PredicateObligation<'tcx>,
tracer: impl Fn() -> Vec<PredicateObligation<'tcx>>,
);
}

impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
fn check_obligation_lints(
&self,
obligation: &PredicateObligation<'tcx>,
tracer: impl Fn() -> Vec<PredicateObligation<'tcx>>,
) {
if self.query_mode != TraitQueryMode::Standard {
return;
}

if let Some(trait_obligation) = {
let binder = obligation.predicate.kind();
match binder.no_bound_vars() {
None => match binder.skip_binder() {
ty::PredicateKind::Trait(data) => Some(obligation.with(binder.rebind(data))),
_ => None,
},
Some(pred) => match pred {
ty::PredicateKind::Trait(data) => Some(obligation.with(Binder::dummy(data))),
_ => None,
},
}
} {
let self_ty = trait_obligation.self_ty().skip_binder();
let trait_ref = trait_obligation.predicate.skip_binder().trait_ref;

// FIXME(skippy) lint experiment: deprecate PartialEq on function pointers
if let ty::FnPtr(_) = self_ty.kind()
&& self.tcx.lang_items().eq_trait() == Some(trait_ref.def_id)
{
let node = self.tcx.hir().get(trait_obligation.cause.body_id);
let mut skip = false;
if let Node::Item(item) = node
&& let ItemKind::Impl(impl_item) = &item.kind
&& let TyKind::BareFn(_) = impl_item.self_ty.kind
{
skip = true;
}
if !skip {
let root_obligation = tracer().last().unwrap_or(&obligation).clone();
self.tcx.struct_span_lint_hir(
DEPRECATED_IN_FUTURE,
root_obligation.cause.body_id,
root_obligation.cause.span,
|lint| {
lint.build(
"FIXME(skippy) PartialEq on function pointers has been deprecated.",
)
.emit();
},
);
}
}
}
}
}
9 changes: 8 additions & 1 deletion compiler/rustc_trait_selection/src/traits/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use super::Unimplemented;
use super::{FulfillmentError, FulfillmentErrorCode};
use super::{ObligationCause, PredicateObligation};

use crate::traits::error_reporting::warnings::InferCtxtExt as _;
use crate::traits::error_reporting::InferCtxtExt as _;
use crate::traits::project::PolyProjectionObligation;
use crate::traits::project::ProjectionCacheKeyExt as _;
Expand Down Expand Up @@ -275,6 +276,7 @@ impl<'a, 'b, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'tcx> {
fn process_obligation(
&mut self,
pending_obligation: &mut Self::Obligation,
tracer: impl Fn() -> Vec<Self::Obligation>,
) -> ProcessResult<Self::Obligation, Self::Error> {
// If we were stalled on some unresolved variables, first check whether
// any of them have been resolved; if not, don't bother doing more work
Expand Down Expand Up @@ -313,7 +315,12 @@ impl<'a, 'b, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'tcx> {
return ProcessResult::Unchanged;
}

self.process_changed_obligations(pending_obligation)
let result = self.process_changed_obligations(pending_obligation);
if let ProcessResult::Changed(_) = result {
let tracer = || tracer().into_iter().map(|x| x.obligation).collect();
self.selcx.infcx().check_obligation_lints(&pending_obligation.obligation, tracer);
}
result
}

fn process_backedge<'c, I>(
Expand Down
13 changes: 0 additions & 13 deletions compiler/rustc_trait_selection/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,19 +98,6 @@ impl SkipLeakCheck {
}
}

/// The mode that trait queries run in.
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum TraitQueryMode {
/// Standard/un-canonicalized queries get accurate
/// spans etc. passed in and hence can do reasonable
/// error reporting on their own.
Standard,
/// Canonicalized queries get dummy spans and hence
/// must generally propagate errors to
/// pre-canonicalization callsites.
Canonical,
}

/// Creates predicate obligations from the generic bounds.
pub fn predicates_for_generics<'tcx>(
cause: ObligationCause<'tcx>,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
use rustc_middle::ty;

use crate::infer::canonical::OriginalQueryValues;
use crate::infer::InferCtxt;
use crate::traits::{
EvaluationResult, OverflowError, PredicateObligation, SelectionContext, TraitQueryMode,
};
use crate::infer::{InferCtxt, TraitQueryMode};
use crate::traits::{EvaluationResult, OverflowError, PredicateObligation, SelectionContext};

pub trait InferCtxtExt<'tcx> {
fn predicate_may_hold(&self, obligation: &PredicateObligation<'tcx>) -> bool;
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ use super::wf;
use super::{
DerivedObligationCause, ErrorReporting, ImplDerivedObligation, ImplDerivedObligationCause,
Normalized, Obligation, ObligationCause, ObligationCauseCode, Overflow, PredicateObligation,
Selection, SelectionError, SelectionResult, TraitObligation, TraitQueryMode,
Selection, SelectionError, SelectionResult, TraitObligation,
};

use crate::infer::{InferCtxt, InferOk, TypeFreshener};
use crate::infer::{InferCtxt, InferOk, TraitQueryMode, TypeFreshener};
use crate::traits::error_reporting::InferCtxtExt;
use crate::traits::project::ProjectAndUnifyResult;
use crate::traits::project::ProjectionCacheKeyExt;
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_traits/src/evaluate_obligation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ use rustc_infer::infer::TyCtxtInferExt;
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{ParamEnvAnd, TyCtxt};
use rustc_span::source_map::DUMMY_SP;
use rustc_trait_selection::infer::TraitQueryMode;
use rustc_trait_selection::traits::query::CanonicalPredicateGoal;
use rustc_trait_selection::traits::{
EvaluationResult, Obligation, ObligationCause, OverflowError, SelectionContext, TraitQueryMode,
EvaluationResult, Obligation, ObligationCause, OverflowError, SelectionContext,
};

crate fn provide(p: &mut Providers) {
Expand Down
17 changes: 12 additions & 5 deletions compiler/rustc_typeck/src/check/coercion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ use rustc_session::parse::feature_err;
use rustc_span::symbol::sym;
use rustc_span::{self, BytePos, DesugaringKind, Span};
use rustc_target::spec::abi::Abi;
use rustc_trait_selection::traits::error_reporting::InferCtxtExt;
use rustc_trait_selection::traits::error_reporting::warnings::InferCtxtExt as _;
use rustc_trait_selection::traits::error_reporting::InferCtxtExt as _;
use rustc_trait_selection::traits::{self, ObligationCause, ObligationCauseCode};

use smallvec::{smallvec, SmallVec};
Expand Down Expand Up @@ -604,15 +605,17 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
// and almost never more than 3. By using a SmallVec we avoid an
// allocation, at the (very small) cost of (occasionally) having to
// shift subsequent elements down when removing the front element.
let mut queue: SmallVec<[_; 4]> = smallvec![traits::predicate_for_trait_def(
let root_obligation = traits::predicate_for_trait_def(
self.tcx,
self.fcx.param_env,
cause,
coerce_unsized_did,
0,
coerce_source,
&[coerce_target.into()]
)];
&[coerce_target.into()],
);
// FIXME(skippy) extra .clone() here due to root_obligation
let mut queue: SmallVec<[_; 4]> = smallvec![root_obligation.clone()];

let mut has_unsized_tuple_coercion = false;
let mut has_trait_upcasting_coercion = false;
Expand Down Expand Up @@ -692,7 +695,11 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
// be silent, as it causes a type mismatch later.
}

Ok(Some(impl_source)) => queue.extend(impl_source.nested_obligations()),
Ok(Some(impl_source)) => {
// FIXME(skippy) just has the root_obligation, not the entire trace
self.check_obligation_lints(&obligation, || vec![root_obligation.clone()]);
queue.extend(impl_source.nested_obligations());
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions library/core/src/task/wake.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ impl RawWaker {
/// any other `data` pointer will cause undefined behavior.
#[stable(feature = "futures_api", since = "1.36.0")]
#[derive(PartialEq, Copy, Clone, Debug)]
// FIXME(skippy) for PartialEq on function pointers
#[allow(deprecated_in_future)]
pub struct RawWakerVTable {
/// This function will be called when the [`RawWaker`] gets cloned, e.g. when
/// the [`Waker`] in which the [`RawWaker`] is stored gets cloned.
Expand Down
Loading