Skip to content

Commit

Permalink
Warn against redundant use<...>
Browse files Browse the repository at this point in the history
  • Loading branch information
compiler-errors committed Apr 20, 2024
1 parent efd136e commit 86f7e9c
Show file tree
Hide file tree
Showing 10 changed files with 185 additions and 64 deletions.
15 changes: 9 additions & 6 deletions compiler/rustc_ast_lowering/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1407,7 +1407,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
bounds,
fn_kind,
itctx,
precise_capturing.as_deref().map(|(args, _)| args.as_slice()),
precise_capturing.as_deref().map(|(args, span)| (args.as_slice(), *span)),
),
ImplTraitContext::Universal => {
if let Some(&(_, span)) = precise_capturing.as_deref() {
Expand Down Expand Up @@ -1523,7 +1523,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
bounds: &GenericBounds,
fn_kind: Option<FnDeclKind>,
itctx: ImplTraitContext,
precise_capturing_args: Option<&[PreciseCapturingArg]>,
precise_capturing_args: Option<(&[PreciseCapturingArg], Span)>,
) -> hir::TyKind<'hir> {
// Make sure we know that some funky desugaring has been going on here.
// This is a first: there is code in other places like for loop
Expand All @@ -1533,7 +1533,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
let opaque_ty_span = self.mark_span_with_reason(DesugaringKind::OpaqueTy, span, None);

let captured_lifetimes_to_duplicate =
if let Some(precise_capturing) = precise_capturing_args {
if let Some((precise_capturing, _)) = precise_capturing_args {
// We'll actually validate these later on; all we need is the list of
// lifetimes to duplicate during this portion of lowering.
precise_capturing
Expand Down Expand Up @@ -1607,7 +1607,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
captured_lifetimes_to_duplicate: FxIndexSet<Lifetime>,
span: Span,
opaque_ty_span: Span,
precise_capturing_args: Option<&[PreciseCapturingArg]>,
precise_capturing_args: Option<(&[PreciseCapturingArg], Span)>,
lower_item_bounds: impl FnOnce(&mut Self) -> &'hir [hir::GenericBound<'hir>],
) -> hir::TyKind<'hir> {
let opaque_ty_def_id = self.create_def(
Expand Down Expand Up @@ -1698,8 +1698,11 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
this.with_remapping(captured_to_synthesized_mapping, |this| {
(
lower_item_bounds(this),
precise_capturing_args.map(|precise_capturing| {
this.lower_precise_capturing_args(precise_capturing)
precise_capturing_args.map(|(precise_capturing, span)| {
(
this.lower_precise_capturing_args(precise_capturing),
this.lower_span(span),
)
}),
)
});
Expand Down
11 changes: 10 additions & 1 deletion compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2558,7 +2558,7 @@ pub struct OpaqueTy<'hir> {
/// lowered as an associated type.
pub in_trait: bool,
/// List of arguments captured via `impl use<'a, P, ...> Trait` syntax.
pub precise_capturing_args: Option<&'hir [PreciseCapturingArg<'hir>]>,
pub precise_capturing_args: Option<(&'hir [PreciseCapturingArg<'hir>], Span)>,
}

#[derive(Debug, Clone, Copy, HashStable_Generic)]
Expand All @@ -2568,6 +2568,15 @@ pub enum PreciseCapturingArg<'hir> {
Param(PreciseCapturingNonLifetimeArg),
}

impl PreciseCapturingArg<'_> {
pub fn hir_id(self) -> HirId {
match self {
PreciseCapturingArg::Lifetime(lt) => lt.hir_id,
PreciseCapturingArg::Param(param) => param.hir_id,
}
}
}

/// We need to have a [`Node`] for the [`HirId`] that we attach the type/const param
/// resolution to. Lifetimes don't have this problem, and for them, it's actually
/// kind of detrimental to use a custom node type versus just using [`Lifetime`],
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir/src/intravisit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ pub fn walk_item<'v, V: Visitor<'v>>(visitor: &mut V, item: &'v Item<'v>) -> V::
try_visit!(visitor.visit_id(item.hir_id()));
try_visit!(walk_generics(visitor, generics));
walk_list!(visitor, visit_param_bound, bounds);
if let Some(precise_capturing_args) = precise_capturing_args {
if let Some((precise_capturing_args, _)) = precise_capturing_args {
for arg in precise_capturing_args {
try_visit!(visitor.visit_precise_capturing_arg(arg));
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/check/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ fn sanity_check_found_hidden_type<'tcx>(
fn check_opaque_precise_captures<'tcx>(tcx: TyCtxt<'tcx>, opaque_def_id: LocalDefId) {
let hir::OpaqueTy { precise_capturing_args, .. } =
*tcx.hir_node_by_def_id(opaque_def_id).expect_item().expect_opaque_ty();
let Some(precise_capturing_args) = precise_capturing_args else {
let Some((precise_capturing_args, _)) = precise_capturing_args else {
// No precise capturing args; nothing to validate
return;
};
Expand Down
19 changes: 11 additions & 8 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,6 @@ lint_array_into_iter =
.use_explicit_into_iter_suggestion =
or use `IntoIterator::into_iter(..)` instead of `.into_iter()` to explicitly iterate by value
lint_impl_trait_overcaptures = `{$self_ty}` will capture more lifetimes than possibly intended in edition 2024
.note = specifically, {$num_captured ->
[one] this lifetime is
*[other] these lifetimes are
} in scope but not mentioned in the type's bounds
.note2 = all lifetimes in scope will be captured by `impl Trait`s in edition 2024
.suggestion = use the precise capturing `use<...>` syntax to make the captures explicit
lint_async_fn_in_trait = use of `async fn` in public traits is discouraged as auto trait bounds cannot be specified
.note = you can suppress this lint if you plan to use the trait only in your own code, or do not care about auto traits like `Send` on the `Future`
.suggestion = you can alternatively desugar to a normal `fn` that returns `impl Future` and add any desired bounds such as `Send`, but these cannot be relaxed without a breaking API change
Expand Down Expand Up @@ -277,6 +269,17 @@ lint_identifier_uncommon_codepoints = identifier contains {$codepoints_len ->
lint_ignored_unless_crate_specified = {$level}({$name}) is ignored unless specified at crate level
lint_impl_trait_overcaptures = `{$self_ty}` will capture more lifetimes than possibly intended in edition 2024
.note = specifically, {$num_captured ->
[one] this lifetime is
*[other] these lifetimes are
} in scope but not mentioned in the type's bounds
.note2 = all lifetimes in scope will be captured by `impl Trait`s in edition 2024
.suggestion = use the precise capturing `use<...>` syntax to make the captures explicit
lint_impl_trait_redundant_captures = all possible in-scope parameters are already captured, so `use<...>` syntax is redundant
.suggestion = remove the `use<...>` syntax
lint_improper_ctypes = `extern` {$desc} uses type `{$ty}`, which is not FFI-safe
.label = not FFI-safe
.note = the type is defined here
Expand Down
109 changes: 91 additions & 18 deletions compiler/rustc_lint/src/impl_trait_overcaptures.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
use rustc_data_structures::fx::FxIndexSet;
use rustc_data_structures::unord::UnordSet;
use rustc_errors::{Applicability, LintDiagnostic};
use rustc_hir as hir;
use rustc_hir::def::DefKind;
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::intravisit;
use rustc_macros::LintDiagnostic;
use rustc_middle::middle::resolve_bound_vars::ResolvedArg;
use rustc_middle::ty::{
self, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitableExt, TypeVisitor,
};
Expand All @@ -14,21 +17,30 @@ use rustc_span::{BytePos, Span};
use crate::fluent_generated as fluent;
use crate::{LateContext, LateLintPass};

// TODO: feature gate these too

declare_lint! {
/// UwU
pub IMPL_TRAIT_OVERCAPTURES,
Warn,
Allow,
"will capture more lifetimes than possibly intended in edition 2024",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::EditionSemanticsChange(Edition::Edition2024),
reference: "<https://doc.rust-lang.org/nightly/edition-guide/rust-2021/IntoIterator-for-arrays.html>",
};
}

declare_lint! {
/// UwU
pub IMPL_TRAIT_REDUNDANT_CAPTURES,
Warn,
"uwu 2"
}

declare_lint_pass!(
/// Lint for opaque types that will begin capturing in-scope but unmentioned lifetimes
/// in edition 2024.
ImplTraitOvercaptures => [IMPL_TRAIT_OVERCAPTURES]
ImplTraitOvercaptures => [IMPL_TRAIT_OVERCAPTURES, IMPL_TRAIT_REDUNDANT_CAPTURES]
);

impl<'tcx> LateLintPass<'tcx> for ImplTraitOvercaptures {
Expand Down Expand Up @@ -109,14 +121,11 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for VisitOpaqueTypes<'tcx> {
let unique = self.in_scope_parameters.insert(def_id);
assert!(unique);
}
ty::BoundVariableKind::Ty(_) => {
todo!("we don't support late-bound type params in `impl Trait`")
}
ty::BoundVariableKind::Region(..) => {
unreachable!("all AST-derived bound regions should have a name")
}
ty::BoundVariableKind::Const => {
unreachable!("non-lifetime binder consts are not allowed")
_ => {
self.tcx.dcx().span_delayed_bug(
self.tcx.def_span(self.parent_def_id),
format!("unsupported bound variable kind: {arg:?}"),
);
}
}
}
Expand Down Expand Up @@ -144,8 +153,6 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for VisitOpaqueTypes<'tcx> {
self.tcx.hir_node_by_def_id(opaque_def_id).expect_item().expect_opaque_ty()
&& let hir::OpaqueTyOrigin::FnReturn(parent_def_id) = opaque.origin
&& parent_def_id == self.parent_def_id
// And if the opaque doesn't already have `use<>` syntax on it...
&& opaque.precise_capturing_args.is_none()
{
// Compute the set of args that are captured by the opaque...
let mut captured = FxIndexSet::default();
Expand Down Expand Up @@ -178,9 +185,16 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for VisitOpaqueTypes<'tcx> {
.map(|def_id| self.tcx.def_span(def_id))
.collect();

if !uncaptured_spans.is_empty() {
let opaque_span = self.tcx.def_span(opaque_def_id);
let opaque_span = self.tcx.def_span(opaque_def_id);
let new_capture_rules =
opaque_span.at_least_rust_2024() || self.tcx.features().lifetime_capture_rules_2024;

// If we have uncaptured args, and if the opaque doesn't already have
// `use<>` syntax on it, and we're < edition 2024, then warn the user.
if !new_capture_rules
&& opaque.precise_capturing_args.is_none()
&& !uncaptured_spans.is_empty()
{
let suggestion = if let Ok(snippet) =
self.tcx.sess.source_map().span_to_snippet(opaque_span)
&& snippet.starts_with("impl ")
Expand All @@ -207,18 +221,72 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for VisitOpaqueTypes<'tcx> {
None
};

self.tcx.emit_node_lint(
self.tcx.emit_node_span_lint(
IMPL_TRAIT_OVERCAPTURES,
self.tcx.local_def_id_to_hir_id(opaque_def_id),
opaque_span,
ImplTraitOvercapturesLint {
opaque_span,
self_ty: t,
num_captured: uncaptured_spans.len(),
uncaptured_spans,
suggestion,
},
);
}
// Otherwise, if we are edition 2024, have `use<>` syntax, and
// have no uncaptured args, then we should warn to the user that
// it's redundant to capture all args explicitly.
else if new_capture_rules
&& let Some((captured_args, capturing_span)) = opaque.precise_capturing_args
{
let mut explicitly_captured = UnordSet::default();
for arg in captured_args {
match self.tcx.named_bound_var(arg.hir_id()) {
Some(
ResolvedArg::EarlyBound(def_id) | ResolvedArg::LateBound(_, _, def_id),
) => {
if self.tcx.def_kind(self.tcx.parent(def_id)) == DefKind::OpaqueTy {
let (ty::ReEarlyParam(ty::EarlyParamRegion { def_id, .. })
| ty::ReLateParam(ty::LateParamRegion {
bound_region: ty::BoundRegionKind::BrNamed(def_id, _),
..
})) = self
.tcx
.map_opaque_lifetime_to_parent_lifetime(def_id.expect_local())
.kind()
else {
span_bug!(
self.tcx.def_span(def_id),
"variable should have been duplicated from a parent"
);
};
explicitly_captured.insert(def_id);
} else {
explicitly_captured.insert(def_id);
}
}
_ => {
self.tcx.dcx().span_delayed_bug(
self.tcx().hir().span(arg.hir_id()),
"no valid for captured arg",
);
}
}
}

if self
.in_scope_parameters
.iter()
.all(|def_id| explicitly_captured.contains(def_id))
{
self.tcx.emit_node_span_lint(
IMPL_TRAIT_REDUNDANT_CAPTURES,
self.tcx.local_def_id_to_hir_id(opaque_def_id),
opaque_span,
ImplTraitRedundantCapturesLint { capturing_span },
);
}
}

// Walk into the bounds of the opaque, too, since we want to get nested opaques
// in this lint as well. Interestingly, one place that I expect this lint to fire
Expand All @@ -236,7 +304,6 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for VisitOpaqueTypes<'tcx> {
}

struct ImplTraitOvercapturesLint<'tcx> {
opaque_span: Span,
uncaptured_spans: Vec<Span>,
self_ty: Ty<'tcx>,
num_captured: usize,
Expand All @@ -247,7 +314,6 @@ impl<'a> LintDiagnostic<'a, ()> for ImplTraitOvercapturesLint<'_> {
fn decorate_lint<'b>(self, diag: &'b mut rustc_errors::Diag<'a, ()>) {
diag.arg("self_ty", self.self_ty.to_string())
.arg("num_captured", self.num_captured)
.span(self.opaque_span)
.span_note(self.uncaptured_spans, fluent::lint_note)
.note(fluent::lint_note2);
if let Some((suggestion, span)) = self.suggestion {
Expand All @@ -265,6 +331,13 @@ impl<'a> LintDiagnostic<'a, ()> for ImplTraitOvercapturesLint<'_> {
}
}

#[derive(LintDiagnostic)]
#[diag(lint_impl_trait_redundant_captures)]
struct ImplTraitRedundantCapturesLint {
#[suggestion(lint_suggestion, code = "", applicability = "machine-applicable")]
capturing_span: Span,
}

fn extract_def_id_from_arg<'tcx>(
tcx: TyCtxt<'tcx>,
generics: &'tcx ty::Generics,
Expand Down
58 changes: 30 additions & 28 deletions compiler/rustc_parse/src/parser/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -675,8 +675,8 @@ impl<'a> Parser<'a> {
let precise_capturing = if self.eat_keyword(kw::Use) {
let use_span = self.prev_token.span;
self.psess.gated_spans.gate(sym::precise_capturing, use_span);
let args = self.parse_precise_capturing_args()?;
Some(P((args, use_span)))
let (args, args_span) = self.parse_precise_capturing_args()?;
Some(P((args, use_span.to(args_span))))
} else {
None
};
Expand All @@ -689,32 +689,34 @@ impl<'a> Parser<'a> {
Ok(TyKind::ImplTrait(ast::DUMMY_NODE_ID, bounds, precise_capturing))
}

fn parse_precise_capturing_args(&mut self) -> PResult<'a, ThinVec<PreciseCapturingArg>> {
Ok(self
.parse_unspanned_seq(
&TokenKind::Lt,
&TokenKind::Gt,
SeqSep::trailing_allowed(token::Comma),
|self_| {
if self_.check_keyword(kw::SelfUpper) {
self_.bump();
Ok(PreciseCapturingArg::Arg(
ast::Path::from_ident(self_.prev_token.ident().unwrap().0),
DUMMY_NODE_ID,
))
} else if self_.check_ident() {
Ok(PreciseCapturingArg::Arg(
ast::Path::from_ident(self_.parse_ident()?),
DUMMY_NODE_ID,
))
} else if self_.check_lifetime() {
Ok(PreciseCapturingArg::Lifetime(self_.expect_lifetime()))
} else {
self_.unexpected_any()
}
},
)?
.0)
fn parse_precise_capturing_args(
&mut self,
) -> PResult<'a, (ThinVec<PreciseCapturingArg>, Span)> {
let lo = self.token.span;
let (args, _) = self.parse_unspanned_seq(
&TokenKind::Lt,
&TokenKind::Gt,
SeqSep::trailing_allowed(token::Comma),
|self_| {
if self_.check_keyword(kw::SelfUpper) {
self_.bump();
Ok(PreciseCapturingArg::Arg(
ast::Path::from_ident(self_.prev_token.ident().unwrap().0),
DUMMY_NODE_ID,
))
} else if self_.check_ident() {
Ok(PreciseCapturingArg::Arg(
ast::Path::from_ident(self_.parse_ident()?),
DUMMY_NODE_ID,
))
} else if self_.check_lifetime() {
Ok(PreciseCapturingArg::Lifetime(self_.expect_lifetime()))
} else {
self_.unexpected_any()
}
},
)?;
Ok((args, lo.to(self.prev_token.span)))
}

/// Is a `dyn B0 + ... + Bn` type allowed here?
Expand Down
Loading

0 comments on commit 86f7e9c

Please sign in to comment.