From 0e259a161fc282b3f1fc6ed7b9880da1e2713230 Mon Sep 17 00:00:00 2001
From: Guillaume Girol <symphorien+git@xlumurb.eu>
Date: Tue, 26 Jul 2022 12:00:00 +0000
Subject: [PATCH] fix attrset merging for >= 3 instances

---
 src/eval.rs  | 59 +++++++++++++++++++++++++++++++++++++++++-----------
 src/tests.rs | 20 ++++++++++++++++++
 2 files changed, 67 insertions(+), 12 deletions(-)

diff --git a/src/eval.rs b/src/eval.rs
index 0871c22..f11b816 100644
--- a/src/eval.rs
+++ b/src/eval.rs
@@ -383,6 +383,34 @@ impl Expr {
 
 /// Used for merging sets during parsing. For example:
 /// { a.b = 1; a.c = 2; } => { a = { b = 1; c = 2; }; }
+///
+/// Nix only allows merging several such KeyValuePairs when they correspond
+/// to bare literals. Inserting a mere indirection through let or a function
+/// prevents this from happening and throws an error instead:
+/// ```text
+/// nix-repl> :p { a = { b = 1; }; a.c = 2; }
+/// { a = { b = 1; c = 2; }; }
+///
+/// nix-repl> :p { a = (x: x){ b = 1; }; a.c = 2; }
+/// error: attribute 'a.c' already defined at (string):1:3
+///
+///        at «string»:1:25:
+///
+///             1| { a = (x: x){ b = 1; }; a.c = 2; }
+///              |                         ^
+///
+/// nix-repl> :p let y = { b = 1; }; in { a = y; a.c = 2; }
+/// error: attribute 'a.c' already defined at (string):1:26
+///
+///        at «string»:1:33:
+///
+///             1| let y = { b = 1; }; in { a = y; a.c = 2; }
+///              |                                 ^
+///
+/// ```
+/// This function takes a and b, attempts to interpret them as literal
+/// key value pairs or literal attrset values, and if it failed returns
+/// such an error.
 pub fn merge_set_literal(name: String, a: Gc<Expr>, b: Gc<Expr>) -> Result<Gc<Expr>, EvalError> {
     // evaluate literal attr sets only, otherwise error
     let eval_literal = |src: Gc<Expr>| {
@@ -391,18 +419,25 @@ pub fn merge_set_literal(name: String, a: Gc<Expr>, b: Gc<Expr>) -> Result<Gc<Ex
         } else {
             src
         };
-        if let ExprSource::AttrSet { .. } = &src.source {
-            src.eval()?.as_map()
-        } else {
-            // We cannot merge a literal with a non-literal. This error is
-            // caused by incorrect expressions such as:
-            // ```
-            // repl> let x = { y = 1; }; in { a = x; a.z = 2; }
-            // error: attribute 'a.z' at (string):1:33 already defined at (string):1:26
-            // ```
-            // The above would be caught because `x` is an ExprSource::Ident (as
-            // opposed to being an ExprSource::AttrSet literal).
-            Err(EvalError::Value(ValueError::AttrAlreadyDefined(name.to_string())))
+        match &src.source {
+            ExprSource::AttrSet { .. } => src.eval()?.as_map(),
+            ExprSource::Literal {
+                value: NixValue::Map(m),
+                ..
+            } => Ok(m.clone()),
+            _ => {
+                // We cannot merge a literal with a non-literal. This error is
+                // caused by incorrect expressions such as:
+                // ```
+                // repl> let x = { y = 1; }; in { a = x; a.z = 2; }
+                // error: attribute 'a.z' at (string):1:33 already defined at (string):1:26
+                // ```
+                // The above would be caught because `x` is an ExprSource::Ident (as
+                // opposed to being an ExprSource::AttrSet literal).
+                Err(EvalError::Value(ValueError::AttrAlreadyDefined(
+                    name.to_string(),
+                )))
+            }
         }
     };
 
diff --git a/src/tests.rs b/src/tests.rs
index 84b887c..aeb0558 100644
--- a/src/tests.rs
+++ b/src/tests.rs
@@ -14,6 +14,8 @@ use serde_json::json;
 use std::time::Duration;
 #[cfg(test)]
 use stoppable_thread::*;
+#[cfg(test)]
+use crate::error::ValueError;
 
 #[allow(dead_code)]
 /// Evaluates a nix code snippet
@@ -294,13 +296,31 @@ fn attrs_merge() {
     assert_eq!(eval(&format!("{}.a.c", code)).as_int().unwrap(), 2);
 }
 
+#[test]
+fn attrs_merge_3() {
+    let code = "{ a.b = 1; a.c = 2; a.d = 3; }".to_string();
+    assert_eq!(eval(&format!("{}.a.b", code)).as_int().unwrap(), 1);
+    assert_eq!(eval(&format!("{}.a.c", code)).as_int().unwrap(), 2);
+    assert_eq!(eval(&format!("{}.a.d", code)).as_int().unwrap(), 3);
+}
+
 #[test]
 fn attrs_merge_conflict() {
     let ast = rnix::parse("{ a = { b = 1; c = 3; }; a.c = 2; }");
     let root = ast.root().inner().unwrap();
     let path = std::env::current_dir().unwrap();
     let parse_result = Expr::parse(root, Gc::new(Scope::Root(path)));
+    assert!(matches!(parse_result, Err(EvalError::Value(ValueError::AttrAlreadyDefined(_)))));
+}
+
+#[test]
+fn attrs_merge_conflict_2() {
+    let ast = rnix::parse("{ a = let y = { b = 1; }; in y; a.c = 2; }");
+    let root = ast.root().inner().unwrap();
+    let path = std::env::current_dir().unwrap();
+    let parse_result = Expr::parse(root, Gc::new(Scope::Root(path)));
     assert!(parse_result.is_err());
+    // assert!(matches!(parse_result, Err(EvalError::Value(ValueError::AttrAlreadyDefined(_)))));
 }
 
 #[test]