Clarify iterator interners #108112

There are several `mk_foo`/`intern_foo` pairs, where the former takes an iterator and the latter takes a slice. (This naming convention is bad, but that's a fix for another PR.) This commit changes several `mk_foo` occurrences into `intern_foo`, avoiding the need for some `.iter()`/`.into_iter()` calls. Affected cases: - mk_type_list - mk_tup - mk_substs - mk_const_list

Giving the item type a name `T` avoids duplication.

`InternIteratorElement` is a trait used to intern values produces by iterators. There are three impls, corresponding to iterators that produce different types: - One for `T`, which operates straightforwardly. - One for `Result<T, E>`, which is fallible, and will fail early with an error result if any of the iterator elements are errors. - One for `&'a T`, which clones the items as it iterates. That last one is bad: it's extremely easy to use it without realizing that it clones, which goes against Rust's normal "explicit is better" approach to cloning. So this commit just removes it. In practice, there weren't many use sites. For all but one of them `into_iter()` could be used, which avoids the need for cloning. And for the one remaining case `copied()` is used.

There are two traits, `InternAs` and `InternIteratorElement`. I found them confusing to use, particularly this: ``` pub fn mk_tup<I: InternAs<Ty<'tcx>, 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::<Vec<_>>())`. 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.

This makes a lot of call sites nicer.

This function has this line twice: ``` let bound_vars = tcx.intern_bound_variable_kinds(&bound_vars); ``` The second occurrence is effectively a no-op, because the first occurrence interned any that needed it.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify iterator interners #108112

Clarify iterator interners #108112

Commits on Feb 17, 2023