Repository: tweag/nickel
Pull request #972 (2022-12-05 17:46:13)

Re-enable rustfmt pre-commit check
Closes https://github.com/tweag/nickel/issues/971


Patch:
diff --git a/flake.nix b/flake.nix
index 499b0f7c..328fed4b 100644
--- a/flake.nix
+++ b/flake.nix
@@ -131,7 +131,11 @@
             targets = [ target ];
           };
 
-      pre-commit-builder = { rust ? mkRust { } }: pre-commit-hooks.lib.${system}.run {
+      # A note on check_format: the way we invoke rustfmt here works locally but fails on CI.
+      # Since the formatting is checked on CI anyway - as part of the rustfmt check - we
+      # disable rustfmt in the pre-commit hook when running checks, but enable it when
+      # running in a dev shell.
+      pre-commit-builder = { rust ? mkRust { }, checkFormat ? false }: pre-commit-hooks.lib.${system}.run {
         src = self;
         hooks = {
           nixpkgs-fmt = {
@@ -145,9 +149,8 @@
           };
 
           rustfmt = {
-            enable = true;
-            # Silly test to ensure Crane's `rustfmt` check was run
-            entry = pkgs.lib.mkForce "test -d ${(mkCraneArtifacts {inherit rust;}).rustfmt}/target";
+            enable = checkFormat;
+            entry = pkgs.lib.mkForce "${rust}/bin/cargo-fmt fmt -- --check --color always";
           };
 
           markdownlint = {
@@ -241,7 +244,7 @@
           pkgs.nodePackages.markdownlint-cli
         ];
 
-        shellHook = (pre-commit-builder { inherit rust; }).shellHook + ''
+        shellHook = (pre-commit-builder { inherit rust; checkFormat = true; }).shellHook + ''
           echo "=== Nickel development shell ==="
           echo "Info: Git hooks can be installed using \`pre-commit install\`"
         '';
diff --git a/src/eval/operation.rs b/src/eval/operation.rs
index 470644f0..01f9cb20 100644
--- a/src/eval/operation.rs
+++ b/src/eval/operation.rs
@@ -1216,8 +1216,8 @@ impl<R: ImportResolver> VirtualMachine<R> {
                     })
                 }
             }
-            UnaryOp::PushDefault() => Ok(PushPriority::Bottom.push_into(t, env, pos)),
-            UnaryOp::PushForce() => Ok(PushPriority::Top.push_into(t, env, pos)),
+            UnaryOp::RecDefault() => Ok(RecPriority::Bottom.propagate_in_term(t, env, pos)),
+            UnaryOp::RecForce() => Ok(RecPriority::Top.propagate_in_term(t, env, pos)),
             UnaryOp::RecordEmptyWithTail() => match_sharedterm! { t,
                 with {
                     Term::Record(r) => {
@@ -2783,56 +2783,56 @@ impl<R: ImportResolver> VirtualMachine<R> {
     }
 }
 
-/// A merge priority that can be pushed down to the leafs of a record. Currently only `default`
-/// (`Bottom`) and `force` (`Top`) can be pushed down a value.
+/// A merge priority that can be recursively pushed down to the leafs of a record. Currently only
+/// `default` (`Bottom`) and `force` (`Top`) can be recursive.
 #[derive(Debug, Clone, Copy, Eq, PartialEq)]
-pub enum PushPriority {
+pub enum RecPriority {
     Bottom,
     Top,
 }
 
-impl From<PushPriority> for MergePriority {
-    fn from(push_prio: PushPriority) -> Self {
-        match push_prio {
-            PushPriority::Top => MergePriority::Top,
-            PushPriority::Bottom => MergePriority::Bottom,
+impl From<RecPriority> for MergePriority {
+    fn from(rec_prio: RecPriority) -> Self {
+        match rec_prio {
+            RecPriority::Top => MergePriority::Top,
+            RecPriority::Bottom => MergePriority::Bottom,
         }
     }
 }
 
-impl PushPriority {
-    /// Return the push operator corresponding to this priority (`$push_force` or `$push_default`)
-    /// applied to the given term.
-    pub fn apply_push_op(&self, rt: RichTerm) -> RichTerm {
+impl RecPriority {
+    /// Return the recursive priority operator corresponding to this priority (`$rec_force` or
+    /// `$rec_default`) applied to the given term.
+    pub fn apply_rec_prio_op(&self, rt: RichTerm) -> RichTerm {
         let pos = rt.pos;
 
         let op = match self {
-            PushPriority::Top => internals::push_force(),
-            PushPriority::Bottom => internals::push_default(),
+            RecPriority::Top => internals::rec_force(),
+            RecPriority::Bottom => internals::rec_default(),
         };
 
         mk_app!(op, rt).with_pos(pos)
     }
 
-    /// Push the priority down the fields of a record.
-    fn push_into_record(&self, record: RecordData, env: &Environment, pos: TermPos) -> Closure {
+    /// Propagate the priority down the fields of a record.
+    fn propagate_in_record(&self, record: RecordData, env: &Environment, pos: TermPos) -> Closure {
         let mut new_env = Environment::new();
 
         let record = record.map_fields_without_optionals(env, |_, rt| {
             // There is a subtlety with respect to overriding here. Take:
             //
             // ```nickel
-            // ({foo = bar + 1, bar = 1} | _push_default) & {bar = 2}
+            // ({foo = bar + 1, bar = 1} | rec default) & {bar = 2}
             // ```
             //
-            // In the example above, if we just map `$push_default` on the value of `foo` and
+            // In the example above, if we just map `$rec_default` on the value of `foo` and
             // closurize it into a new, normal thunk (non revertible), we lose the ability to
             // override `foo` and we end up with the unexpected result `{foo = 2, bar = 2}`.
             //
             // What we want is that:
             //
             // ```nickel
-            // {foo = bar + 1, bar = 1} | _push_default
+            // {foo = bar + 1, bar = 1} | rec default
             // ```
             //
             // is equivalent to writing:
@@ -2854,13 +2854,13 @@ impl PushPriority {
                     env.get(id)
                         .unwrap()
                         .map(|Closure { ref body, ref env }| Closure {
-                            body: self.apply_push_op(body.clone()),
+                            body: self.apply_rec_prio_op(body.clone()),
                             env: env.clone(),
                         })
                 }
                 _ => eval::lazy::Thunk::new(
                     Closure {
-                        body: self.apply_push_op(rt),
+                        body: self.apply_rec_prio_op(rt),
                         env: env.clone(),
                     },
                     eval::IdentKind::Record,
@@ -2884,7 +2884,7 @@ impl PushPriority {
     ///
     /// - `st` must represent an evaluated term (a weak head normal form), that is `st.is_whnf()`
     ///   must be true. Otherwise, this function panics
-    fn push_into(&self, st: SharedTerm, env: Environment, pos: TermPos) -> Closure {
+    fn propagate_in_term(&self, st: SharedTerm, env: Environment, pos: TermPos) -> Closure {
         let update_priority = |meta: &mut MetaValue| {
             if let MergePriority::Neutral = meta.priority {
                 meta.priority = (*self).into();
@@ -2894,7 +2894,7 @@ impl PushPriority {
         let t = st.into_owned();
 
         if let Term::Record(record) = t {
-            self.push_into_record(record, &env, pos)
+            self.propagate_in_record(record, &env, pos)
         } else {
             // Extract the metavalue, or if `st` isn't a metavalue, wrap it in a new one to set the merge
             // priority.
@@ -2916,7 +2916,7 @@ impl PushPriority {
             };
 
             if let Some(inner) = meta.value.take() {
-                // The term is expected to be forced before calling to push_priority, which means that
+                // The term is expected to be forced before calling to rec_priority, which means that
                 // inner is either a simple value, or a thunk containing a weak head normal form.
                 let (inner, env_inner) = if let Term::Var(id) = inner.as_ref() {
                     let closure = env.get(id).unwrap().get_owned();
@@ -2932,7 +2932,7 @@ impl PushPriority {
                         let Closure {
                             body,
                             env: record_env,
-                        } = self.push_into_record(record, &env_inner, pos_inner);
+                        } = self.propagate_in_record(record, &env_inner, pos_inner);
                         meta.value = Some(body.closurize(&mut env, record_env));
                     }
                     t if t.is_whnf() => {
@@ -2940,7 +2940,7 @@ impl PushPriority {
                         meta.value =
                             Some(RichTerm::new(t, pos_inner).closurize(&mut env, env_inner));
                     }
-                    _ => panic!("push_priority: expected an evaluated form"),
+                    _ => panic!("rec_priority: expected an evaluated form"),
                 }
             } else {
                 update_priority(&mut meta);
diff --git a/src/parser/grammar.lalrpop b/src/parser/grammar.lalrpop
index 5edf86e4..164b76fa 100644
--- a/src/parser/grammar.lalrpop
+++ b/src/parser/grammar.lalrpop
@@ -124,8 +124,8 @@ LetAnnotAtom<TypeRule>: MetaAnnot = {
     },
 }
 
-// A single field metadata annotation, without the pseudo-metadata (such as push
-// priorities).
+// A single field metadata annotation, without the pseudo-metadata (such as
+// recursive priorities).
 //
 // The rule to use for type annotations is given as a parameter (cf AnnotAtom
 // rule).
@@ -156,12 +156,12 @@ SimpleFieldAnnotAtom<TypeRule>: MetaAnnot = {
 // rule).
 FieldAnnotAtom<TypeRule>: FieldAnnot = {
     <SimpleFieldAnnotAtom<TypeRule>> => <>.into(),
-    "|" "push force" => FieldAnnot {
-        push_force: true,
+    "|" "rec" "force" => FieldAnnot {
+        rec_force: true,
         ..Default::default()
     },
-    "|" "push default" => FieldAnnot {
-        push_default: true,
+    "|" "rec" "default" => FieldAnnot {
+        rec_default: true,
         ..Default::default()
     },
 }
@@ -174,7 +174,7 @@ Annot<TypeRule>: MetaAnnot = AnnotSeries<AnnotAtom<TypeRule>>;
 LetAnnot<TypeRule>: MetaAnnot = AnnotSeries<LetAnnotAtom<TypeRule>>;
 
 // A simple field annotation, with possibly many annotations chained. A simple
-// field annotation excludes pseudo metadata like push priorities operator.
+// field annotation excludes pseudo metadata like recursive priorities operator.
 SimpleFieldAnnot<TypeRule>: MetaAnnot = AnnotSeries<SimpleFieldAnnotAtom<TypeRule>>;
 
 // A field annotation, with possibly many annotations chained.
@@ -384,14 +384,14 @@ RecordField: (FieldPath, RichTerm) = {
             .map(|t| t.pos.clone())
             .unwrap_or(mk_pos(src_id, l, r));
         let term = match (ann, t) {
-            (Some(FieldAnnot { meta, push_force: false, push_default: false }), None) => {
+            (Some(FieldAnnot { meta, rec_force: false, rec_default: false }), None) => {
                 RichTerm::new(Term::MetaValue(meta), pos)
             }
             (Some(ann), Some(t)) => {
                 ann.attach(t, pos)
             }
             (Some(_), None) => {
-                panic!("can't use push default/push force on a field without definition");
+                panic!("can't use rec default/rec force on a field without definition");
             }
             (None, Some(t)) => t,
             (None, None) => RichTerm::new(Term::MetaValue(MetaValue::new()), pos),
@@ -581,8 +581,8 @@ UOp: UnaryOp = {
     "enum_from" => UnaryOp::EnumFromStr(),
     "str_is_match" => UnaryOp::StrIsMatch(),
     "str_match" => UnaryOp::StrMatch(),
-    "push_force_op" => UnaryOp::PushForce(),
-    "push_default_op" => UnaryOp::PushDefault(),
+    "rec_force_op" => UnaryOp::RecForce(),
+    "rec_default_op" => UnaryOp::RecDefault(),
     "record_empty_with_tail" => UnaryOp::RecordEmptyWithTail(),
 };
 
@@ -922,8 +922,8 @@ extern {
         "fields" => Token::Normal(NormalToken::FieldsOf),
         "values" => Token::Normal(NormalToken::ValuesOf),
         "pow" => Token::Normal(NormalToken::Pow),
-        "push_force_op" => Token::Normal(NormalToken::PushForceOp),
-        "push_default_op" => Token::Normal(NormalToken::PushDefaultOp),
+        "rec_force_op" => Token::Normal(NormalToken::RecForceOp),
+        "rec_default_op" => Token::Normal(NormalToken::RecDefaultOp),
 
         "has_field" => Token::Normal(NormalToken::HasField),
         "map" => Token::Normal(NormalToken::Map),
@@ -936,8 +936,6 @@ extern {
         "doc" => Token::Normal(NormalToken::Doc),
         "optional" => Token::Normal(NormalToken::Optional),
         "priority" => Token::Normal(NormalToken::Priority),
-        "push force" => Token::Normal(NormalToken::PushForce),
-        "push default" => Token::Normal(NormalToken::PushDefault),
 
         "hash" => Token::Normal(NormalToken::OpHash),
         "serialize" => Token::Normal(NormalToken::Serialize),
diff --git a/src/parser/lexer.rs b/src/parser/lexer.rs
index 876a25fa..8da08c48 100644
--- a/src/parser/lexer.rs
+++ b/src/parser/lexer.rs
@@ -230,10 +230,10 @@ pub enum NormalToken<'input> {
     ElemAt,
     #[token("%generate%")]
     ArrayGen,
-    #[token("%push_force%")]
-    PushForceOp,
-    #[token("%push_default%")]
-    PushDefaultOp,
+    #[token("%rec_force%")]
+    RecForceOp,
+    #[token("%rec_default%")]
+    RecDefaultOp,
 
     #[token("merge")]
     Merge,
@@ -247,10 +247,6 @@ pub enum NormalToken<'input> {
     Priority,
     #[token("force")]
     Force,
-    #[token("_push_force")]
-    PushForce,
-    #[token("_push_default")]
-    PushDefault,
 
     #[token("%hash%")]
     OpHash,
diff --git a/src/parser/utils.rs b/src/parser/utils.rs
index 85eb734c..eaa6a747 100644
--- a/src/parser/utils.rs
+++ b/src/parser/utils.rs
@@ -11,7 +11,7 @@ use super::error::ParseError;
 
 use crate::{
     destruct::Destruct,
-    eval::operation::PushPriority,
+    eval::operation::RecPriority,
     identifier::Ident,
     label::Label,
     mk_app, mk_fun,
@@ -155,16 +155,16 @@ pub fn combine_match_annots(
 }
 
 /// Some constructs are introduced with the metadata pipe operator `|`, but aren't metadata per se
-/// (ex: `push force`/`push default`). Those are collected in this extended annotation and then
+/// (ex: `rec force`/`rec default`). Those are collected in this extended annotation and then
 /// desugared into a standard metavalue.
 #[derive(Clone, Debug, Default)]
 pub struct FieldAnnot {
     /// Standard metadata.
     pub meta: MetaValue,
     /// Presence of an annotation `push force`
-    pub push_force: bool,
+    pub rec_force: bool,
     /// Presence of an annotation `push default`
-    pub push_default: bool,
+    pub rec_default: bool,
 }
 
 impl FieldAnnot {
@@ -175,14 +175,14 @@ impl FieldAnnot {
 
 impl Annot for FieldAnnot {
     fn attach(mut self, value: RichTerm, pos: TermPos) -> RichTerm {
-        if self.push_force || self.push_default {
-            let push_prio = if self.push_force {
-                PushPriority::Top
+        if self.rec_force || self.rec_default {
+            let rec_prio = if self.rec_force {
+                RecPriority::Top
             } else {
-                PushPriority::Bottom
+                RecPriority::Bottom
             };
 
-            self.meta.value = Some(push_prio.apply_push_op(value).with_pos(pos));
+            self.meta.value = Some(rec_prio.apply_rec_prio_op(value).with_pos(pos));
         } else {
             self.meta.value = Some(value);
         }
@@ -192,13 +192,13 @@ impl Annot for FieldAnnot {
 
     fn combine(outer: Self, inner: Self) -> Self {
         let meta = MetaValue::flatten(outer.meta, inner.meta);
-        let push_force = outer.push_force || inner.push_force;
-        let push_default = outer.push_default || inner.push_default;
+        let rec_force = outer.rec_force || inner.rec_force;
+        let rec_default = outer.rec_default || inner.rec_default;
 
         FieldAnnot {
             meta,
-            push_force,
-            push_default,
+            rec_force,
+            rec_default,
         }
     }
 }
diff --git a/src/stdlib.rs b/src/stdlib.rs
index 472b4579..545c8495 100644
--- a/src/stdlib.rs
+++ b/src/stdlib.rs
@@ -133,6 +133,6 @@ pub mod contract {
 pub mod internals {
     use super::*;
 
-    generate_accessor!(push_default);
-    generate_accessor!(push_force);
+    generate_accessor!(rec_default);
+    generate_accessor!(rec_force);
 }
diff --git a/src/term.rs b/src/term.rs
index e5081193..6b16d596 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -1347,11 +1347,41 @@ pub enum UnaryOp {
     /// It's also worth noting that [`UnaryOp::DeepSeq`] should be, in principle, more efficient that [`UnaryOp::Force`]
     /// as it does less cloning.
     Force(Option<crate::eval::callstack::StackElem>),
-
-    /// doc: TODO
-    PushDefault(),
-    /// doc: TODO
-    PushForce(),
+    /// Recursive default priority operator. Recursively propagates a default priority through a
+    /// record, stopping whenever a field isn't a record anymore to then turn into a simple
+    /// `default`.
+    ///
+    /// For example:
+    ///
+    /// ```nickel
+    /// {
+    ///   foo | rec default = {
+    ///     bar = 1,
+    ///     baz = "a",
+    ///   }
+    /// }
+    /// ```
+    ///
+    /// Is evaluated to:
+    ///
+    /// ```nickel
+    /// {
+    ///   foo = {
+    ///     bar | default = 1,
+    ///     baz | default = "a",
+    ///   }
+    /// }
+    /// ```
+    ///
+    /// If a value has any explicit priority annotation, then the original annotation takes
+    /// precedence and the default doesn't apply.
+    RecDefault(),
+    /// Recursive force priority operator. Similar to [UnaryOp::RecDefault], but propagate the
+    /// `force` annotation.
+    ///
+    /// As opposed to `RecDefault`, the `force` takes precedence and erase any prior explicit
+    /// priority annotation.
+    RecForce(),
 
     /// Creates an "empty" record with the sealed tail of its [`Term::Record`]
     /// argument.
@@ -1399,7 +1429,7 @@ pub enum OpPos {
 impl UnaryOp {
     pub fn eval_mode(&self) -> EvalMode {
         match self {
-            UnaryOp::PushDefault() | UnaryOp::PushForce() => EvalMode::StopAtMeta,
+            UnaryOp::RecDefault() | UnaryOp::RecForce() => EvalMode::StopAtMeta,
             _ => EvalMode::default(),
         }
     }
diff --git a/src/typecheck/operation.rs b/src/typecheck/operation.rs
index a478ac09..e33d8895 100644
--- a/src/typecheck/operation.rs
+++ b/src/typecheck/operation.rs
@@ -192,12 +192,12 @@ pub fn get_uop_type(
         // Dyn -> Dyn
         UnaryOp::Force(_) => (mk_uniftype::dynamic(), mk_uniftype::dynamic()),
         // forall a. a -> a
-        UnaryOp::PushDefault() => {
+        UnaryOp::RecDefault() => {
             let ty = state.table.fresh_type_uvar();
             (ty.clone(), ty)
         }
         // forall a. a -> a
-        UnaryOp::PushForce() => {
+        UnaryOp::RecForce() => {
             let ty = state.table.fresh_type_uvar();
             (ty.clone(), ty)
         }
diff --git a/stdlib/internals.ncl b/stdlib/internals.ncl
index 61566a71..eca27921 100644
--- a/stdlib/internals.ncl
+++ b/stdlib/internals.ncl
@@ -111,8 +111,8 @@
       if t == {} then acc
       else %blame% (%tag% "extra field `%{%head% (%fields% t)}`" l),
 
-  # Push priorities operators
+  # Recursive priorities operators
 
-  "$push_force" = fun val => %seq% val (%push_force% val),
-  "$push_default" = fun val => %seq% val (%push_default% val),
+  "$rec_force" = fun val => %seq% val (%rec_force% val),
+  "$rec_default" = fun val => %seq% val (%rec_default% val),
 }


Test patch:
diff --git a/tests/integration/pass/multiple-overrides.ncl b/tests/integration/pass/multiple-overrides.ncl
index 7504abbb..afc6fba9 100644
--- a/tests/integration/pass/multiple-overrides.ncl
+++ b/tests/integration/pass/multiple-overrides.ncl
@@ -89,7 +89,7 @@ let {check, ..} = import "testlib.ncl" in
                | default = "",
       snd_data = "snd",
     },
-    final_override | _push_force = {
+    final_override | rec force = {
       fst_data = "override",
       snd_data = "override",
       common.final = fst_data ++ "_" ++ snd_data,
diff --git a/tests/integration/pass/priorities.ncl b/tests/integration/pass/priorities.ncl
index 3bccad55..b0580aa7 100644
--- a/tests/integration/pass/priorities.ncl
+++ b/tests/integration/pass/priorities.ncl
@@ -41,16 +41,16 @@ let {Assert, check, ..} =  import "testlib.ncl" in
     d = 1,
   } | Assert,
 
-  {foo | _push_default = 1} & {foo = 2} == {foo = 2} | Assert,
-  {foo | _push_force = 1} & {foo = 2} == {foo = 1} | Assert,
-  {val | _push_default = {foo = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
-  {val | _push_force = {foo = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,
+  {foo | rec default = 1} & {foo = 2} == {foo = 2} | Assert,
+  {foo | rec force = 1} & {foo = 2} == {foo = 1} | Assert,
+  {val | rec default = {foo = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
+  {val | rec force = {foo = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,
 
   # Pushed priorities should only modifies fields without explicitly set priorities
-  {val | _push_force = {foo | priority -1 = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
-  {val | _push_force = {foo | default = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
-  {val | _push_default = {foo | priority 1 = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,
-  {val | _push_default = {foo | force = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,
+  {val | rec force = {foo | priority -1 = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
+  {val | rec force = {foo | default = 1}} & {val.foo = 2} == {val.foo = 2} | Assert,
+  {val | rec default = {foo | priority 1 = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,
+  {val | rec default = {foo | force = 1}} & {val.foo = 2} == {val.foo = 1} | Assert,
 
   let x = {
     foo | force = "old",
@@ -59,16 +59,16 @@ let {Assert, check, ..} =  import "testlib.ncl" in
         baz' = "old"
       } |> record.map (fun _name value => value)
   } in
-  {val | _push_default = x} & {val = {foo = "new", bar.baz = "new"}}
+  {val | rec default = x} & {val = {foo = "new", bar.baz = "new"}}
     == { val = {foo = "old", bar = {baz = "new", baz' = "old"}}}
     | Assert,
 
-  # Interaction of recursive overriding and push priorities
+  # Interaction of recursive overriding and recursive priorities
   let context = 1 in
   let x = {
     foo = context + 2 + bar,
     bar | default = 1,
   } in
-  {val | _push_force = x } & {val = {foo = 0, bar = 10}}
+  {val | rec force = x } & {val = {foo = 0, bar = 10}}
     == {val = {bar = 10, foo = 1 + 2 + 10}} | Assert,
 ] |> check