Rollup merge of rust-lang#132816 - compiler-errors:2024-apit, r=jieyouxu

Dont suggest `use<impl Trait>` when we have an edition-2024-related borrowck issue

rust-lang#131186 implements some machinery to detect in borrowck when we may have RPIT overcaptures due to edition 2024, and suggests adding `+ use<'a, T>` to go back to the edition 2021 capture rules. However, we weren't filtering out cases when there are APITs in scope.

This PR implements a more sophisticated diagnostic where we will suggest turning any APITs in scope into type parameters, and applies this to both the borrowck error note, and to the `impl_trait_overcaptures` migration lint.

cc rust-lang#132809

Author: matthiaskrgr

compiler/rustc_borrowck/src/diagnostics/opaque_suggestions.rs

@@ -4,15 +4,16 @@
 use std::ops::ControlFlow;
 
 use either::Either;
+use itertools::Itertools as _;
 use rustc_data_structures::fx::FxIndexSet;
-use rustc_errors::{Applicability, Diag};
+use rustc_errors::{Diag, Subdiagnostic};
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
 use rustc_middle::mir::{self, ConstraintCategory, Location};
 use rustc_middle::ty::{
     self, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitableExt, TypeVisitor,
 };
-use rustc_span::Symbol;
+use rustc_trait_selection::errors::impl_trait_overcapture_suggestion;
 
 use crate::MirBorrowckCtxt;
 use crate::borrow_set::BorrowData;
@@ -61,6 +62,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
             // *does* mention. We'll use that for the `+ use<'a>` suggestion below.
             let mut visitor = CheckExplicitRegionMentionAndCollectGenerics {
                 tcx,
+                generics: tcx.generics_of(opaque_def_id),
                 offending_region_idx,
                 seen_opaques: [opaque_def_id].into_iter().collect(),
                 seen_lifetimes: Default::default(),
@@ -83,34 +85,50 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
                         "this call may capture more lifetimes than intended, \
                         because Rust 2024 has adjusted the `impl Trait` lifetime capture rules",
                     );
-                    let mut seen_generics: Vec<_> =
-                        visitor.seen_lifetimes.iter().map(ToString::to_string).collect();
-                    // Capture all in-scope ty/const params.
-                    seen_generics.extend(
-                        ty::GenericArgs::identity_for_item(tcx, opaque_def_id)
-                            .iter()
-                            .filter(|arg| {
-                                matches!(
-                                    arg.unpack(),
-                                    ty::GenericArgKind::Type(_) | ty::GenericArgKind::Const(_)
-                                )
-                            })
-                            .map(|arg| arg.to_string()),
-                    );
-                    if opaque_def_id.is_local() {
-                        diag.span_suggestion_verbose(
-                            tcx.def_span(opaque_def_id).shrink_to_hi(),
-                            "add a precise capturing bound to avoid overcapturing",
-                            format!(" + use<{}>", seen_generics.join(", ")),
-                            Applicability::MaybeIncorrect,
-                        );
+                    let mut captured_args = visitor.seen_lifetimes;
+                    // Add in all of the type and const params, too.
+                    // Ordering here is kinda strange b/c we're walking backwards,
+                    // but we're trying to provide *a* suggestion, not a nice one.
+                    let mut next_generics = Some(visitor.generics);
+                    let mut any_synthetic = false;
+                    while let Some(generics) = next_generics {
+                        for param in &generics.own_params {
+                            if param.kind.is_ty_or_const() {
+                                captured_args.insert(param.def_id);
+                            }
+                            if param.kind.is_synthetic() {
+                                any_synthetic = true;
+                            }
+                        }
+                        next_generics = generics.parent.map(|def_id| tcx.generics_of(def_id));
+                    }
+
+                    if let Some(opaque_def_id) = opaque_def_id.as_local()
+                        && let hir::OpaqueTyOrigin::FnReturn { parent, .. } =
+                            tcx.hir().expect_opaque_ty(opaque_def_id).origin
+                    {
+                        if let Some(sugg) = impl_trait_overcapture_suggestion(
+                            tcx,
+                            opaque_def_id,
+                            parent,
+                            captured_args,
+                        ) {
+                            sugg.add_to_diag(diag);
+                        }
                     } else {
                         diag.span_help(
                             tcx.def_span(opaque_def_id),
                             format!(
                                 "if you can modify this crate, add a precise \
                                 capturing bound to avoid overcapturing: `+ use<{}>`",
-                                seen_generics.join(", ")
+                                if any_synthetic {
+                                    "/* Args */".to_string()
+                                } else {
+                                    captured_args
+                                        .into_iter()
+                                        .map(|def_id| tcx.item_name(def_id))
+                                        .join(", ")
+                                }
                             ),
                         );
                     }
@@ -182,9 +200,10 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for FindOpaqueRegion<'_, 'tcx> {
 
 struct CheckExplicitRegionMentionAndCollectGenerics<'tcx> {
     tcx: TyCtxt<'tcx>,
+    generics: &'tcx ty::Generics,
     offending_region_idx: usize,
     seen_opaques: FxIndexSet<DefId>,
-    seen_lifetimes: FxIndexSet<Symbol>,
+    seen_lifetimes: FxIndexSet<DefId>,
 }
 
 impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for CheckExplicitRegionMentionAndCollectGenerics<'tcx> {
@@ -214,7 +233,7 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for CheckExplicitRegionMentionAndCollectGen
                 if param.index as usize == self.offending_region_idx {
                     ControlFlow::Break(())
                 } else {
-                    self.seen_lifetimes.insert(param.name);
+                    self.seen_lifetimes.insert(self.generics.region_param(param, self.tcx).def_id);
                     ControlFlow::Continue(())
                 }
             }

compiler/rustc_lint/messages.ftl

@@ -346,7 +346,6 @@ lint_impl_trait_overcaptures = `{$self_ty}` will capture more lifetimes than pos
         *[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

compiler/rustc_lint/src/impl_trait_overcaptures.rs

@@ -3,7 +3,7 @@ use std::cell::LazyCell;
 
 use rustc_data_structures::fx::{FxHashMap, FxIndexMap, FxIndexSet};
 use rustc_data_structures::unord::UnordSet;
-use rustc_errors::{Applicability, LintDiagnostic};
+use rustc_errors::{LintDiagnostic, Subdiagnostic};
 use rustc_hir as hir;
 use rustc_hir::def::DefKind;
 use rustc_hir::def_id::{DefId, LocalDefId};
@@ -22,6 +22,9 @@ use rustc_session::lint::FutureIncompatibilityReason;
 use rustc_session::{declare_lint, declare_lint_pass};
 use rustc_span::edition::Edition;
 use rustc_span::{Span, Symbol};
+use rustc_trait_selection::errors::{
+    AddPreciseCapturingForOvercapture, impl_trait_overcapture_suggestion,
+};
 use rustc_trait_selection::traits::ObligationCtxt;
 use rustc_trait_selection::traits::outlives_bounds::InferCtxtExt;
 
@@ -334,32 +337,12 @@ where
                 // 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 !uncaptured_args.is_empty() {
-                    let suggestion = if let Ok(snippet) =
-                        self.tcx.sess.source_map().span_to_snippet(opaque_span)
-                        && snippet.starts_with("impl ")
-                    {
-                        let (lifetimes, others): (Vec<_>, Vec<_>) =
-                            captured.into_iter().partition(|def_id| {
-                                self.tcx.def_kind(*def_id) == DefKind::LifetimeParam
-                            });
-                        // Take all lifetime params first, then all others (ty/ct).
-                        let generics: Vec<_> = lifetimes
-                            .into_iter()
-                            .chain(others)
-                            .map(|def_id| self.tcx.item_name(def_id).to_string())
-                            .collect();
-                        // Make sure that we're not trying to name any APITs
-                        if generics.iter().all(|name| !name.starts_with("impl ")) {
-                            Some((
-                                format!(" + use<{}>", generics.join(", ")),
-                                opaque_span.shrink_to_hi(),
-                            ))
-                        } else {
-                            None
-                        }
-                    } else {
-                        None
-                    };
+                    let suggestion = impl_trait_overcapture_suggestion(
+                        self.tcx,
+                        opaque_def_id,
+                        self.parent_def_id,
+                        captured,
+                    );
 
                     let uncaptured_spans: Vec<_> = uncaptured_args
                         .into_iter()
@@ -451,7 +434,7 @@ struct ImplTraitOvercapturesLint<'tcx> {
     uncaptured_spans: Vec<Span>,
     self_ty: Ty<'tcx>,
     num_captured: usize,
-    suggestion: Option<(String, Span)>,
+    suggestion: Option<AddPreciseCapturingForOvercapture>,
 }
 
 impl<'a> LintDiagnostic<'a, ()> for ImplTraitOvercapturesLint<'_> {
@@ -461,13 +444,8 @@ impl<'a> LintDiagnostic<'a, ()> for ImplTraitOvercapturesLint<'_> {
             .arg("num_captured", self.num_captured)
             .span_note(self.uncaptured_spans, fluent::lint_note)
             .note(fluent::lint_note2);
-        if let Some((suggestion, span)) = self.suggestion {
-            diag.span_suggestion(
-                span,
-                fluent::lint_suggestion,
-                suggestion,
-                Applicability::MachineApplicable,
-            );
+        if let Some(suggestion) = self.suggestion {
+            suggestion.add_to_diag(diag);
         }
     }
 }

compiler/rustc_trait_selection/messages.ftl

@@ -282,6 +282,8 @@ trait_selection_precise_capturing_new = add a `use<...>` bound to explicitly cap
 
 trait_selection_precise_capturing_new_but_apit = add a `use<...>` bound to explicitly capture `{$new_lifetime}` after turning all argument-position `impl Trait` into type parameters, noting that this possibly affects the API of this crate
 
+trait_selection_precise_capturing_overcaptures = use the precise capturing `use<...>` syntax to make the captures explicit
+
 trait_selection_prlf_defined_with_sub = the lifetime `{$sub_symbol}` defined here...
 trait_selection_prlf_defined_without_sub = the lifetime defined here...
 trait_selection_prlf_known_limitation = this is a known limitation that will be removed in the future (see issue #100013 <https://github.com/rust-lang/rust/issues/100013> for more information)
@@ -455,7 +457,9 @@ trait_selection_unable_to_construct_constant_value = unable to construct a const
 trait_selection_unknown_format_parameter_for_on_unimplemented_attr = there is no parameter `{$argument_name}` on trait `{$trait_name}`
     .help = expect either a generic argument name or {"`{Self}`"} as format argument
 
-trait_selection_warn_removing_apit_params = you could use a `use<...>` bound to explicitly capture `{$new_lifetime}`, but argument-position `impl Trait`s are not nameable
+trait_selection_warn_removing_apit_params_for_overcapture = you could use a `use<...>` bound to explicitly specify captures, but argument-position `impl Trait`s are not nameable
+
+trait_selection_warn_removing_apit_params_for_undercapture = you could use a `use<...>` bound to explicitly capture `{$new_lifetime}`, but argument-position `impl Trait`s are not nameable
 
 trait_selection_where_copy_predicates = copy the `where` clause predicates from the trait
 

compiler/rustc_trait_selection/src/errors.rs

@@ -1,13 +1,14 @@
 use std::path::PathBuf;
 
-use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::fx::{FxHashSet, FxIndexSet};
 use rustc_errors::codes::*;
 use rustc_errors::{
     Applicability, Diag, DiagCtxtHandle, DiagMessage, DiagStyledString, Diagnostic,
     EmissionGuarantee, IntoDiagArg, Level, MultiSpan, SubdiagMessageOp, Subdiagnostic,
 };
 use rustc_hir as hir;
-use rustc_hir::def_id::LocalDefId;
+use rustc_hir::def::DefKind;
+use rustc_hir::def_id::{DefId, LocalDefId};
 use rustc_hir::intravisit::{Visitor, walk_ty};
 use rustc_hir::{FnRetTy, GenericParamKind};
 use rustc_macros::{Diagnostic, Subdiagnostic};
@@ -1792,6 +1793,130 @@ impl Subdiagnostic for AddPreciseCapturingAndParams {
             self.suggs,
             Applicability::MaybeIncorrect,
         );
-        diag.span_note(self.apit_spans, fluent::trait_selection_warn_removing_apit_params);
+        diag.span_note(
+            self.apit_spans,
+            fluent::trait_selection_warn_removing_apit_params_for_undercapture,
+        );
+    }
+}
+
+/// Given a set of captured `DefId` for an RPIT (opaque_def_id) and a given
+/// function (fn_def_id), try to suggest adding `+ use<...>` to capture just
+/// the specified parameters. If one of those parameters is an APIT, then try
+/// to suggest turning it into a regular type parameter.
+pub fn impl_trait_overcapture_suggestion<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    opaque_def_id: LocalDefId,
+    fn_def_id: LocalDefId,
+    captured_args: FxIndexSet<DefId>,
+) -> Option<AddPreciseCapturingForOvercapture> {
+    let generics = tcx.generics_of(fn_def_id);
+
+    let mut captured_lifetimes = FxIndexSet::default();
+    let mut captured_non_lifetimes = FxIndexSet::default();
+    let mut synthetics = vec![];
+
+    for arg in captured_args {
+        if tcx.def_kind(arg) == DefKind::LifetimeParam {
+            captured_lifetimes.insert(tcx.item_name(arg));
+        } else {
+            let idx = generics.param_def_id_to_index(tcx, arg).expect("expected arg in scope");
+            let param = generics.param_at(idx as usize, tcx);
+            if param.kind.is_synthetic() {
+                synthetics.push((tcx.def_span(arg), param.name));
+            } else {
+                captured_non_lifetimes.insert(tcx.item_name(arg));
+            }
+        }
+    }
+
+    let mut next_fresh_param = || {
+        ["T", "U", "V", "W", "X", "Y", "A", "B", "C"]
+            .into_iter()
+            .map(Symbol::intern)
+            .chain((0..).map(|i| Symbol::intern(&format!("T{i}"))))
+            .find(|s| captured_non_lifetimes.insert(*s))
+            .unwrap()
+    };
+
+    let mut suggs = vec![];
+    let mut apit_spans = vec![];
+
+    if !synthetics.is_empty() {
+        let mut new_params = String::new();
+        for (i, (span, name)) in synthetics.into_iter().enumerate() {
+            apit_spans.push(span);
+
+            let fresh_param = next_fresh_param();
+
+            // Suggest renaming.
+            suggs.push((span, fresh_param.to_string()));
+
+            // Super jank. Turn `impl Trait` into `T: Trait`.
+            //
+            // This currently involves stripping the `impl` from the name of
+            // the parameter, since APITs are always named after how they are
+            // rendered in the AST. This sucks! But to recreate the bound list
+            // from the APIT itself would be miserable, so we're stuck with
+            // this for now!
+            if i > 0 {
+                new_params += ", ";
+            }
+            let name_as_bounds = name.as_str().trim_start_matches("impl").trim_start();
+            new_params += fresh_param.as_str();
+            new_params += ": ";
+            new_params += name_as_bounds;
+        }
+
+        let Some(generics) = tcx.hir().get_generics(fn_def_id) else {
+            // This shouldn't happen, but don't ICE.
+            return None;
+        };
+
+        // Add generics or concatenate to the end of the list.
+        suggs.push(if let Some(params_span) = generics.span_for_param_suggestion() {
+            (params_span, format!(", {new_params}"))
+        } else {
+            (generics.span, format!("<{new_params}>"))
+        });
+    }
+
+    let concatenated_bounds = captured_lifetimes
+        .into_iter()
+        .chain(captured_non_lifetimes)
+        .map(|sym| sym.to_string())
+        .collect::<Vec<_>>()
+        .join(", ");
+
+    suggs.push((
+        tcx.def_span(opaque_def_id).shrink_to_hi(),
+        format!(" + use<{concatenated_bounds}>"),
+    ));
+
+    Some(AddPreciseCapturingForOvercapture { suggs, apit_spans })
+}
+
+pub struct AddPreciseCapturingForOvercapture {
+    pub suggs: Vec<(Span, String)>,
+    pub apit_spans: Vec<Span>,
+}
+
+impl Subdiagnostic for AddPreciseCapturingForOvercapture {
+    fn add_to_diag_with<G: EmissionGuarantee, F: SubdiagMessageOp<G>>(
+        self,
+        diag: &mut Diag<'_, G>,
+        _f: &F,
+    ) {
+        diag.multipart_suggestion_verbose(
+            fluent::trait_selection_precise_capturing_overcaptures,
+            self.suggs,
+            Applicability::MaybeIncorrect,
+        );
+        if !self.apit_spans.is_empty() {
+            diag.span_note(
+                self.apit_spans,
+                fluent::trait_selection_warn_removing_apit_params_for_overcapture,
+            );
+        }
     }
 }