Skip to content

Commit 7f0fa48

Browse files
committed
Auto merge of rust-lang#115582 - compiler-errors:refine-yeet, r=oli-obk
Implement refinement lint for RPITIT Implements a lint that warns against accidentally refining an RPITIT in an implementation. This is not a hard error, and can be suppressed with `#[allow(refining_impl_trait)]`, since this behavior may be desirable -- the lint just serves as an acknowledgement from the impl author that they understand that the types they write in the implementation are an API guarantee. This compares bounds syntactically, not semantically -- semantic implication is more difficult and essentially relies on adding the ability to keep the RPITIT hidden in the trait system so that things can be proven about the type that shows up in the impl without its own bounds leaking through, either via a new reveal mode or something else. This was experimentally implemented in rust-lang#111931. Somewhat opinionated choices: 1. Putting the lint behind `refining_impl_trait` rather than a blanket `refine` lint. This could be changed, but I like keeping the lint specialized to RPITITs so the explanation can be tailored to it. 2. This PR does not include the `#[refine]` attribute or the feature gate, since it's kind of orthogonal and can be added in a separate PR. r? `@oli-obk`
2 parents d5573d7 + 7714db8 commit 7f0fa48

26 files changed

+577
-45
lines changed

compiler/rustc_errors/src/lib.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ use rustc_fluent_macro::fluent_messages;
4444
pub use rustc_lint_defs::{pluralize, Applicability};
4545
use rustc_span::source_map::SourceMap;
4646
pub use rustc_span::ErrorGuaranteed;
47-
use rustc_span::{Loc, Span};
47+
use rustc_span::{Loc, Span, DUMMY_SP};
4848

4949
use std::borrow::Cow;
5050
use std::error::Report;
@@ -1754,7 +1754,7 @@ impl DelayedDiagnostic {
17541754
BacktraceStatus::Captured => {
17551755
let inner = &self.inner;
17561756
self.inner.subdiagnostic(DelayedAtWithNewline {
1757-
span: inner.span.primary_span().unwrap(),
1757+
span: inner.span.primary_span().unwrap_or(DUMMY_SP),
17581758
emitted_at: inner.emitted_at.clone(),
17591759
note: self.note,
17601760
});
@@ -1764,7 +1764,7 @@ impl DelayedDiagnostic {
17641764
_ => {
17651765
let inner = &self.inner;
17661766
self.inner.subdiagnostic(DelayedAtWithoutNewline {
1767-
span: inner.span.primary_span().unwrap(),
1767+
span: inner.span.primary_span().unwrap_or(DUMMY_SP),
17681768
emitted_at: inner.emitted_at.clone(),
17691769
note: self.note,
17701770
});

compiler/rustc_hir_analysis/messages.ftl

+6
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,12 @@ hir_analysis_return_type_notation_on_non_rpitit =
222222
.note = function returns `{$ty}`, which is not compatible with associated type return bounds
223223
.label = this function must be `async` or return `impl Trait`
224224
225+
hir_analysis_rpitit_refined = impl trait in impl method signature does not match trait method signature
226+
.suggestion = replace the return type so that it matches the trait
227+
.label = return type from trait method defined here
228+
.unmatched_bound_label = this bound is stronger than that defined on the trait
229+
.note = add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate
230+
225231
hir_analysis_self_in_impl_self =
226232
`Self` is not valid in the self type of an impl block
227233
.note = replace `Self` with a different type

compiler/rustc_hir_analysis/src/check/compare_impl_item.rs

+8
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ use rustc_trait_selection::traits::{
2828
use std::borrow::Cow;
2929
use std::iter;
3030

31+
mod refine;
32+
3133
/// Checks that a method from an impl conforms to the signature of
3234
/// the same method as declared in the trait.
3335
///
@@ -53,6 +55,12 @@ pub(super) fn compare_impl_method<'tcx>(
5355
impl_trait_ref,
5456
CheckImpliedWfMode::Check,
5557
)?;
58+
refine::check_refining_return_position_impl_trait_in_trait(
59+
tcx,
60+
impl_m,
61+
trait_m,
62+
impl_trait_ref,
63+
);
5664
};
5765
}
5866

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,311 @@
1+
use rustc_data_structures::fx::FxIndexSet;
2+
use rustc_hir as hir;
3+
use rustc_hir::def_id::DefId;
4+
use rustc_infer::infer::{outlives::env::OutlivesEnvironment, TyCtxtInferExt};
5+
use rustc_lint_defs::builtin::REFINING_IMPL_TRAIT;
6+
use rustc_middle::traits::{ObligationCause, Reveal};
7+
use rustc_middle::ty::{
8+
self, Ty, TyCtxt, TypeFoldable, TypeSuperVisitable, TypeVisitable, TypeVisitor,
9+
};
10+
use rustc_span::{Span, DUMMY_SP};
11+
use rustc_trait_selection::traits::{
12+
elaborate, normalize_param_env_or_error, outlives_bounds::InferCtxtExt, ObligationCtxt,
13+
};
14+
use std::ops::ControlFlow;
15+
16+
/// Check that an implementation does not refine an RPITIT from a trait method signature.
17+
pub(super) fn check_refining_return_position_impl_trait_in_trait<'tcx>(
18+
tcx: TyCtxt<'tcx>,
19+
impl_m: ty::AssocItem,
20+
trait_m: ty::AssocItem,
21+
impl_trait_ref: ty::TraitRef<'tcx>,
22+
) {
23+
if !tcx.impl_method_has_trait_impl_trait_tys(impl_m.def_id) {
24+
return;
25+
}
26+
// crate-private traits don't have any library guarantees, there's no need to do this check.
27+
if !tcx.visibility(trait_m.container_id(tcx)).is_public() {
28+
return;
29+
}
30+
31+
// If a type in the trait ref is private, then there's also no reason to to do this check.
32+
let impl_def_id = impl_m.container_id(tcx);
33+
for arg in impl_trait_ref.args {
34+
if let Some(ty) = arg.as_type()
35+
&& let Some(self_visibility) = type_visibility(tcx, ty)
36+
&& !self_visibility.is_public()
37+
{
38+
return;
39+
}
40+
}
41+
42+
let impl_m_args = ty::GenericArgs::identity_for_item(tcx, impl_m.def_id);
43+
let trait_m_to_impl_m_args = impl_m_args.rebase_onto(tcx, impl_def_id, impl_trait_ref.args);
44+
let bound_trait_m_sig = tcx.fn_sig(trait_m.def_id).instantiate(tcx, trait_m_to_impl_m_args);
45+
let trait_m_sig = tcx.liberate_late_bound_regions(impl_m.def_id, bound_trait_m_sig);
46+
// replace the self type of the trait ref with `Self` so that diagnostics render better.
47+
let trait_m_sig_with_self_for_diag = tcx.liberate_late_bound_regions(
48+
impl_m.def_id,
49+
tcx.fn_sig(trait_m.def_id).instantiate(
50+
tcx,
51+
tcx.mk_args_from_iter(
52+
[tcx.types.self_param.into()]
53+
.into_iter()
54+
.chain(trait_m_to_impl_m_args.iter().skip(1)),
55+
),
56+
),
57+
);
58+
59+
let Ok(hidden_tys) = tcx.collect_return_position_impl_trait_in_trait_tys(impl_m.def_id) else {
60+
// Error already emitted, no need to delay another.
61+
return;
62+
};
63+
64+
let mut collector = ImplTraitInTraitCollector { tcx, types: FxIndexSet::default() };
65+
trait_m_sig.visit_with(&mut collector);
66+
67+
// Bound that we find on RPITITs in the trait signature.
68+
let mut trait_bounds = vec![];
69+
// Bounds that we find on the RPITITs in the impl signature.
70+
let mut impl_bounds = vec![];
71+
72+
for trait_projection in collector.types.into_iter().rev() {
73+
let impl_opaque_args = trait_projection.args.rebase_onto(tcx, trait_m.def_id, impl_m_args);
74+
let hidden_ty = hidden_tys[&trait_projection.def_id].instantiate(tcx, impl_opaque_args);
75+
76+
// If the hidden type is not an opaque, then we have "refined" the trait signature.
77+
let ty::Alias(ty::Opaque, impl_opaque) = *hidden_ty.kind() else {
78+
report_mismatched_rpitit_signature(
79+
tcx,
80+
trait_m_sig_with_self_for_diag,
81+
trait_m.def_id,
82+
impl_m.def_id,
83+
None,
84+
);
85+
return;
86+
};
87+
88+
// This opaque also needs to be from the impl method -- otherwise,
89+
// it's a refinement to a TAIT.
90+
if !tcx.hir().get_if_local(impl_opaque.def_id).map_or(false, |node| {
91+
matches!(
92+
node.expect_item().expect_opaque_ty().origin,
93+
hir::OpaqueTyOrigin::AsyncFn(def_id) | hir::OpaqueTyOrigin::FnReturn(def_id)
94+
if def_id == impl_m.def_id.expect_local()
95+
)
96+
}) {
97+
report_mismatched_rpitit_signature(
98+
tcx,
99+
trait_m_sig_with_self_for_diag,
100+
trait_m.def_id,
101+
impl_m.def_id,
102+
None,
103+
);
104+
return;
105+
}
106+
107+
trait_bounds.extend(
108+
tcx.item_bounds(trait_projection.def_id).iter_instantiated(tcx, trait_projection.args),
109+
);
110+
impl_bounds.extend(elaborate(
111+
tcx,
112+
tcx.explicit_item_bounds(impl_opaque.def_id)
113+
.iter_instantiated_copied(tcx, impl_opaque.args),
114+
));
115+
}
116+
117+
let hybrid_preds = tcx
118+
.predicates_of(impl_def_id)
119+
.instantiate_identity(tcx)
120+
.into_iter()
121+
.chain(tcx.predicates_of(trait_m.def_id).instantiate_own(tcx, trait_m_to_impl_m_args))
122+
.map(|(clause, _)| clause);
123+
let param_env = ty::ParamEnv::new(tcx.mk_clauses_from_iter(hybrid_preds), Reveal::UserFacing);
124+
let param_env = normalize_param_env_or_error(tcx, param_env, ObligationCause::dummy());
125+
126+
let ref infcx = tcx.infer_ctxt().build();
127+
let ocx = ObligationCtxt::new(infcx);
128+
129+
// Normalize the bounds. This has two purposes:
130+
//
131+
// 1. Project the RPITIT projections from the trait to the opaques on the impl,
132+
// which means that they don't need to be mapped manually.
133+
//
134+
// 2. Project any other projections that show up in the bound. That makes sure that
135+
// we don't consider `tests/ui/async-await/in-trait/async-associated-types.rs`
136+
// to be refining.
137+
let (trait_bounds, impl_bounds) =
138+
ocx.normalize(&ObligationCause::dummy(), param_env, (trait_bounds, impl_bounds));
139+
140+
// Since we've normalized things, we need to resolve regions, since we'll
141+
// possibly have introduced region vars during projection. We don't expect
142+
// this resolution to have incurred any region errors -- but if we do, then
143+
// just delay a bug.
144+
let mut implied_wf_types = FxIndexSet::default();
145+
implied_wf_types.extend(trait_m_sig.inputs_and_output);
146+
implied_wf_types.extend(ocx.normalize(
147+
&ObligationCause::dummy(),
148+
param_env,
149+
trait_m_sig.inputs_and_output,
150+
));
151+
if !ocx.select_all_or_error().is_empty() {
152+
tcx.sess.delay_span_bug(
153+
DUMMY_SP,
154+
"encountered errors when checking RPITIT refinement (selection)",
155+
);
156+
return;
157+
}
158+
let outlives_env = OutlivesEnvironment::with_bounds(
159+
param_env,
160+
infcx.implied_bounds_tys(param_env, impl_m.def_id.expect_local(), implied_wf_types),
161+
);
162+
let errors = infcx.resolve_regions(&outlives_env);
163+
if !errors.is_empty() {
164+
tcx.sess.delay_span_bug(
165+
DUMMY_SP,
166+
"encountered errors when checking RPITIT refinement (regions)",
167+
);
168+
return;
169+
}
170+
// Resolve any lifetime variables that may have been introduced during normalization.
171+
let Ok((trait_bounds, impl_bounds)) = infcx.fully_resolve((trait_bounds, impl_bounds)) else {
172+
tcx.sess.delay_span_bug(
173+
DUMMY_SP,
174+
"encountered errors when checking RPITIT refinement (resolution)",
175+
);
176+
return;
177+
};
178+
179+
// For quicker lookup, use an `IndexSet`
180+
// (we don't use one earlier because it's not foldable..)
181+
let trait_bounds = FxIndexSet::from_iter(trait_bounds);
182+
183+
// Find any clauses that are present in the impl's RPITITs that are not
184+
// present in the trait's RPITITs. This will trigger on trivial predicates,
185+
// too, since we *do not* use the trait solver to prove that the RPITIT's
186+
// bounds are not stronger -- we're doing a simple, syntactic compatibility
187+
// check between bounds. This is strictly forwards compatible, though.
188+
for (clause, span) in impl_bounds {
189+
if !trait_bounds.contains(&clause) {
190+
report_mismatched_rpitit_signature(
191+
tcx,
192+
trait_m_sig_with_self_for_diag,
193+
trait_m.def_id,
194+
impl_m.def_id,
195+
Some(span),
196+
);
197+
return;
198+
}
199+
}
200+
}
201+
202+
struct ImplTraitInTraitCollector<'tcx> {
203+
tcx: TyCtxt<'tcx>,
204+
types: FxIndexSet<ty::AliasTy<'tcx>>,
205+
}
206+
207+
impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for ImplTraitInTraitCollector<'tcx> {
208+
type BreakTy = !;
209+
210+
fn visit_ty(&mut self, ty: Ty<'tcx>) -> std::ops::ControlFlow<Self::BreakTy> {
211+
if let ty::Alias(ty::Projection, proj) = *ty.kind()
212+
&& self.tcx.is_impl_trait_in_trait(proj.def_id)
213+
{
214+
if self.types.insert(proj) {
215+
for (pred, _) in self
216+
.tcx
217+
.explicit_item_bounds(proj.def_id)
218+
.iter_instantiated_copied(self.tcx, proj.args)
219+
{
220+
pred.visit_with(self)?;
221+
}
222+
}
223+
ControlFlow::Continue(())
224+
} else {
225+
ty.super_visit_with(self)
226+
}
227+
}
228+
}
229+
230+
fn report_mismatched_rpitit_signature<'tcx>(
231+
tcx: TyCtxt<'tcx>,
232+
trait_m_sig: ty::FnSig<'tcx>,
233+
trait_m_def_id: DefId,
234+
impl_m_def_id: DefId,
235+
unmatched_bound: Option<Span>,
236+
) {
237+
let mapping = std::iter::zip(
238+
tcx.fn_sig(trait_m_def_id).skip_binder().bound_vars(),
239+
tcx.fn_sig(impl_m_def_id).skip_binder().bound_vars(),
240+
)
241+
.filter_map(|(impl_bv, trait_bv)| {
242+
if let ty::BoundVariableKind::Region(impl_bv) = impl_bv
243+
&& let ty::BoundVariableKind::Region(trait_bv) = trait_bv
244+
{
245+
Some((impl_bv, trait_bv))
246+
} else {
247+
None
248+
}
249+
})
250+
.collect();
251+
252+
let mut return_ty =
253+
trait_m_sig.output().fold_with(&mut super::RemapLateBound { tcx, mapping: &mapping });
254+
255+
if tcx.asyncness(impl_m_def_id).is_async() && tcx.asyncness(trait_m_def_id).is_async() {
256+
let ty::Alias(ty::Projection, future_ty) = return_ty.kind() else {
257+
bug!();
258+
};
259+
let Some(future_output_ty) = tcx
260+
.explicit_item_bounds(future_ty.def_id)
261+
.iter_instantiated_copied(tcx, future_ty.args)
262+
.find_map(|(clause, _)| match clause.kind().no_bound_vars()? {
263+
ty::ClauseKind::Projection(proj) => proj.term.ty(),
264+
_ => None,
265+
})
266+
else {
267+
bug!()
268+
};
269+
return_ty = future_output_ty;
270+
}
271+
272+
let (span, impl_return_span, pre, post) =
273+
match tcx.hir().get_by_def_id(impl_m_def_id.expect_local()).fn_decl().unwrap().output {
274+
hir::FnRetTy::DefaultReturn(span) => (tcx.def_span(impl_m_def_id), span, "-> ", " "),
275+
hir::FnRetTy::Return(ty) => (ty.span, ty.span, "", ""),
276+
};
277+
let trait_return_span =
278+
tcx.hir().get_if_local(trait_m_def_id).map(|node| match node.fn_decl().unwrap().output {
279+
hir::FnRetTy::DefaultReturn(_) => tcx.def_span(trait_m_def_id),
280+
hir::FnRetTy::Return(ty) => ty.span,
281+
});
282+
283+
let span = unmatched_bound.unwrap_or(span);
284+
tcx.emit_spanned_lint(
285+
REFINING_IMPL_TRAIT,
286+
tcx.local_def_id_to_hir_id(impl_m_def_id.expect_local()),
287+
span,
288+
crate::errors::ReturnPositionImplTraitInTraitRefined {
289+
impl_return_span,
290+
trait_return_span,
291+
pre,
292+
post,
293+
return_ty,
294+
unmatched_bound,
295+
},
296+
);
297+
}
298+
299+
fn type_visibility<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Option<ty::Visibility<DefId>> {
300+
match *ty.kind() {
301+
ty::Ref(_, ty, _) => type_visibility(tcx, ty),
302+
ty::Adt(def, args) => {
303+
if def.is_fundamental() {
304+
type_visibility(tcx, args.type_at(0))
305+
} else {
306+
Some(tcx.visibility(def.did()))
307+
}
308+
}
309+
_ => None,
310+
}
311+
}

compiler/rustc_hir_analysis/src/errors.rs

+16
Original file line numberDiff line numberDiff line change
@@ -919,6 +919,22 @@ pub struct UnusedAssociatedTypeBounds {
919919
pub span: Span,
920920
}
921921

922+
#[derive(LintDiagnostic)]
923+
#[diag(hir_analysis_rpitit_refined)]
924+
#[note]
925+
pub(crate) struct ReturnPositionImplTraitInTraitRefined<'tcx> {
926+
#[suggestion(applicability = "maybe-incorrect", code = "{pre}{return_ty}{post}")]
927+
pub impl_return_span: Span,
928+
#[label]
929+
pub trait_return_span: Option<Span>,
930+
#[label(hir_analysis_unmatched_bound_label)]
931+
pub unmatched_bound: Option<Span>,
932+
933+
pub pre: &'static str,
934+
pub post: &'static str,
935+
pub return_ty: Ty<'tcx>,
936+
}
937+
922938
#[derive(Diagnostic)]
923939
#[diag(hir_analysis_assoc_bound_on_const)]
924940
#[note]

0 commit comments

Comments
 (0)