rustdoc: take a more conservative approach when eliding generic args

Author: fmease

Diff for: src/librustdoc/clean/utils.rs

@@ -14,16 +14,11 @@ use rustc_ast::tokenstream::TokenTree;
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{DefId, LocalDefId, LOCAL_CRATE};
-use rustc_infer::infer::at::ToTrace;
-use rustc_infer::infer::outlives::env::OutlivesEnvironment;
 use rustc_metadata::rendered_const;
 use rustc_middle::mir;
-use rustc_middle::traits::ObligationCause;
 use rustc_middle::ty::{self, GenericArgKind, GenericArgsRef, TyCtxt};
 use rustc_middle::ty::{TypeVisitable, TypeVisitableExt};
 use rustc_span::symbol::{kw, sym, Symbol};
-use rustc_trait_selection::infer::TyCtxtInferExt;
-use rustc_trait_selection::traits::ObligationCtxt;
 use std::fmt::Write as _;
 use std::mem;
 use std::sync::LazyLock as Lazy;
@@ -86,8 +81,6 @@ pub(crate) fn ty_args_to_args<'tcx>(
     has_self: bool,
     container: Option<DefId>,
 ) -> Vec<GenericArg> {
-    let param_env = ty::ParamEnv::empty();
-    let cause = ObligationCause::dummy();
     let params = container.map(|container| &cx.tcx.generics_of(container).params);
     let mut elision_has_failed_once_before = false;
 
@@ -107,14 +100,7 @@ pub(crate) fn ty_args_to_args<'tcx>(
                 let default =
                     ty_args.map_bound(|args| default.instantiate(cx.tcx, args).expect_ty());
 
-                if can_elide_generic_arg(
-                    cx.tcx,
-                    &cause,
-                    param_env,
-                    ty_args.rebind(ty),
-                    default,
-                    params[index].def_id,
-                ) {
+                if can_elide_generic_arg(ty_args.rebind(ty), default) {
                     return None;
                 }
 
@@ -141,14 +127,7 @@ pub(crate) fn ty_args_to_args<'tcx>(
                 let default =
                     ty_args.map_bound(|args| default.instantiate(cx.tcx, args).expect_const());
 
-                if can_elide_generic_arg(
-                    cx.tcx,
-                    &cause,
-                    param_env,
-                    ty_args.rebind(ct),
-                    default,
-                    params[index].def_id,
-                ) {
+                if can_elide_generic_arg(ty_args.rebind(ct), default) {
                     return None;
                 }
 
@@ -165,50 +144,53 @@ pub(crate) fn ty_args_to_args<'tcx>(
 }
 
 /// Check if the generic argument `actual` coincides with the `default` and can therefore be elided.
-fn can_elide_generic_arg<'tcx, T: ToTrace<'tcx> + TypeVisitable<TyCtxt<'tcx>>>(
-    tcx: TyCtxt<'tcx>,
-    cause: &ObligationCause<'tcx>,
-    param_env: ty::ParamEnv<'tcx>,
-    actual: ty::Binder<'tcx, T>,
-    default: ty::Binder<'tcx, T>,
-    did: DefId,
-) -> bool {
-    // The operations below are only correct if we don't have any inference variables.
-    debug_assert!(!actual.has_infer());
-    debug_assert!(!default.has_infer());
+///
+/// This uses a very conservative approach for performance and correctness reasons, meaning for
+/// several classes of terms it claims that they cannot be elided even if they theoretically could.
+/// This is absolutely fine since this concerns mostly edge cases.
+fn can_elide_generic_arg<'tcx, Term>(
+    actual: ty::Binder<'tcx, Term>,
+    default: ty::Binder<'tcx, Term>,
+) -> bool
+where
+    Term: Eq + TypeVisitable<TyCtxt<'tcx>>,
+{
+    // In practice, we shouldn't have any inference variables at this point. However to be safe, we
+    // bail out if we do happen to stumble upon them. For performance reasons, we don't want to
+    // construct an `InferCtxt` here to properly handle them.
+    if actual.has_infer() || default.has_infer() {
+        return false;
+    }
 
-    // Since we don't properly keep track of bound variables, don't attempt to make
-    // any sense out of escaping bound variables (we just don't have enough context).
+    // Since we don't properly keep track of bound variables in rustdoc (yet), we don't attempt to
+    // make any sense out of escaping bound variables. We simply don't have enough context and it
+    // would be incorrect to try to do so anyway.
     if actual.has_escaping_bound_vars() || default.has_escaping_bound_vars() {
         return false;
     }
 
-    // If the arguments contain projections or (non-escaping) late-bound regions, we have to examine
-    // them more closely and can't take the fast path.
-    // Having projections means that there's potential to be further normalized thereby revealing if
-    // they are equal after all. Regarding late-bound regions, they can be liberated allowing us to
-    // consider more types to be equal by ignoring the names of binders.
-    if !actual.has_late_bound_regions()
-        && !actual.has_projections()
-        && !default.has_late_bound_regions()
-        && !default.has_projections()
+    // Theoretically we could now check if either term contains (non-escaping) late-bound regions or
+    // projections, relate the two using an `InferCtxt` and check if the resulting obligations hold
+    // since having projections means that the terms can potentially be further normalized thereby
+    // revealing if they are equal after all. Regarding late-bound regions, they would need to be
+    // liberated allowing us to consider more types to be equal by ignoring the names of binders
+    // (e.g., `for<'a> ...` and `for<'b> ...`).
+    //
+    // However, we are mostly interested in eliding generic args that were originally elided by the
+    // user and later filled in by the compiler (i.e., re-eliding) compared to eliding arbitrary
+    // generic arguments if they happen to coincide with the default ignoring the fact we can't
+    // possibly distinguish these two cases. Therefore and for performance reasons, we just bail out
+    // instead.
+    if actual.has_late_bound_regions()
+        || actual.has_projections()
+        || default.has_late_bound_regions()
+        || default.has_projections()
     {
-        // Check the memory addresses of the interned arguments for equality.
-        return actual.skip_binder() == default.skip_binder();
+        return false;
     }
 
-    let actual = tcx.liberate_late_bound_regions(did, actual);
-    let default = tcx.liberate_late_bound_regions(did, default);
-
-    let infcx = tcx.infer_ctxt().build();
-    let ocx = ObligationCtxt::new(&infcx);
-
-    let actual = ocx.normalize(cause, param_env, actual);
-    let default = ocx.normalize(cause, param_env, default);
-
-    ocx.eq(cause, param_env, actual, default).is_ok()
-        && ocx.select_all_or_error().is_empty()
-        && infcx.resolve_regions(&OutlivesEnvironment::new(param_env)).is_empty()
+    // Check the memory addresses of the interned arguments for equality.
+    actual.skip_binder() == default.skip_binder()
 }
 
 fn external_generic_args<'tcx>(

Diff for: tests/rustdoc/inline_cross/default-generic-args.rs

@@ -35,9 +35,8 @@ pub use default_generic_args::C0;
 pub use default_generic_args::C1;
 
 // @has user/type.C2.html
-// Test that we normalize constants in this case:
-// FIXME: Ideally, we would render `3` here instead of the def-path str of the normalized constant.
-// @has - '//*[@class="rust item-decl"]//code' "CtPair<default_generic_args::::C2::{constant#0}>"
+// FIXME: Add a comment here.
+// @has - '//*[@class="rust item-decl"]//code' "CtPair<default_generic_args::::C2::{constant#0}, 3>"
 pub use default_generic_args::C2;
 
 // @has user/type.R0.html
@@ -54,13 +53,13 @@ pub use default_generic_args::R1;
 pub use default_generic_args::R2;
 
 // @has user/type.H0.html
-// Check that we handle higher-ranked regions correctly:
+// FIXME: Update this comment: Check that we handle higher-ranked regions correctly:
 // FIXME: Ideally we would also print the *binders* here.
-// @has - '//*[@class="rust item-decl"]//code' "fn(_: fn(_: Re<'a>))"
+// @has - '//*[@class="rust item-decl"]//code' "fn(_: fn(_: Re<'a, &'a ()>))"
 pub use default_generic_args::H0;
 
 // @has user/type.H1.html
-// Check that we don't conflate distinct universially quantified regions (#1):
+// FIXME: Update this: Check that we don't conflate distinct universially quantified regions (#1):
 // FIXME: Ideally we would also print the *binders* here.
 // @has - '//*[@class="rust item-decl"]//code' "fn(_: fn(_: Re<'a, &'b ()>))"
 pub use default_generic_args::H1;
@@ -71,11 +70,12 @@ pub use default_generic_args::H1;
 pub use default_generic_args::H2;
 
 // @has user/type.P0.html
-// @has - '//*[@class="rust item-decl"]//code' "Proj<()>"
+// FIXME: Add comment here.
+// @has - '//*[@class="rust item-decl"]//code' "Proj<(), <() as Basis>::Assoc>"
 pub use default_generic_args::P0;
 
 // @has user/type.P1.html
-// @has - '//*[@class="rust item-decl"]//code' "Proj<()>"
+// @has - '//*[@class="rust item-decl"]//code' "Proj<(), bool>"
 pub use default_generic_args::P1;
 
 // @has user/type.P2.html