Merge #10387

10387: Move `IdxRange` into la-arena r=Veykril a=arzg

Currently, `IdxRange` (named `IdRange`) is located in `hir_def::item_tree`, when really it isn’t specific to `hir_def` and could become part of la-arena. The rename from `IdRange` to `IdxRange` is to maintain consistency with the naming convention used throughout la-arena (`Idx` instead of `Id`, `RawIdx` instead of `RawId`). This PR also adds a few new APIs to la-arena on top of `IdxRange` for convenience, namely:

- indexing into an `Arena` by an `IdxRange` and getting a slice of values back
- creating an `IdxRange` from an inclusive range

Currently this PR also exposes a new `Arena::next_idx` method to make constructing inclusive`IdxRange`s using `IdxRange::new` easier; however, it would in my opinion be better to remove this as it allows for easy creation of out-of-bounds `Idx`s, when `IdxRange::new_inclusive` mostly covers the same use-case while being less error-prone.

I decided to bump the la-arena version to 0.3.0 from 0.2.0 because adding a new `Index` impl for `Arena` turned out to be a breaking change: I had to add a type hint in `crates/hir_def/src/body/scope.rs` when one wasn’t necessary before, since rustc couldn’t work out the type of a closure parameter now that there are multiple `Index` impls. I’m not sure whether this is the right decision, though. 

Co-authored-by: Aramis Razzaghipour <aramisnoah@gmail.com>

Cargo.lock

@@ -796,7 +796,7 @@ dependencies = [
 
 [[package]]
 name = "la-arena"
-version = "0.2.1"
+version = "0.3.0"
 
 [[package]]
 name = "lazy_static"

crates/hir_def/Cargo.toml

@@ -21,7 +21,7 @@ fst = { version = "0.4", default-features = false }
 itertools = "0.10.0"
 indexmap = "1.4.0"
 smallvec = "1.4.0"
-la-arena = { version = "0.2.0", path = "../../lib/arena" }
+la-arena = { version = "0.3.0", path = "../../lib/arena" }
 
 stdx = { path = "../stdx", version = "0.0.0" }
 base_db = { path = "../base_db", version = "0.0.0" }

crates/hir_def/src/body/scope.rs

@@ -174,7 +174,7 @@ fn compute_block_scopes(
 
 fn compute_expr_scopes(expr: ExprId, body: &Body, scopes: &mut ExprScopes, scope: ScopeId) {
     let make_label =
-        |label: &Option<_>| label.map(|label| (label, body.labels[label].name.clone()));
+        |label: &Option<LabelId>| label.map(|label| (label, body.labels[label].name.clone()));
 
     scopes.set_scope(expr, scope);
     match &body[expr] {

crates/hir_def/src/item_tree.rs

@@ -36,11 +36,10 @@
 mod tests;
 
 use std::{
-    any::type_name,
     fmt::{self, Debug},
     hash::{Hash, Hasher},
     marker::PhantomData,
-    ops::{Index, Range},
+    ops::Index,
     sync::Arc,
 };
 
@@ -53,7 +52,7 @@
     name::{name, AsName, Name},
     ExpandTo, HirFileId, InFile,
 };
-use la_arena::{Arena, Idx, RawIdx};
+use la_arena::{Arena, Idx, IdxRange, RawIdx};
 use profile::Count;
 use rustc_hash::FxHashMap;
 use smallvec::SmallVec;
@@ -606,7 +605,7 @@ pub struct Function {
     pub visibility: RawVisibilityId,
     pub explicit_generic_params: Interned<GenericParams>,
     pub abi: Option<Interned<str>>,
-    pub params: IdRange<Param>,
+    pub params: IdxRange<Param>,
     pub ret_type: Interned<TypeRef>,
     pub async_ret_type: Option<Interned<TypeRef>>,
     pub ast_id: FileAstId<ast::Fn>,
@@ -659,7 +658,7 @@ pub struct Enum {
     pub name: Name,
     pub visibility: RawVisibilityId,
     pub generic_params: Interned<GenericParams>,
-    pub variants: IdRange<Variant>,
+    pub variants: IdxRange<Variant>,
     pub ast_id: FileAstId<ast::Enum>,
 }
 
@@ -947,59 +946,10 @@ pub struct Variant {
     pub fields: Fields,
 }
 
-/// A range of densely allocated ItemTree IDs.
-pub struct IdRange<T> {
-    range: Range<u32>,
-    _p: PhantomData<T>,
-}
-
-impl<T> IdRange<T> {
-    fn new(range: Range<Idx<T>>) -> Self {
-        Self { range: range.start.into_raw().into()..range.end.into_raw().into(), _p: PhantomData }
-    }
-
-    fn is_empty(&self) -> bool {
-        self.range.is_empty()
-    }
-}
-
-impl<T> Iterator for IdRange<T> {
-    type Item = Idx<T>;
-    fn next(&mut self) -> Option<Self::Item> {
-        self.range.next().map(|raw| Idx::from_raw(raw.into()))
-    }
-}
-
-impl<T> DoubleEndedIterator for IdRange<T> {
-    fn next_back(&mut self) -> Option<Self::Item> {
-        self.range.next_back().map(|raw| Idx::from_raw(raw.into()))
-    }
-}
-
-impl<T> fmt::Debug for IdRange<T> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        f.debug_tuple(&format!("IdRange::<{}>", type_name::<T>())).field(&self.range).finish()
-    }
-}
-
-impl<T> Clone for IdRange<T> {
-    fn clone(&self) -> Self {
-        Self { range: self.range.clone(), _p: PhantomData }
-    }
-}
-
-impl<T> PartialEq for IdRange<T> {
-    fn eq(&self, other: &Self) -> bool {
-        self.range == other.range
-    }
-}
-
-impl<T> Eq for IdRange<T> {}
-
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum Fields {
-    Record(IdRange<Field>),
+    Record(IdxRange<Field>),
-    Tuple(IdRange<Field>),
+    Tuple(IdxRange<Field>),
     Unit,
 }
 

crates/hir_def/src/item_tree/lower.rs

@@ -229,7 +229,7 @@ fn lower_fields(&mut self, strukt_kind: &ast::StructKind) -> Fields {
         }
     }
 
-    fn lower_record_fields(&mut self, fields: &ast::RecordFieldList) -> IdRange<Field> {
+    fn lower_record_fields(&mut self, fields: &ast::RecordFieldList) -> IdxRange<Field> {
         let start = self.next_field_idx();
         for field in fields.fields() {
             if let Some(data) = self.lower_record_field(&field) {
@@ -238,7 +238,7 @@ fn lower_record_fields(&mut self, fields: &ast::RecordFieldList) -> IdRange<Fiel
             }
         }
         let end = self.next_field_idx();
-        IdRange::new(start..end)
+        IdxRange::new(start..end)
     }
 
     fn lower_record_field(&mut self, field: &ast::RecordField) -> Option<Field> {
@@ -249,7 +249,7 @@ fn lower_record_field(&mut self, field: &ast::RecordField) -> Option<Field> {
         Some(res)
     }
 
-    fn lower_tuple_fields(&mut self, fields: &ast::TupleFieldList) -> IdRange<Field> {
+    fn lower_tuple_fields(&mut self, fields: &ast::TupleFieldList) -> IdxRange<Field> {
         let start = self.next_field_idx();
         for (i, field) in fields.fields().enumerate() {
             let data = self.lower_tuple_field(i, &field);
@@ -257,7 +257,7 @@ fn lower_tuple_fields(&mut self, fields: &ast::TupleFieldList) -> IdRange<Field>
             self.add_attrs(idx.into(), RawAttrs::new(self.db, &field, &self.hygiene));
         }
         let end = self.next_field_idx();
-        IdRange::new(start..end)
+        IdxRange::new(start..end)
     }
 
     fn lower_tuple_field(&mut self, idx: usize, field: &ast::TupleField) -> Field {
@@ -273,7 +273,7 @@ fn lower_union(&mut self, union: &ast::Union) -> Option<FileItemTreeId<Union>> {
         let generic_params = self.lower_generic_params(GenericsOwner::Union, union);
         let fields = match union.record_field_list() {
             Some(record_field_list) => self.lower_fields(&StructKind::Record(record_field_list)),
-            None => Fields::Record(IdRange::new(self.next_field_idx()..self.next_field_idx())),
+            None => Fields::Record(IdxRange::new(self.next_field_idx()..self.next_field_idx())),
         };
         let ast_id = self.source_ast_id_map.ast_id(union);
         let res = Union { name, visibility, generic_params, fields, ast_id };
@@ -287,14 +287,14 @@ fn lower_enum(&mut self, enum_: &ast::Enum) -> Option<FileItemTreeId<Enum>> {
         let variants =
             self.with_inherited_visibility(visibility, |this| match &enum_.variant_list() {
                 Some(variant_list) => this.lower_variants(variant_list),
-                None => IdRange::new(this.next_variant_idx()..this.next_variant_idx()),
+                None => IdxRange::new(this.next_variant_idx()..this.next_variant_idx()),
             });
         let ast_id = self.source_ast_id_map.ast_id(enum_);
         let res = Enum { name, visibility, generic_params, variants, ast_id };
         Some(id(self.data().enums.alloc(res)))
     }
 
-    fn lower_variants(&mut self, variants: &ast::VariantList) -> IdRange<Variant> {
+    fn lower_variants(&mut self, variants: &ast::VariantList) -> IdxRange<Variant> {
         let start = self.next_variant_idx();
         for variant in variants.variants() {
             if let Some(data) = self.lower_variant(&variant) {
@@ -303,7 +303,7 @@ fn lower_variants(&mut self, variants: &ast::VariantList) -> IdRange<Variant> {
             }
         }
         let end = self.next_variant_idx();
-        IdRange::new(start..end)
+        IdxRange::new(start..end)
     }
 
     fn lower_variant(&mut self, variant: &ast::Variant) -> Option<Variant> {
@@ -358,7 +358,7 @@ fn lower_function(&mut self, func: &ast::Fn) -> Option<FileItemTreeId<Function>>
             }
         }
         let end_param = self.next_param_idx();
-        let params = IdRange::new(start_param..end_param);
+        let params = IdxRange::new(start_param..end_param);
 
         let ret_type = match func.ret_type().and_then(|rt| rt.ty()) {
             Some(type_ref) => TypeRef::from_ast(&self.body_ctx, type_ref),

crates/hir_expand/Cargo.toml

@@ -13,7 +13,7 @@ cov-mark = "2.0.0-pre.1"
 tracing = "0.1"
 either = "1.5.3"
 rustc-hash = "1.0.0"
-la-arena = { version = "0.2.0", path = "../../lib/arena" }
+la-arena = { version = "0.3.0", path = "../../lib/arena" }
 itertools = "0.10.0"
 
 base_db = { path = "../base_db", version = "0.0.0" }

crates/hir_ty/Cargo.toml

@@ -20,7 +20,7 @@ scoped-tls = "1"
 chalk-solve = { version = "0.71", default-features = false }
 chalk-ir = "0.71"
 chalk-recursive = { version = "0.71", default-features = false }
-la-arena = { version = "0.2.0", path = "../../lib/arena" }
+la-arena = { version = "0.3.0", path = "../../lib/arena" }
 once_cell = { version = "1.5.0" }
 
 stdx = { path = "../stdx", version = "0.0.0" }

crates/profile/Cargo.toml

@@ -12,7 +12,7 @@ doctest = false
 once_cell = "1.3.1"
 cfg-if = "1"
 libc = "0.2"
-la-arena = { version = "0.2.0", path = "../../lib/arena" }
+la-arena = { version = "0.3.0", path = "../../lib/arena" }
 countme = { version = "2.0.1", features = ["enable"] }
 jemalloc-ctl = { version = "0.4.1", package = "tikv-jemalloc-ctl", optional = true }
 

crates/project_model/Cargo.toml

@@ -17,7 +17,7 @@ serde = { version = "1.0.106", features = ["derive"] }
 serde_json = "1.0.48"
 anyhow = "1.0.26"
 expect-test = "1.2.0-pre.1"
-la-arena = { version = "0.2.0", path = "../../lib/arena" }
+la-arena = { version = "0.3.0", path = "../../lib/arena" }
 
 cfg = { path = "../cfg", version = "0.0.0" }
 base_db = { path = "../base_db", version = "0.0.0" }

lib/arena/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "la-arena"
-version = "0.2.1"
+version = "0.3.0"
 description = "Simple index-based arena without deletion."
 license = "MIT OR Apache-2.0"
 repository = "https://github.com/rust-analyzer/rust-analyzer"

lib/arena/src/lib.rs

@@ -7,7 +7,7 @@
     hash::{Hash, Hasher},
     iter::FromIterator,
     marker::PhantomData,
-    ops::{Index, IndexMut},
+    ops::{Index, IndexMut, Range, RangeInclusive},
 };
 
 mod map;
@@ -89,6 +89,101 @@ pub fn into_raw(self) -> RawIdx {
     }
 }
 
+/// A range of densely allocated arena values.
+pub struct IdxRange<T> {
+    range: Range<u32>,
+    _p: PhantomData<T>,
+}
+
+impl<T> IdxRange<T> {
+    /// Creates a new index range
+    /// inclusive of the start value and exclusive of the end value.
+    ///
+    /// ```
+    /// let mut arena = la_arena::Arena::new();
+    /// let a = arena.alloc("a");
+    /// let b = arena.alloc("b");
+    /// let c = arena.alloc("c");
+    /// let d = arena.alloc("d");
+    ///
+    /// let range = la_arena::IdxRange::new(b..d);
+    /// assert_eq!(&arena[range], &["b", "c"]);
+    /// ```
+    pub fn new(range: Range<Idx<T>>) -> Self {
+        Self { range: range.start.into_raw().into()..range.end.into_raw().into(), _p: PhantomData }
+    }
+
+    /// Creates a new index range
+    /// inclusive of the start value and end value.
+    ///
+    /// ```
+    /// let mut arena = la_arena::Arena::new();
+    /// let foo = arena.alloc("foo");
+    /// let bar = arena.alloc("bar");
+    /// let baz = arena.alloc("baz");
+    ///
+    /// let range = la_arena::IdxRange::new_inclusive(foo..=baz);
+    /// assert_eq!(&arena[range], &["foo", "bar", "baz"]);
+    ///
+    /// let range = la_arena::IdxRange::new_inclusive(foo..=foo);
+    /// assert_eq!(&arena[range], &["foo"]);
+    /// ```
+    pub fn new_inclusive(range: RangeInclusive<Idx<T>>) -> Self {
+        Self {
+            range: u32::from(range.start().into_raw())..u32::from(range.end().into_raw()) + 1,
+            _p: PhantomData,
+        }
+    }
+
+    /// Returns whether the index range is empty.
+    ///
+    /// ```
+    /// let mut arena = la_arena::Arena::new();
+    /// let one = arena.alloc(1);
+    /// let two = arena.alloc(2);
+    ///
+    /// assert!(la_arena::IdxRange::new(one..one).is_empty());
+    /// ```
+    pub fn is_empty(&self) -> bool {
+        self.range.is_empty()
+    }
+}
+
+impl<T> Iterator for IdxRange<T> {
+    type Item = Idx<T>;
+    fn next(&mut self) -> Option<Self::Item> {
+        self.range.next().map(|raw| Idx::from_raw(raw.into()))
+    }
+}
+
+impl<T> DoubleEndedIterator for IdxRange<T> {
+    fn next_back(&mut self) -> Option<Self::Item> {
+        self.range.next_back().map(|raw| Idx::from_raw(raw.into()))
+    }
+}
+
+impl<T> fmt::Debug for IdxRange<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_tuple(&format!("IdxRange::<{}>", std::any::type_name::<T>()))
+            .field(&self.range)
+            .finish()
+    }
+}
+
+impl<T> Clone for IdxRange<T> {
+    fn clone(&self) -> Self {
+        Self { range: self.range.clone(), _p: PhantomData }
+    }
+}
+
+impl<T> PartialEq for IdxRange<T> {
+    fn eq(&self, other: &Self) -> bool {
+        self.range == other.range
+    }
+}
+
+impl<T> Eq for IdxRange<T> {}
+
 /// Yet another index-based arena.
 #[derive(Clone, PartialEq, Eq, Hash)]
 pub struct Arena<T> {
@@ -170,9 +265,9 @@ pub fn is_empty(&self) -> bool {
     /// assert_eq!(arena[idx], 50);
     /// ```
     pub fn alloc(&mut self, value: T) -> Idx<T> {
-        let idx = RawIdx(self.data.len() as u32);
+        let idx = self.next_idx();
         self.data.push(value);
-        Idx::from_raw(idx)
+        idx
     }
 
     /// Returns an iterator over the arena’s elements.
@@ -221,6 +316,13 @@ pub fn iter_mut(
     pub fn shrink_to_fit(&mut self) {
         self.data.shrink_to_fit();
     }
+
+    /// Returns the index of the next value allocated on the arena.
+    ///
+    /// This method should remain private to make creating invalid `Idx`s harder.
+    fn next_idx(&self) -> Idx<T> {
+        Idx::from_raw(RawIdx(self.data.len() as u32))
+    }
 }
 
 impl<T> Default for Arena<T> {
@@ -244,6 +346,15 @@ fn index_mut(&mut self, idx: Idx<T>) -> &mut T {
     }
 }
 
+impl<T> Index<IdxRange<T>> for Arena<T> {
+    type Output = [T];
+    fn index(&self, range: IdxRange<T>) -> &[T] {
+        let start = range.range.start as usize;
+        let end = range.range.end as usize;
+        &self.data[start..end]
+    }
+}
+
 impl<T> FromIterator<T> for Arena<T> {
     fn from_iter<I>(iter: I) -> Self
     where