Cleanup the code a bit (a lot) (#1595)

* Removed explicit state from Traverse trait

* Make clippy happy

* Consistent whitespace between documented items

I kept running into this while going through the codebase,
so I thought I'd fix it once and for all.

And no, I did not do this by hand. So I might have missed some cases.

* More formatting.

Mainly includes wrapping of comments to the 100 column mark,
as well as some more formatting on macros and strings.

* restore try_map_state in Traverse impl

* Removed IdentKind

It was technically only used in the PartialEq implmentation of Thunk.
Still, I don't think that had any effect.

* Fix doc comment

Co-authored-by: Yann Hamdaoui <yann.hamdaoui@gmail.com>

---------

Co-authored-by: Taeer Bar-Yam <taeer.bar-yam@tweag.io>
Co-authored-by: Yann Hamdaoui <yann.hamdaoui@gmail.com>
Co-authored-by: Yann Hamdaoui <yann.hamdaoui@tweag.io>

cli/src/export.rs

@@ -181,8 +181,9 @@ impl TermInterface {
             term_cmd.clap_cmd = term_cmd.clap_cmd.after_long_help(format!(
                 "\
             [CONFLICT] This configuration has a field named `{conflict_field}` which conflicts \
-            with the built-in `--{conflict_field}` argument. To set this field to e.g. \"some_value\", \
-            use `--override {conflict_field} \"some_value\"` instead of `--{conflict_field} \"some_value\"`\
+            with the built-in `--{conflict_field}` argument. To set this field to e.g. \
+            \"some_value\", use `--override {conflict_field} \"some_value\"` instead of \
+            `--{conflict_field} \"some_value\"`\
             {extra}\n\n\
             {EXPERIMENTAL_MSG}"
             ));

core/src/cache.rs

@@ -113,6 +113,12 @@ impl Envs {
     }
 }
 
+impl Default for Envs {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 /// An entry in the term cache. Stores the parsed term together with some metadata and state.
 #[derive(Debug, Clone, PartialEq)]
 pub struct TermEntry {
@@ -166,7 +172,8 @@ pub struct NameIdEntry {
 pub enum EntryState {
     /// The term have just been parsed.
     Parsed,
-    /// The imports of the entry have been resolved, and the imports of its (transitive) imports are being resolved.
+    /// The imports of the entry have been resolved, and the imports of its (transitive) imports are
+    /// being resolved.
     ImportsResolving,
     /// The imports of the entry and its transitive dependencies has been resolved.
     ImportsResolved,
@@ -553,7 +560,10 @@ impl Cache {
                     .collect::<Result<Vec<_>, _>>()?;
 
                 if terms.is_empty() {
-                    unreachable!("serde always produces at least one document, the empty string turns into `null`")
+                    unreachable!(
+                        "serde always produces at least one document, \
+                        the empty string turns into `null`"
+                    )
                 } else if terms.len() == 1 {
                     Ok((
                         terms.pop().expect("we just checked the length"),
@@ -1014,7 +1024,8 @@ impl Cache {
         self.terms.get(&file_id).map(|TermEntry { term, .. }| term)
     }
 
-    /// Returns true if a particular file id represents a Nickel standard library file, false otherwise.
+    /// Returns true if a particular file id represents a Nickel standard library file, false
+    /// otherwise.
     pub fn is_stdlib_module(&self, file: FileId) -> bool {
         let Some(table) = &self.stdlib_ids else {
             return false;
@@ -1097,8 +1108,8 @@ impl Cache {
         // We have a small bootstraping problem: to typecheck the initial environment, we already
         // need an initial evaluation environment, since stdlib parts may reference each other. But
         // typechecking is performed before program transformations, so this environment is not
-        // final one. We have create a temporary initial environment just for typechecking, which is dropped
-        // right after. However:
+        // final one. We have create a temporary initial environment just for typechecking, which is
+        // dropped right after. However:
         // 1. The stdlib is meant to stay relatively light.
         // 2. Typechecking the standard library ought to occur only during development. Once the
         //    stdlib is stable, we won't have typecheck it at every execution.
@@ -1128,10 +1139,10 @@ impl Cache {
         }
     }
 
-    /// Load, parse, and apply program transformations to the standard library. Do not typecheck
-    /// for performance reasons: this is done in the test suite. Return an initial environment
-    /// containing both the evaluation and type environments. If you only need the type environment, use
-    /// `load_stdlib` then `mk_type_env` to avoid transformations and evaluation preparation.
+    /// Load, parse, and apply program transformations to the standard library. Do not typecheck for
+    /// performance reasons: this is done in the test suite. Return an initial environment
+    /// containing both the evaluation and type environments. If you only need the type environment,
+    /// use `load_stdlib` then `mk_type_env` to avoid transformations and evaluation preparation.
     pub fn prepare_stdlib<EC: EvalCache>(&mut self, eval_cache: &mut EC) -> Result<Envs, Error> {
         #[cfg(debug_assertions)]
         if self.skip_stdlib {
@@ -1191,8 +1202,8 @@ impl Cache {
         Ok(typecheck::mk_initial_ctxt(&stdlib_terms_vec).unwrap())
     }
 
-    /// Generate the initial evaluation environment from the list of `file_ids` corresponding to the standard
-    /// library parts.
+    /// Generate the initial evaluation environment from the list of `file_ids` corresponding to the
+    /// standard library parts.
     pub fn mk_eval_env<EC: EvalCache>(
         &self,
         eval_cache: &mut EC,
@@ -1211,17 +1222,23 @@ impl Cache {
                         )),
                     );
                     if let Err(eval::EnvBuildError::NotARecord(rt)) = result {
-                         panic!(
-                            "cache::load_stdlib(): expected the stdlib module {} to be a record, got {:?}",
+                        panic!(
+                            "cache::load_stdlib(): \
+                            expected the stdlib module {} to be a record, got {:?}",
                             self.name(*file_id).to_string_lossy().as_ref(),
                             rt
-                         )
+                        )
                     }
-                }
-                else {
-                    eval::env_add(eval_cache, &mut eval_env, module.name().into(), self.get_owned(*file_id).expect(
+                } else {
+                    eval::env_add(
+                        eval_cache,
+                        &mut eval_env,
+                        module.name().into(),
+                        self.get_owned(*file_id).expect(
                             "cache::mk_eval_env(): can't build environment, stdlib not parsed",
-                        ), eval::Environment::new());
+                        ),
+                        eval::Environment::new(),
+                    );
                 }
             });
 
@@ -1233,15 +1250,15 @@ impl Cache {
 }
 
 /// Abstract the access to imported files and the import cache. Used by the evaluator, the
-/// typechecker and at [import resolution](../transformations/import_resolution/index.html) phase.
+/// typechecker and at the [import resolution](crate::transform::import_resolution) phase.
 ///
 /// The standard implementation uses 2 caches, the file cache for raw contents and the term cache
 /// for parsed contents, mirroring the 2 steps when resolving an import:
 /// 1. When an import is encountered for the first time, the content of the corresponding file is
 ///    read and stored in the file cache (consisting of the file database plus a map between paths
 ///    and ids in the database, the name-id table). The content is parsed, stored in the term
 ///    cache, and queued somewhere so that it can undergo the standard
-///    [transformations](../transformations/index.html) (including import resolution) later.
+///    [transformations](crate::transform) (including import resolution) later.
 /// 2. When it is finally processed, the term cache is updated with the transformed term.
 pub trait ImportResolver {
     /// Resolve an import.

core/src/deserialize.rs

@@ -626,9 +626,10 @@ mod tests {
         }
 
         assert_eq!(
-            A::deserialize(
-                eval(r#"{ a = 10, b = "test string", c = null, d = true, e = 'foo, f = null, g = -10, h = { bar = "some other string" } }"#)
-            )
+            A::deserialize(eval(
+                r#"{ a = 10, b = "test string", c = null, d = true, e = 'foo, f = null,
+                g = -10, h = { bar = "some other string" } }"#
+            ))
             .expect("deserialization shouldn't fail"),
             A {
                 a: 10.0,

core/src/environment.rs

@@ -88,7 +88,8 @@ impl<K: Hash + Eq, V: PartialEq> Environment<K, V> {
             env: if !self.was_cloned() {
                 Some(NonNull::from(self))
             } else {
-                // if was cloned, current is the same as first of previous (that cannot be empty then)
+                // if was cloned, current is the same as first of previous (that cannot be empty
+                // then)
                 self.previous
                     .borrow()
                     .as_ref()
@@ -99,25 +100,26 @@ impl<K: Hash + Eq, V: PartialEq> Environment<K, V> {
         }
     }
 
-    /// Creates an iterator that visits all elements from the Environment, from the oldest layer
-    /// to the most recent one. It uses this order, so calling `collect` on this iterator to create
-    /// a hashmap would have the same values as the Environment.
-    /// The element iterator type is `(&'env K, &'env V)`, with `'env` being the lifetime of the Environment.
+    /// Creates an iterator that visits all elements from the Environment, from the oldest layer to
+    /// the most recent one. It uses this order, so calling `collect` on this iterator to create a
+    /// hashmap would have the same values as the Environment. The element iterator type is `(&'env
+    /// K, &'env V)`, with `'env` being the lifetime of the Environment.
     pub fn iter_elems(&self) -> EnvElemIter<'_, K, V> {
         let mut env: Vec<NonNull<HashMap<K, V>>> = self
             .iter_layers()
-            // SAFETY: all NonNull::new_unchecked comes from pointers created from Rc, so cannot be null
+            // SAFETY: Rc::as_ptr never returnes null
             .map(|hmap| unsafe { NonNull::new_unchecked(Rc::as_ptr(hmap) as *mut _) })
             .collect();
         // SAFETY: by design, env cannot be empty, and coming from an Rc, it is well aligned and initialized
         let current_map = unsafe { env.pop().unwrap().as_ref() }.iter();
         EnvElemIter { env, current_map }
     }
 
-    /// Creates an iterator that visits all elements from the Environment, from the current layer to the oldest one.
-    /// If values are present multiple times, only the most recent one appears.
-    /// [`iter_elems`] should be preferred, since it does not need to create an intermediary hashmap.
-    /// The element iterator type is `(&'env K, &'env V)`, with `'env` being the lifetime of the Environment.
+    /// Creates an iterator that visits all elements from the Environment, from the current layer to
+    /// the oldest one. If values are present multiple times, only the most recent one appears.
+    /// [`iter_elems`] should be preferred, since it does not need to create an intermediary
+    /// hashmap. The element iterator type is `(&'env K, &'env V)`, with `'env` being the lifetime
+    /// of the Environment.
     ///
     /// [`iter_elems`]: Environment::iter_elems
     ///
@@ -184,7 +186,8 @@ impl<'a, K: 'a + Hash + Eq, V: 'a + PartialEq> Iterator for EnvLayerIter<'a, K,
     }
 }
 
-/// An iterator over all the elements inside the `Environment`, from the oldest layer to the current one.
+/// An iterator over all the elements inside the `Environment`, from the oldest layer to the current
+/// one.
 ///
 /// Created by the [`Environment::iter_elems`] method.
 ///