From c8237db3eed2c91c14ce46b482b3d479a39d328d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 16 Feb 2023 12:06:36 +1100 Subject: [PATCH] Clarify iterator interners. There are two traits, `InternAs` and `InternIteratorElement`. I found them confusing to use, particularly this: ``` pub fn mk_tup, Ty<'tcx>>>(self, iter: I) -> I::Output { iter.intern_with(|ts| self.intern_tup(ts)) } ``` where I thought there might have been two levels of interning going on (there isn't) due to the `intern_with`/`InternAs` + `intern_tup` naming. And then I found the actual traits and impls themselves *very* confusing. - `InternAs` has a single impl, for iterators, with four type variables. - `InternAs` is only implemented for iterators because it wouldn't really make sense to implement for any other type. And you can't really understand the trait without seeing that single impl, which is suspicious. - `InternAs` is basically just a wrapper for `InternIteratorElement` which does all the actual work. - Neither trait actually does any interning. They just have `Intern` in their name because they are used *by* interning code. - There are no comments. So this commit improves things. - It removes `InternAs` completely. This makes the `mk_*` function signatures slightly more verbose -- two trait bounds instead of one -- but much easier to read, because you only need to understand one trait instead of two. - It renames `InternIteratorElement` as `CollectAndApply`. Likewise, it renames its method `intern_with` as `collect_and_apply`. These names describe better what's going on: we collect the iterator elements into a slice and then apply a function to the slice. - It adds comments, making clear that all this is all there just to provide an optimized version of `f(&iter.collect::>())`. It took me a couple of attempts to come up with this commit. My initial attempt kept `InternAs` around, but renamed things and added comments, and I wasn't happy with it. I think this version is much better. The resulting code is shorter, despite the addition of the comments. --- compiler/rustc_middle/src/ty/context.rs | 95 ++++++++++++++----------- compiler/rustc_type_ir/src/lib.rs | 62 ++++++++-------- 2 files changed, 87 insertions(+), 70 deletions(-) diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index 7b144d478e0..7b4434a3843 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -67,7 +67,7 @@ use rustc_target::abi::{Layout, LayoutS, TargetDataLayout, VariantIdx}; use rustc_target::spec::abi; use rustc_type_ir::sty::TyKind::*; use rustc_type_ir::WithCachedTypeInfo; -use rustc_type_ir::{DynKind, InternAs, InternIteratorElement, Interner, TypeFlags}; +use rustc_type_ir::{CollectAndApply, DynKind, Interner, TypeFlags}; use std::any::Any; use std::borrow::Borrow; @@ -1835,8 +1835,12 @@ impl<'tcx> TyCtxt<'tcx> { if ts.is_empty() { self.types.unit } else { self.mk_ty(Tuple(self.intern_type_list(&ts))) } } - pub fn mk_tup, Ty<'tcx>>>(self, iter: I) -> I::Output { - iter.intern_with(|ts| self.intern_tup(ts)) + pub fn mk_tup(self, iter: I) -> T::Output + where + I: Iterator, + T: CollectAndApply, Ty<'tcx>>, + { + T::collect_and_apply(iter, |ts| self.intern_tup(ts)) } #[inline] @@ -2157,11 +2161,12 @@ impl<'tcx> TyCtxt<'tcx> { } } - pub fn mk_const_list, &'tcx List>>>( - self, - iter: I, - ) -> I::Output { - iter.intern_with(|xs| self.intern_const_list(xs)) + pub fn mk_const_list(self, iter: I) -> T::Output + where + I: Iterator, + T: CollectAndApply, &'tcx List>>, + { + T::collect_and_apply(iter, |xs| self.intern_const_list(xs)) } pub fn intern_const_list(self, cs: &[ty::Const<'tcx>]) -> &'tcx List> { @@ -2220,9 +2225,9 @@ impl<'tcx> TyCtxt<'tcx> { ) -> T::Output where I: Iterator, - T: InternIteratorElement, ty::FnSig<'tcx>>, + T: CollectAndApply, ty::FnSig<'tcx>>, { - inputs.chain(iter::once(output)).intern_with(|xs| ty::FnSig { + T::collect_and_apply(inputs.chain(iter::once(output)), |xs| ty::FnSig { inputs_and_output: self.intern_type_list(xs), c_variadic, unsafety, @@ -2230,38 +2235,47 @@ impl<'tcx> TyCtxt<'tcx> { }) } - pub fn mk_poly_existential_predicates< - I: InternAs, &'tcx List>>, - >( - self, - iter: I, - ) -> I::Output { - iter.intern_with(|xs| self.intern_poly_existential_predicates(xs)) + pub fn mk_poly_existential_predicates(self, iter: I) -> T::Output + where + I: Iterator, + T: CollectAndApply< + PolyExistentialPredicate<'tcx>, + &'tcx List>, + >, + { + T::collect_and_apply(iter, |xs| self.intern_poly_existential_predicates(xs)) } - pub fn mk_predicates, &'tcx List>>>( - self, - iter: I, - ) -> I::Output { - iter.intern_with(|xs| self.intern_predicates(xs)) + pub fn mk_predicates(self, iter: I) -> T::Output + where + I: Iterator, + T: CollectAndApply, &'tcx List>>, + { + T::collect_and_apply(iter, |xs| self.intern_predicates(xs)) } - pub fn mk_type_list, &'tcx List>>>(self, iter: I) -> I::Output { - iter.intern_with(|xs| self.intern_type_list(xs)) + pub fn mk_type_list(self, iter: I) -> T::Output + where + I: Iterator, + T: CollectAndApply, &'tcx List>>, + { + T::collect_and_apply(iter, |xs| self.intern_type_list(xs)) } - pub fn mk_substs, &'tcx List>>>( - self, - iter: I, - ) -> I::Output { - iter.intern_with(|xs| self.intern_substs(xs)) + pub fn mk_substs(self, iter: I) -> T::Output + where + I: Iterator, + T: CollectAndApply, &'tcx List>>, + { + T::collect_and_apply(iter, |xs| self.intern_substs(xs)) } - pub fn mk_place_elems, &'tcx List>>>( - self, - iter: I, - ) -> I::Output { - iter.intern_with(|xs| self.intern_place_elems(xs)) + pub fn mk_place_elems(self, iter: I) -> T::Output + where + I: Iterator, + T: CollectAndApply, &'tcx List>>, + { + T::collect_and_apply(iter, |xs| self.intern_place_elems(xs)) } pub fn mk_substs_trait( @@ -2290,13 +2304,12 @@ impl<'tcx> TyCtxt<'tcx> { ty::AliasTy { def_id, substs, _use_mk_alias_ty_instead: () } } - pub fn mk_bound_variable_kinds< - I: InternAs>, - >( - self, - iter: I, - ) -> I::Output { - iter.intern_with(|xs| self.intern_bound_variable_kinds(xs)) + pub fn mk_bound_variable_kinds(self, iter: I) -> T::Output + where + I: Iterator, + T: CollectAndApply>, + { + T::collect_and_apply(iter, |xs| self.intern_bound_variable_kinds(xs)) } /// Emit a lint at `span` from a lint struct (some type that implements `DecorateLint`, diff --git a/compiler/rustc_type_ir/src/lib.rs b/compiler/rustc_type_ir/src/lib.rs index d3c0a410bfc..5a991e03dee 100644 --- a/compiler/rustc_type_ir/src/lib.rs +++ b/compiler/rustc_type_ir/src/lib.rs @@ -69,38 +69,37 @@ pub trait Interner: Sized { type PlaceholderRegion: Clone + Debug + Hash + Ord; } -pub trait InternAs { +/// Imagine you have a function `F: FnOnce(&[T]) -> R`, plus an iterator `iter` +/// that produces `T` items. You could combine them with +/// `f(&iter.collect::>())`, but this requires allocating memory for the +/// `Vec`. +/// +/// This trait allows for faster implementations, intended for cases where the +/// number of items produced by the iterator is small. There is a blanket impl +/// for `T` items, but there is also a fallible impl for `Result` items. +pub trait CollectAndApply: Sized { type Output; - fn intern_with(self, f: F) -> Self::Output + + /// Produce a result of type `Self::Output` from `iter`. The result will + /// typically be produced by applying `f` on the elements produced by + /// `iter`, though this may not happen in some impls, e.g. if an error + /// occured during iteration. + fn collect_and_apply(iter: I, f: F) -> Self::Output where + I: Iterator, F: FnOnce(&[T]) -> R; } -impl InternAs for I -where - E: InternIteratorElement, - I: Iterator, -{ - type Output = E::Output; - fn intern_with(self, f: F) -> Self::Output +/// The blanket impl that always collects all elements and applies `f`. +impl CollectAndApply for T { + type Output = R; + + /// Equivalent to `f(&iter.collect::>())`. + fn collect_and_apply(mut iter: I, f: F) -> R where + I: Iterator, F: FnOnce(&[T]) -> R, { - E::intern_with(self, f) - } -} - -pub trait InternIteratorElement: Sized { - type Output; - fn intern_with, F: FnOnce(&[T]) -> R>(iter: I, f: F) -> Self::Output; -} - -impl InternIteratorElement for T { - type Output = R; - fn intern_with, F: FnOnce(&[T]) -> R>( - mut iter: I, - f: F, - ) -> Self::Output { // This code is hot enough that it's worth specializing for the most // common length lists, to avoid the overhead of `SmallVec` creation. // Lengths 0, 1, and 2 typically account for ~95% of cases. If @@ -127,12 +126,17 @@ impl InternIteratorElement for T { } } -impl InternIteratorElement for Result { +/// A fallible impl that will fail, without calling `f`, if there are any +/// errors during collection. +impl CollectAndApply for Result { type Output = Result; - fn intern_with, F: FnOnce(&[T]) -> R>( - mut iter: I, - f: F, - ) -> Self::Output { + + /// Equivalent to `Ok(f(&iter.collect::>>()?))`. + fn collect_and_apply(mut iter: I, f: F) -> Result + where + I: Iterator>, + F: FnOnce(&[T]) -> R, + { // This code is hot enough that it's worth specializing for the most // common length lists, to avoid the overhead of `SmallVec` creation. // Lengths 0, 1, and 2 typically account for ~95% of cases. If