bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Auto merge of #127117 - Urgau:non_local_def-syntactic, r=BoxyUwU

Browse source 
Rework `non_local_definitions` lint to only use a syntactic heuristic

This PR reworks the `non_local_definitions` lint to only use a syntactic heuristic, i.e. not use a type-system logic for whenever an `impl` is local or not.

Instead the new logic wanted by T-lang in https://github.com/rust-lang/rust/issues/126768#issuecomment-2192634762, which is to consider every paths in `Self` and `Trait` and to no longer use the type-system inference trick.

`@rustbot` labels +L-non_local_definitions
Fixes #126768
This commit is contained in:
bors 2024-09-24 03:43:01 +00:00
commit f5cd2c5888
31 changed files with 186 additions and 956 deletions
Download patch file Download diff file Expand all files Collapse all files

38
compiler/rustc_lint/src/lints.rs
View file

@ -1375,12 +1375,7 @@ pub(crate) enum NonLocalDefinitionsDiag {
body_name: String,
cargo_update: Option<NonLocalDefinitionsCargoUpdateNote>,
const_anon: Option<Option<Span>>,
move_to: Option<(Span, Vec<Span>)>,
doctest: bool,
may_remove: Option<(Span, String)>,
has_trait: bool,
self_ty_str: String,
of_trait_str: Option<String>,
macro_to_change: Option<(String, &'static str)>,
},
MacroRules {
@ -1401,22 +1396,13 @@ impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag {
body_name,
cargo_update,
const_anon,
move_to,
doctest,
may_remove,
has_trait,
self_ty_str,
of_trait_str,
macro_to_change,
} => {
diag.primary_message(fluent::lint_non_local_definitions_impl);
diag.arg("depth", depth);
diag.arg("body_kind_descr", body_kind_descr);
diag.arg("body_name", body_name);
diag.arg("self_ty_str", self_ty_str);
if let Some(of_trait_str) = of_trait_str {
diag.arg("of_trait_str", of_trait_str);
}
if let Some((macro_to_change, macro_kind)) = macro_to_change {
diag.arg("macro_to_change", macro_to_change);
@ -1427,34 +1413,12 @@ impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag {
diag.subdiagnostic(cargo_update);
}
if has_trait {
diag.note(fluent::lint_bounds);
diag.note(fluent::lint_with_trait);
} else {
diag.note(fluent::lint_without_trait);
}
diag.note(fluent::lint_non_local);
if let Some((move_help, may_move)) = move_to {
let mut ms = MultiSpan::from_span(move_help);
for sp in may_move {
ms.push_span_label(sp, fluent::lint_non_local_definitions_may_move);
}
diag.span_help(ms, fluent::lint_non_local_definitions_impl_move_help);
}
if doctest {
diag.help(fluent::lint_doctest);
}
if let Some((span, part)) = may_remove {
diag.arg("may_remove_part", part);
diag.span_suggestion(
span,
fluent::lint_remove_help,
"",
Applicability::MaybeIncorrect,
);
}
if let Some(const_anon) = const_anon {
diag.note(fluent::lint_exception);
if let Some(const_anon) = const_anon {

308
compiler/rustc_lint/src/non_local_def.rs
View file

@ -1,20 +1,12 @@
use rustc_errors::MultiSpan;
use rustc_hir::def::DefKind;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::intravisit::{self, Visitor};
use rustc_hir::{Body, HirId, Item, ItemKind, Node, Path, QPath, TyKind};
use rustc_infer::infer::InferCtxt;
use rustc_infer::traits::{Obligation, ObligationCause};
use rustc_middle::ty::{
self, Binder, EarlyBinder, TraitRef, Ty, TyCtxt, TypeFoldable, TypeFolder, TypeSuperFoldable,
};
use rustc_hir::{Body, HirId, Item, ItemKind, Node, Path, TyKind};
use rustc_middle::ty::TyCtxt;
use rustc_session::{declare_lint, impl_lint_pass};
use rustc_span::def_id::{DefId, LOCAL_CRATE};
use rustc_span::symbol::kw;
use rustc_span::{ExpnKind, MacroKind, Span, Symbol, sym};
use rustc_trait_selection::error_reporting::traits::ambiguity::{
CandidateSource, compute_applicable_impls_for_diagnostics,
};
use rustc_trait_selection::infer::TyCtxtInferExt;
use rustc_span::{ExpnKind, MacroKind, Span, sym};
use crate::lints::{NonLocalDefinitionsCargoUpdateNote, NonLocalDefinitionsDiag};
use crate::{LateContext, LateLintPass, LintContext, fluent_generated as fluent};
@ -49,7 +41,7 @@ declare_lint! {
/// All nested bodies (functions, enum discriminant, array length, consts) (expect for
/// `const _: Ty = { ... }` in top-level module, which is still undecided) are checked.
pub NON_LOCAL_DEFINITIONS,
Allow,
Warn,
"checks for non-local definitions",
report_in_external_macro
}
@ -142,42 +134,28 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
None
};
// Part 1: Is the Self type local?
let self_ty_has_local_parent =
ty_has_local_parent(&impl_.self_ty.kind, cx, parent, parent_parent);
if self_ty_has_local_parent {
return;
// 1. We collect all the `hir::Path` from the `Self` type and `Trait` ref
// of the `impl` definition
let mut collector = PathCollector { paths: Vec::new() };
collector.visit_ty(&impl_.self_ty);
if let Some(of_trait) = &impl_.of_trait {
collector.visit_trait_ref(of_trait);
}
// Part 2: Is the Trait local?
let of_trait_has_local_parent = impl_
.of_trait
.map(|of_trait| path_has_local_parent(of_trait.path, cx, parent, parent_parent))
.unwrap_or(false);
// 1.5. Remove any path that doesn't resolve to a `DefId` or if it resolve to a
// type-param (e.g. `T`).
collector.paths.retain(
|p| matches!(p.res, Res::Def(def_kind, _) if def_kind != DefKind::TyParam),
);
if of_trait_has_local_parent {
return;
}
// Part 3: Is the impl definition leaking outside it's defining scope?
//
// We always consider inherent impls to be leaking.
let impl_has_enough_non_local_candidates = cx
.tcx
.impl_trait_ref(def_id)
.map(|binder| {
impl_trait_ref_has_enough_non_local_candidates(
cx.tcx,
item.span,
def_id,
binder,
|did| did_has_local_parent(did, cx.tcx, parent, parent_parent),
)
})
.unwrap_or(false);
if impl_has_enough_non_local_candidates {
// 2. We check if any of path reference a "local" parent and if that the case
// we bail out as asked by T-lang, even though this isn't correct from a
// type-system point of view, as inference exists and could still leak the impl.
if collector
.paths
.iter()
.any(|path| path_has_local_parent(path, cx, parent, parent_parent))
{
return;
}
@ -199,76 +177,28 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
let const_anon = matches!(parent_def_kind, DefKind::Const | DefKind::Static { .. })
.then_some(span_for_const_anon_suggestion);
let may_remove = match &impl_.self_ty.kind {
TyKind::Ptr(mut_ty) | TyKind::Ref(_, mut_ty)
if ty_has_local_parent(&mut_ty.ty.kind, cx, parent, parent_parent) =>
{
let type_ =
if matches!(impl_.self_ty.kind, TyKind::Ptr(_)) { "*" } else { "&" };
let part = format!("{}{}", type_, mut_ty.mutbl.prefix_str());
Some((impl_.self_ty.span.shrink_to_lo().until(mut_ty.ty.span), part))
}
_ => None,
};
let impl_span = item.span.shrink_to_lo().to(impl_.self_ty.span);
let mut ms = MultiSpan::from_span(impl_span);
let (self_ty_span, self_ty_str) =
self_ty_kind_for_diagnostic(&impl_.self_ty, cx.tcx);
ms.push_span_label(
self_ty_span,
fluent::lint_non_local_definitions_self_ty_not_local,
);
let of_trait_str = if let Some(of_trait) = &impl_.of_trait {
for path in &collector.paths {
// FIXME: While a translatable diagnostic message can have an argument
// we (currently) have no way to set different args per diag msg with
// `MultiSpan::push_span_label`.
#[allow(rustc::untranslatable_diagnostic)]
ms.push_span_label(
path_span_without_args(&of_trait.path),
fluent::lint_non_local_definitions_of_trait_not_local,
path_span_without_args(path),
format!("`{}` is not local", path_name_to_string(path)),
);
Some(path_name_to_string(&of_trait.path))
} else {
None
};
}
let (doctest, move_to) = if is_at_toplevel_doctest() {
(true, None)
} else {
let mut collector = PathCollector { paths: Vec::new() };
collector.visit_ty(&impl_.self_ty);
if let Some(of_trait) = &impl_.of_trait {
collector.visit_trait_ref(of_trait);
}
collector.visit_generics(&impl_.generics);
let doctest = is_at_toplevel_doctest();
let mut may_move: Vec<Span> = collector
.paths
.into_iter()
.filter_map(|path| {
if let Some(did) = path.res.opt_def_id()
&& did_has_local_parent(did, cx.tcx, parent, parent_parent)
{
Some(cx.tcx.def_span(did))
} else {
None
}
})
.collect();
may_move.sort();
may_move.dedup();
let move_to = if may_move.is_empty() {
ms.push_span_label(
cx.tcx.def_span(parent),
fluent::lint_non_local_definitions_impl_move_help,
);
None
} else {
Some((cx.tcx.def_span(parent), may_move))
};
(false, move_to)
};
if !doctest {
ms.push_span_label(
cx.tcx.def_span(parent),
fluent::lint_non_local_definitions_impl_move_help,
);
}
let macro_to_change =
if let ExpnKind::Macro(kind, name) = item.span.ctxt().outer_expn_data().kind {
@ -285,12 +215,7 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
.unwrap_or_else(|| "<unnameable>".to_string()),
cargo_update: cargo_update(),
const_anon,
self_ty_str,
of_trait_str,
move_to,
doctest,
may_remove,
has_trait: impl_.of_trait.is_some(),
macro_to_change,
})
}
@ -316,90 +241,6 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
}
}
// Detecting if the impl definition is leaking outside of its defining scope.
//
// Rule: for each impl, instantiate all local types with inference vars and
// then assemble candidates for that goal, if there are more than 1 (non-private
// impls), it does not leak.
//
// https://github.com/rust-lang/rust/issues/121621#issuecomment-1976826895
fn impl_trait_ref_has_enough_non_local_candidates<'tcx>(
tcx: TyCtxt<'tcx>,
infer_span: Span,
trait_def_id: DefId,
binder: EarlyBinder<'tcx, TraitRef<'tcx>>,
mut did_has_local_parent: impl FnMut(DefId) -> bool,
) -> bool {
let infcx = tcx
.infer_ctxt()
// We use the new trait solver since the obligation we are trying to
// prove here may overflow and those are fatal in the old trait solver.
// Which is unacceptable for a lint.
//
// Thanksfully the part we use here are very similar to the
// new-trait-solver-as-coherence, which is in stabilization.
//
// https://github.com/rust-lang/rust/issues/123573
.with_next_trait_solver(true)
.build();
let trait_ref = binder.instantiate(tcx, infcx.fresh_args_for_item(infer_span, trait_def_id));
let trait_ref = trait_ref.fold_with(&mut ReplaceLocalTypesWithInfer {
infcx: &infcx,
infer_span,
did_has_local_parent: &mut did_has_local_parent,
});
let poly_trait_obligation = Obligation::new(
tcx,
ObligationCause::dummy(),
ty::ParamEnv::empty(),
Binder::dummy(trait_ref),
);
let ambiguities = compute_applicable_impls_for_diagnostics(&infcx, &poly_trait_obligation);
let mut it = ambiguities.iter().filter(|ambi| match ambi {
CandidateSource::DefId(did) => !did_has_local_parent(*did),
CandidateSource::ParamEnv(_) => unreachable!(),
});
let _ = it.next();
it.next().is_some()
}
/// Replace every local type by inference variable.
///
/// ```text
/// <Global<Local> as std::cmp::PartialEq<Global<Local>>>
/// to
/// <Global<_> as std::cmp::PartialEq<Global<_>>>
/// ```
struct ReplaceLocalTypesWithInfer<'a, 'tcx, F: FnMut(DefId) -> bool> {
infcx: &'a InferCtxt<'tcx>,
did_has_local_parent: F,
infer_span: Span,
}
impl<'a, 'tcx, F: FnMut(DefId) -> bool> TypeFolder<TyCtxt<'tcx>>
for ReplaceLocalTypesWithInfer<'a, 'tcx, F>
{
fn cx(&self) -> TyCtxt<'tcx> {
self.infcx.tcx
}
fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> {
if let Some(def) = t.ty_adt_def()
&& (self.did_has_local_parent)(def.did())
{
self.infcx.next_ty_var(self.infer_span)
} else {
t.super_fold_with(self)
}
}
}
/// Simple hir::Path collector
struct PathCollector<'tcx> {
paths: Vec<Path<'tcx>>,
@ -412,42 +253,6 @@ impl<'tcx> Visitor<'tcx> for PathCollector<'tcx> {
}
}
/// Given a `Ty` we check if the (outermost) type is local.
fn ty_has_local_parent(
ty_kind: &TyKind<'_>,
cx: &LateContext<'_>,
impl_parent: DefId,
impl_parent_parent: Option<DefId>,
) -> bool {
match ty_kind {
TyKind::Path(QPath::Resolved(_, ty_path)) => {
path_has_local_parent(ty_path, cx, impl_parent, impl_parent_parent)
}
TyKind::TraitObject([principle_poly_trait_ref, ..], _, _) => path_has_local_parent(
principle_poly_trait_ref.0.trait_ref.path,
cx,
impl_parent,
impl_parent_parent,
),
TyKind::TraitObject([], _, _)
| TyKind::InferDelegation(_, _)
| TyKind::Slice(_)
| TyKind::Array(_, _)
| TyKind::Ptr(_)
| TyKind::Ref(_, _)
| TyKind::BareFn(_)
| TyKind::Never
| TyKind::Tup(_)
| TyKind::Path(_)
| TyKind::Pat(..)
| TyKind::AnonAdt(_)
| TyKind::OpaqueDef(_, _, _)
| TyKind::Typeof(_)
| TyKind::Infer
| TyKind::Err(_) => false,
}
}
/// Given a path and a parent impl def id, this checks if the if parent resolution
/// def id correspond to the def id of the parent impl definition.
///
@ -503,38 +308,3 @@ fn path_span_without_args(path: &Path<'_>) -> Span {
fn path_name_to_string(path: &Path<'_>) -> String {
path.segments.last().unwrap().ident.name.to_ident_string()
}
/// Compute the `Span` and visual representation for the `Self` we want to point at;
/// It follows part of the actual logic of non-local, and if possible return the least
/// amount possible for the span and representation.
fn self_ty_kind_for_diagnostic(ty: &rustc_hir::Ty<'_>, tcx: TyCtxt<'_>) -> (Span, String) {
match ty.kind {
TyKind::Path(QPath::Resolved(_, ty_path)) => (
path_span_without_args(ty_path),
ty_path
.res
.opt_def_id()
.map(|did| tcx.opt_item_name(did))
.flatten()
.as_ref()
.map(|s| Symbol::as_str(s))
.unwrap_or("<unnameable>")
.to_string(),
),
TyKind::TraitObject([principle_poly_trait_ref, ..], _, _) => {
let path = &principle_poly_trait_ref.0.trait_ref.path;
(
path_span_without_args(path),
path.res
.opt_def_id()
.map(|did| tcx.opt_item_name(did))
.flatten()
.as_ref()
.map(|s| Symbol::as_str(s))
.unwrap_or("<unnameable>")
.to_string(),
)
}
_ => (ty.span, rustc_hir_pretty::ty_to_string(&tcx, ty)),
}
}