parser-state: fix attribute merging

src/libexpr/parser-state.hh

@@ -88,6 +88,7 @@ struct ParserState
     void dupAttr(const AttrPath & attrPath, const PosIdx pos, const PosIdx prevPos);
     void dupAttr(Symbol attr, const PosIdx pos, const PosIdx prevPos);
     void addAttr(ExprAttrs * attrs, AttrPath && attrPath, const ParserLocation & loc, Expr * e, const ParserLocation & exprLoc);
+    void addAttr(ExprAttrs * attrs, AttrPath & attrPath, const Symbol & symbol, ExprAttrs::AttrDef && def);
     Formals * validateFormals(Formals * formals, PosIdx pos = noPos, Symbol arg = {});
     Expr * stripIndentation(const PosIdx pos,
         std::vector<std::pair<PosIdx, std::variant<Expr *, StringToken>>> && es);
@@ -120,64 +121,29 @@ inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, const
     // Checking attrPath validity.
     // ===========================
     for (i = attrPath.begin(); i + 1 < attrPath.end(); i++) {
+        ExprAttrs * nested;
         if (i->symbol) {
             ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol);
             if (j != attrs->attrs.end()) {
-                if (j->second.kind != ExprAttrs::AttrDef::Kind::Inherited) {
+                nested = dynamic_cast<ExprAttrs *>(j->second.e);
-                    ExprAttrs * attrs2 = dynamic_cast<ExprAttrs *>(j->second.e);
+                if (!nested) {
-                    if (!attrs2) dupAttr(attrPath, pos, j->second.pos);
+                    attrPath.erase(i + 1, attrPath.end());
-                    attrs = attrs2;
-                } else
                     dupAttr(attrPath, pos, j->second.pos);
+                }
             } else {
-                ExprAttrs * nested = new ExprAttrs;
+                nested = new ExprAttrs;
                 attrs->attrs[i->symbol] = ExprAttrs::AttrDef(nested, pos);
-                attrs = nested;
             }
         } else {
-            ExprAttrs *nested = new ExprAttrs;
+            nested = new ExprAttrs;
             attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(i->expr, nested, pos));
-            attrs = nested;
         }
+        attrs = nested;
     }
     // Expr insertion.
     // ==========================
     if (i->symbol) {
-        ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol);
+        addAttr(attrs, attrPath, i->symbol, ExprAttrs::AttrDef(e, pos));
-        if (j != attrs->attrs.end()) {
-            // This attr path is already defined. However, if both
-            // e and the expr pointed by the attr path are two attribute sets,
-            // we want to merge them.
-            // Otherwise, throw an error.
-            auto ae = dynamic_cast<ExprAttrs *>(e);
-            auto jAttrs = dynamic_cast<ExprAttrs *>(j->second.e);
-            if (jAttrs && ae) {
-                if (ae->inheritFromExprs && !jAttrs->inheritFromExprs)
-                    jAttrs->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
-                for (auto & ad : ae->attrs) {
-                    auto j2 = jAttrs->attrs.find(ad.first);
-                    if (j2 != jAttrs->attrs.end()) // Attr already defined in iAttrs, error.
-                        dupAttr(ad.first, j2->second.pos, ad.second.pos);
-                    jAttrs->attrs.emplace(ad.first, ad.second);
-                    if (ad.second.kind == ExprAttrs::AttrDef::Kind::InheritedFrom) {
-                        auto & sel = dynamic_cast<ExprSelect &>(*ad.second.e);
-                        auto & from = dynamic_cast<ExprInheritFrom &>(*sel.e);
-                        from.displ += jAttrs->inheritFromExprs->size();
-                    }
-                }
-                jAttrs->dynamicAttrs.insert(jAttrs->dynamicAttrs.end(), ae->dynamicAttrs.begin(), ae->dynamicAttrs.end());
-                if (ae->inheritFromExprs) {
-                    jAttrs->inheritFromExprs->insert(jAttrs->inheritFromExprs->end(),
-                        ae->inheritFromExprs->begin(), ae->inheritFromExprs->end());
-                }
-            } else {
-                dupAttr(attrPath, pos, j->second.pos);
-            }
-        } else {
-            // This attr path is not defined. Let's create it.
-            attrs->attrs.emplace(i->symbol, ExprAttrs::AttrDef(e, pos));
-            e->setName(i->symbol);
-        }
     } else {
         attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(i->expr, e, pos));
     }
@@ -189,6 +155,60 @@ inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, const
     }
 }
 
+/**
+ * Precondition: attrPath is used for error messages and should already contain
+ * symbol as its last element.
+ */
+inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath & attrPath, const Symbol & symbol, ExprAttrs::AttrDef && def)
+{
+    ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(symbol);
+    if (j != attrs->attrs.end()) {
+        // This attr path is already defined. However, if both
+        // e and the expr pointed by the attr path are two attribute sets,
+        // we want to merge them.
+        // Otherwise, throw an error.
+        auto ae = dynamic_cast<ExprAttrs *>(def.e);
+        auto jAttrs = dynamic_cast<ExprAttrs *>(j->second.e);
+
+        // N.B. In a world in which we are less bound by our past mistakes, we
+        // would also test that jAttrs and ae are not recursive. The effect of
+        // not doing so is that any `rec` marker on ae is discarded, and any
+        // `rec` marker on jAttrs will apply to the attributes in ae.
+        // See https://github.com/NixOS/nix/issues/9020.
+        if (jAttrs && ae) {
+            if (ae->inheritFromExprs && !jAttrs->inheritFromExprs)
+                jAttrs->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
+            for (auto & ad : ae->attrs) {
+                if (ad.second.kind == ExprAttrs::AttrDef::Kind::InheritedFrom) {
+                    auto & sel = dynamic_cast<ExprSelect &>(*ad.second.e);
+                    auto & from = dynamic_cast<ExprInheritFrom &>(*sel.e);
+                    from.displ += jAttrs->inheritFromExprs->size();
+                }
+                attrPath.emplace_back(AttrName(ad.first));
+                addAttr(jAttrs, attrPath, ad.first, std::move(ad.second));
+                attrPath.pop_back();
+            }
+            ae->attrs.clear();
+            jAttrs->dynamicAttrs.insert(jAttrs->dynamicAttrs.end(),
+                std::make_move_iterator(ae->dynamicAttrs.begin()),
+                std::make_move_iterator(ae->dynamicAttrs.end()));
+            ae->dynamicAttrs.clear();
+            if (ae->inheritFromExprs) {
+                jAttrs->inheritFromExprs->insert(jAttrs->inheritFromExprs->end(),
+                    std::make_move_iterator(ae->inheritFromExprs->begin()),
+                    std::make_move_iterator(ae->inheritFromExprs->end()));
+                ae->inheritFromExprs = nullptr;
+            }
+        } else {
+            dupAttr(attrPath, def.pos, j->second.pos);
+        }
+    } else {
+        // This attr path is not defined. Let's create it.
+        attrs->attrs.emplace(symbol, def);
+        def.e->setName(symbol);
+    }
+}
+
 inline Formals * ParserState::validateFormals(Formals * formals, PosIdx pos, Symbol arg)
 {
     std::sort(formals->formals.begin(), formals->formals.end(),

tests/functional/lang/parse-fail-mixed-nested-attrs1.err.exp

@@ -1,6 +1,6 @@
-error: attribute 'z' already defined at «stdin»:3:16
+error: attribute 'x.z' already defined at «stdin»:2:3
-       at «stdin»:2:3:
+       at «stdin»:3:16:
-            1| {
             2|   x.z = 3;
-             |   ^
             3|   x = { y = 3; z = 3; };
+             |                ^
+            4| }

tests/functional/lang/parse-fail-mixed-nested-attrs2.err.exp

@@ -1,6 +1,6 @@
-error: attribute 'y' already defined at «stdin»:3:9
+error: attribute 'x.y.y' already defined at «stdin»:2:3
-       at «stdin»:2:3:
+       at «stdin»:3:9:
-            1| {
             2|   x.y.y = 3;
-             |   ^
             3|   x = { y.y= 3; z = 3; };
+             |         ^
+            4| }

tests/unit/libexpr/trivial.cc

@@ -177,6 +177,48 @@ namespace nix {
             )
     );
 
+#define X_EXPAND_IF0(k, v) k "." v
+#define X_EXPAND_IF1(k, v) k " = { " v " };"
+#define X4(w, x, y, z) \
+    TEST_F(TrivialExpressionTest, nestedAttrsetMerge##w##x##y##z) { \
+        auto v = eval("{ a.b = { c = 1; d = 2; }; } == { " \
+                X_EXPAND_IF##w("a", X_EXPAND_IF##x("b","c = 1;")) " " \
+                X_EXPAND_IF##y("a", X_EXPAND_IF##z("b","d = 2;")) " }"); \
+        ASSERT_THAT(v, IsTrue()); \
+    }; \
+    TEST_F(TrivialExpressionTest, nestedAttrsetMergeDup##w##x##y##z) { \
+        ASSERT_THROW(eval("{ " \
+                X_EXPAND_IF##w("a", X_EXPAND_IF##x("b","c = 1;")) " " \
+                X_EXPAND_IF##y("a", X_EXPAND_IF##z("b","c = 2;")) " }"), Error); \
+    }; \
+    TEST_F(TrivialExpressionTest, nestedAttrsetMergeLet##w##x##y##z) { \
+        auto v = eval("{ b = { c = 1; d = 2; }; } == (let " \
+                X_EXPAND_IF##w("a", X_EXPAND_IF##x("b","c = 1;")) " " \
+                X_EXPAND_IF##y("a", X_EXPAND_IF##z("b","d = 2;")) " in a)"); \
+        ASSERT_THAT(v, IsTrue()); \
+    };
+#define X3(...) X4(__VA_ARGS__, 0) X4(__VA_ARGS__, 1)
+#define X2(...) X3(__VA_ARGS__, 0) X3(__VA_ARGS__, 1)
+#define X1(...) X2(__VA_ARGS__, 0) X2(__VA_ARGS__, 1)
+    X1(0) X1(1)
+#undef X_EXPAND_IF0
+#undef X_EXPAND_IF1
+#undef X1
+#undef X2
+#undef X3
+#undef X4
+
+    // This is for backwards compatibility, not because we like it.
+    // See https://github.com/NixOS/nix/issues/9020.
+    TEST_F(TrivialExpressionTest, regrettableRecAttrsetMerge) {
+        auto v = eval("{ a = rec { b = c + 1; d = 2; }; a.c = d + 3; }.a.b");
+        ASSERT_THAT(v, IsIntEq(6));
+    }
+
+    TEST_F(TrivialExpressionTest, attrsetMergeDropsLaterRec) {
+        ASSERT_THROW(eval("{ a.b = 1; a = rec { c = d + 2; d = 3; }; }.c"), Error);
+    }
+
     TEST_F(TrivialExpressionTest, functor) {
         auto v = eval("{ __functor = self: arg: self.v + arg; v = 10; } 5");
         ASSERT_THAT(v, IsIntEq(15));