libexpr: add expr memory management

with the prepatory work done this mostly means turning plain pointers
into unique_ptrs, with all the associated churn that necessitates. we
might want to change some of these to box_ptrs at some point as well,
but that would be a semantic change that isn't fully appropriate yet.

Change-Id: I0c238c118617420650432f4ed45569baa3e3f413

7 files changed, 168 insertions, 147 deletions

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index c8329a602..25d98b23b 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -1247,7 +1247,7 @@ Env * ExprAttrs::buildInheritFromEnv(EvalState & state, Env & up)
     inheritEnv.up = &up;
 
     Displacement displ = 0;
-    for (auto from : *inheritFromExprs)
+    for (auto & from : *inheritFromExprs)
         inheritEnv.values[displ++] = from->maybeThunk(state, up);
 
     return &inheritEnv;
diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc
index 03e1effc0..bc53ca053 100644
--- a/src/libexpr/nixexpr.cc
+++ b/src/libexpr/nixexpr.cc
@@ -79,7 +79,7 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co
         return sa < sb;
     });
     std::vector<Symbol> inherits;
-    std::map<ExprInheritFrom *, std::vector<Symbol>> inheritsFrom;
+    std::map<Displacement, std::vector<Symbol>> inheritsFrom;
     for (auto & i : sorted) {
         switch (i->second.kind) {
         case AttrDef::Kind::Plain:
@@ -90,7 +90,7 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co
         case AttrDef::Kind::InheritedFrom: {
             auto & select = dynamic_cast<ExprSelect &>(*i->second.e);
             auto & from = dynamic_cast<ExprInheritFrom &>(*select.e);
-            inheritsFrom[&from].push_back(i->first);
+            inheritsFrom[from.displ].push_back(i->first);
             break;
         }
         }
@@ -102,7 +102,7 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co
     }
     for (const auto & [from, syms] : inheritsFrom) {
         str << "inherit (";
-        (*inheritFromExprs)[from->displ]->show(symbols, str);
+        (*inheritFromExprs)[from]->show(symbols, str);
         str << ")";
         for (auto sym : syms) str << " " << symbols[sym];
         str << "; ";
@@ -151,7 +151,7 @@ void ExprLambda::show(const SymbolTable & symbols, std::ostream & str) const
         // the natural Symbol ordering is by creation time, which can lead to the
         // same expression being printed in two different ways depending on its
         // context. always use lexicographic ordering to avoid this.
-        for (auto & i : formals->lexicographicOrder(symbols)) {
+        for (const Formal & i : formals->lexicographicOrder(symbols)) {
             if (first) first = false; else str << ", ";
             str << symbols[i.name];
             if (i.def) {
@@ -176,7 +176,7 @@ void ExprCall::show(const SymbolTable & symbols, std::ostream & str) const
 {
     str << '(';
     fun->show(symbols, str);
-    for (auto e : args) {
+    for (auto & e : args) {
         str <<  ' ';
         e->show(symbols, str);
     }
@@ -375,7 +375,7 @@ std::shared_ptr<const StaticEnv> ExprAttrs::bindInheritSources(
     // not even *have* an expr that grabs anything from this env since it's fully
     // invisible, but the evaluator does not allow for this yet.
     auto inner = std::make_shared<StaticEnv>(nullptr, env.get(), 0);
-    for (auto from : *inheritFromExprs)
+    for (auto & from : *inheritFromExprs)
         from->bindVars(es, env);
 
     return inner;
@@ -462,7 +462,7 @@ void ExprCall::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> &
         es.exprEnvs.insert(std::make_pair(this, env));
 
     fun->bindVars(es, env);
-    for (auto e : args)
+    for (auto & e : args)
         e->bindVars(es, env);
 }
 
diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh
index 91516c092..45d44d1d1 100644
--- a/src/libexpr/nixexpr.hh
+++ b/src/libexpr/nixexpr.hh
@@ -28,9 +28,9 @@ struct StaticEnv;
 struct AttrName
 {
     Symbol symbol;
-    Expr * expr;
+    std::unique_ptr<Expr> expr;
     AttrName(Symbol s) : symbol(s) {};
-    AttrName(Expr * e) : expr(e) {};
+    AttrName(std::unique_ptr<Expr> e) : expr(std::move(e)) {};
 };
 
 typedef std::vector<AttrName> AttrPath;
@@ -157,19 +157,19 @@ struct ExprInheritFrom : ExprVar
 struct ExprSelect : Expr
 {
     PosIdx pos;
-    Expr * e, * def;
+    std::unique_ptr<Expr> e, def;
     AttrPath attrPath;
-    ExprSelect(const PosIdx & pos, Expr * e, AttrPath attrPath, Expr * def) : pos(pos), e(e), def(def), attrPath(std::move(attrPath)) { };
-    ExprSelect(const PosIdx & pos, Expr * e, Symbol name) : pos(pos), e(e), def(0) { attrPath.push_back(AttrName(name)); };
+    ExprSelect(const PosIdx & pos, std::unique_ptr<Expr> e, AttrPath attrPath, std::unique_ptr<Expr> def) : pos(pos), e(std::move(e)), def(std::move(def)), attrPath(std::move(attrPath)) { };
+    ExprSelect(const PosIdx & pos, std::unique_ptr<Expr> e, Symbol name) : pos(pos), e(std::move(e)) { attrPath.push_back(AttrName(name)); };
     PosIdx getPos() const override { return pos; }
     COMMON_METHODS
 };
 
 struct ExprOpHasAttr : Expr
 {
-    Expr * e;
+    std::unique_ptr<Expr> e;
     AttrPath attrPath;
-    ExprOpHasAttr(Expr * e, AttrPath attrPath) : e(e), attrPath(std::move(attrPath)) { };
+    ExprOpHasAttr(std::unique_ptr<Expr> e, AttrPath attrPath) : e(std::move(e)), attrPath(std::move(attrPath)) { };
     PosIdx getPos() const override { return e->getPos(); }
     COMMON_METHODS
 };
@@ -189,11 +189,11 @@ struct ExprAttrs : Expr
         };
 
         Kind kind;
-        Expr * e;
+        std::unique_ptr<Expr> e;
         PosIdx pos;
         Displacement displ; // displacement
-        AttrDef(Expr * e, const PosIdx & pos, Kind kind = Kind::Plain)
-            : kind(kind), e(e), pos(pos) { };
+        AttrDef(std::unique_ptr<Expr> e, const PosIdx & pos, Kind kind = Kind::Plain)
+            : kind(kind), e(std::move(e)), pos(pos) { };
         AttrDef() { };
 
         template<typename T>
@@ -212,12 +212,12 @@ struct ExprAttrs : Expr
     };
     typedef std::map<Symbol, AttrDef> AttrDefs;
     AttrDefs attrs;
-    std::unique_ptr<std::vector<Expr *>> inheritFromExprs;
+    std::unique_ptr<std::vector<std::unique_ptr<Expr>>> inheritFromExprs;
     struct DynamicAttrDef {
-        Expr * nameExpr, * valueExpr;
+        std::unique_ptr<Expr> nameExpr, valueExpr;
         PosIdx pos;
-        DynamicAttrDef(Expr * nameExpr, Expr * valueExpr, const PosIdx & pos)
-            : nameExpr(nameExpr), valueExpr(valueExpr), pos(pos) { };
+        DynamicAttrDef(std::unique_ptr<Expr> nameExpr, std::unique_ptr<Expr> valueExpr, const PosIdx & pos)
+            : nameExpr(std::move(nameExpr)), valueExpr(std::move(valueExpr)), pos(pos) { };
     };
     typedef std::vector<DynamicAttrDef> DynamicAttrDefs;
     DynamicAttrDefs dynamicAttrs;
@@ -234,7 +234,7 @@ struct ExprAttrs : Expr
 
 struct ExprList : Expr
 {
-    std::vector<Expr *> elems;
+    std::vector<std::unique_ptr<Expr>> elems;
     ExprList() { };
     COMMON_METHODS
     Value * maybeThunk(EvalState & state, Env & env) override;
@@ -249,7 +249,7 @@ struct Formal
 {
     PosIdx pos;
     Symbol name;
-    Expr * def;
+    std::unique_ptr<Expr> def;
 };
 
 /** Attribute set destructuring in arguments of a lambda, if present */
@@ -266,9 +266,9 @@ struct Formals
         return it != formals.end() && it->name == arg;
     }
 
-    std::vector<Formal> lexicographicOrder(const SymbolTable & symbols) const
+    std::vector<std::reference_wrapper<const Formal>> lexicographicOrder(const SymbolTable & symbols) const
     {
-        std::vector<Formal> result(formals.begin(), formals.end());
+        std::vector<std::reference_wrapper<const Formal>> result(formals.begin(), formals.end());
         std::sort(result.begin(), result.end(),
             [&] (const Formal & a, const Formal & b) {
                 std::string_view sa = symbols[a.name], sb = symbols[b.name];
@@ -292,14 +292,14 @@ struct ExprLambda : Expr
     Symbol arg;
     /** Formals are present when the lambda destructures an attr set as
      * argument, with or without ellipsis */
-    Formals * formals;
-    Expr * body;
-    ExprLambda(PosIdx pos, Symbol arg, Formals * formals, Expr * body)
-        : pos(pos), arg(arg), formals(formals), body(body)
+    std::unique_ptr<Formals> formals;
+    std::unique_ptr<Expr> body;
+    ExprLambda(PosIdx pos, Symbol arg, std::unique_ptr<Formals> formals, std::unique_ptr<Expr> body)
+        : pos(pos), arg(arg), formals(std::move(formals)), body(std::move(body))
     {
     };
-    ExprLambda(PosIdx pos, Formals * formals, Expr * body)
-        : pos(pos), formals(formals), body(body)
+    ExprLambda(PosIdx pos, std::unique_ptr<Formals> formals, std::unique_ptr<Expr> body)
+        : pos(pos), formals(std::move(formals)), body(std::move(body))
     {
     }
     void setName(Symbol name) override;
@@ -336,11 +336,11 @@ struct ExprLambda : Expr
 
 struct ExprCall : Expr
 {
-    Expr * fun;
-    std::vector<Expr *> args;
+    std::unique_ptr<Expr> fun;
+    std::vector<std::unique_ptr<Expr>> args;
     PosIdx pos;
-    ExprCall(const PosIdx & pos, Expr * fun, std::vector<Expr *> && args)
-        : fun(fun), args(std::move(args)), pos(pos)
+    ExprCall(const PosIdx & pos, std::unique_ptr<Expr> fun, std::vector<std::unique_ptr<Expr>> && args)
+        : fun(std::move(fun)), args(std::move(args)), pos(pos)
     { }
     PosIdx getPos() const override { return pos; }
     COMMON_METHODS
@@ -348,19 +348,19 @@ struct ExprCall : Expr
 
 struct ExprLet : Expr
 {
-    ExprAttrs * attrs;
-    Expr * body;
-    ExprLet(ExprAttrs * attrs, Expr * body) : attrs(attrs), body(body) { };
+    std::unique_ptr<ExprAttrs> attrs;
+    std::unique_ptr<Expr> body;
+    ExprLet(std::unique_ptr<ExprAttrs> attrs, std::unique_ptr<Expr> body) : attrs(std::move(attrs)), body(std::move(body)) { };
     COMMON_METHODS
 };
 
 struct ExprWith : Expr
 {
     PosIdx pos;
-    Expr * attrs, * body;
+    std::unique_ptr<Expr> attrs, body;
     size_t prevWith;
     ExprWith * parentWith;
-    ExprWith(const PosIdx & pos, Expr * attrs, Expr * body) : pos(pos), attrs(attrs), body(body) { };
+    ExprWith(const PosIdx & pos, std::unique_ptr<Expr> attrs, std::unique_ptr<Expr> body) : pos(pos), attrs(std::move(attrs)), body(std::move(body)) { };
     PosIdx getPos() const override { return pos; }
     COMMON_METHODS
 };
@@ -368,8 +368,8 @@ struct ExprWith : Expr
 struct ExprIf : Expr
 {
     PosIdx pos;
-    Expr * cond, * then, * else_;
-    ExprIf(const PosIdx & pos, Expr * cond, Expr * then, Expr * else_) : pos(pos), cond(cond), then(then), else_(else_) { };
+    std::unique_ptr<Expr> cond, then, else_;
+    ExprIf(const PosIdx & pos, std::unique_ptr<Expr> cond, std::unique_ptr<Expr> then, std::unique_ptr<Expr> else_) : pos(pos), cond(std::move(cond)), then(std::move(then)), else_(std::move(else_)) { };
     PosIdx getPos() const override { return pos; }
     COMMON_METHODS
 };
@@ -377,16 +377,16 @@ struct ExprIf : Expr
 struct ExprAssert : Expr
 {
     PosIdx pos;
-    Expr * cond, * body;
-    ExprAssert(const PosIdx & pos, Expr * cond, Expr * body) : pos(pos), cond(cond), body(body) { };
+    std::unique_ptr<Expr> cond, body;
+    ExprAssert(const PosIdx & pos, std::unique_ptr<Expr> cond, std::unique_ptr<Expr> body) : pos(pos), cond(std::move(cond)), body(std::move(body)) { };
     PosIdx getPos() const override { return pos; }
     COMMON_METHODS
 };
 
 struct ExprOpNot : Expr
 {
-    Expr * e;
-    ExprOpNot(Expr * e) : e(e) { };
+    std::unique_ptr<Expr> e;
+    ExprOpNot(std::unique_ptr<Expr> e) : e(std::move(e)) { };
     PosIdx getPos() const override { return e->getPos(); }
     COMMON_METHODS
 };
@@ -395,9 +395,9 @@ struct ExprOpNot : Expr
     struct name : Expr \
     { \
         PosIdx pos; \
-        Expr * e1, * e2; \
-        name(Expr * e1, Expr * e2) : e1(e1), e2(e2) { }; \
-        name(const PosIdx & pos, Expr * e1, Expr * e2) : pos(pos), e1(e1), e2(e2) { }; \
+        std::unique_ptr<Expr> e1, e2; \
+        name(std::unique_ptr<Expr> e1, std::unique_ptr<Expr> e2) : e1(std::move(e1)), e2(std::move(e2)) { }; \
+        name(const PosIdx & pos, std::unique_ptr<Expr> e1, std::unique_ptr<Expr> e2) : pos(pos), e1(std::move(e1)), e2(std::move(e2)) { }; \
         void show(const SymbolTable & symbols, std::ostream & str) const override \
         { \
             str << "("; e1->show(symbols, str); str << " " s " "; e2->show(symbols, str); str << ")"; \
@@ -422,8 +422,8 @@ struct ExprConcatStrings : Expr
 {
     PosIdx pos;
     bool forceString;
-    std::vector<std::pair<PosIdx, Expr *>> es;
-    ExprConcatStrings(const PosIdx & pos, bool forceString, std::vector<std::pair<PosIdx, Expr *>> es)
+    std::vector<std::pair<PosIdx, std::unique_ptr<Expr>>> es;
+    ExprConcatStrings(const PosIdx & pos, bool forceString, std::vector<std::pair<PosIdx, std::unique_ptr<Expr>>> es)
         : pos(pos), forceString(forceString), es(std::move(es)) { };
     PosIdx getPos() const override { return pos; }
     COMMON_METHODS
diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh
index 468b34bd3..cb1f12230 100644
--- a/src/libexpr/parser-state.hh
+++ b/src/libexpr/parser-state.hh
@@ -49,10 +49,10 @@ struct ParserState
 
     [[nodiscard]] ParseError dupAttr(const AttrPath & attrPath, const PosIdx pos, const PosIdx prevPos);
     [[nodiscard]] ParseError dupAttr(Symbol attr, const PosIdx pos, const PosIdx prevPos);
-    [[nodiscard]] std::optional<ParseError> addAttr(ExprAttrs * attrs, AttrPath && attrPath, Expr * e, const PosIdx pos);
+    [[nodiscard]] std::optional<ParseError> addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_ptr<Expr> e, const PosIdx pos);
     [[nodiscard]] std::optional<ParseError> validateFormals(Formals * formals, PosIdx pos = noPos, Symbol arg = {});
-    Expr * stripIndentation(const PosIdx pos,
-        std::vector<std::pair<PosIdx, std::variant<Expr *, StringToken>>> && es);
+    std::unique_ptr<Expr> stripIndentation(const PosIdx pos,
+        std::vector<std::pair<PosIdx, std::variant<std::unique_ptr<Expr>, StringToken>>> && es);
     PosIdx at(const ParserLocation & loc);
 };
 
@@ -73,7 +73,7 @@ inline ParseError ParserState::dupAttr(Symbol attr, const PosIdx pos, const PosI
     });
 }
 
-inline std::optional<ParseError> ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, Expr * e, const PosIdx pos)
+inline std::optional<ParseError> ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_ptr<Expr> e, const PosIdx pos)
 {
     AttrPath::iterator i;
     // All attrpaths have at least one attr
@@ -85,20 +85,25 @@ inline std::optional<ParseError> ParserState::addAttr(ExprAttrs * attrs, AttrPat
             ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol);
             if (j != attrs->attrs.end()) {
                 if (j->second.kind != ExprAttrs::AttrDef::Kind::Inherited) {
-                    ExprAttrs * attrs2 = dynamic_cast<ExprAttrs *>(j->second.e);
-                    if (!attrs2) return dupAttr({attrPath.begin(), i + 1}, pos, j->second.pos);
+                    ExprAttrs * attrs2 = dynamic_cast<ExprAttrs *>(j->second.e.get());
+                    if (!attrs2) {
+                        attrPath.erase(i + 1, attrPath.end());
+                        return dupAttr(attrPath, pos, j->second.pos);
+                    }
                     attrs = attrs2;
-                } else
-                    return dupAttr({attrPath.begin(), i + 1}, pos, j->second.pos);
+                } else {
+                    attrPath.erase(i + 1, attrPath.end());
+                    return dupAttr(attrPath, pos, j->second.pos);
+                }
             } else {
-                ExprAttrs * nested = new ExprAttrs;
-                attrs->attrs[i->symbol] = ExprAttrs::AttrDef(nested, pos);
-                attrs = nested;
+                auto next = attrs->attrs.emplace(std::piecewise_construct,
+                    std::tuple(i->symbol),
+                    std::tuple(std::make_unique<ExprAttrs>(), pos));
+                attrs = static_cast<ExprAttrs *>(next.first->second.e.get());
             }
         } else {
-            ExprAttrs *nested = new ExprAttrs;
-            attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(i->expr, nested, pos));
-            attrs = nested;
+            auto & next = attrs->dynamicAttrs.emplace_back(std::move(i->expr), std::make_unique<ExprAttrs>(), pos);
+            attrs = static_cast<ExprAttrs *>(next.valueExpr.get());
         }
     }
     // Expr insertion.
@@ -110,37 +115,37 @@ inline std::optional<ParseError> ParserState::addAttr(ExprAttrs * attrs, AttrPat
             // 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);
+            auto * ae = dynamic_cast<ExprAttrs *>(e.get());
+            auto * jAttrs = dynamic_cast<ExprAttrs *>(j->second.e.get());
             if (jAttrs && ae) {
                 if (ae->inheritFromExprs && !jAttrs->inheritFromExprs)
-                    jAttrs->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
+                    jAttrs->inheritFromExprs = std::make_unique<std::vector<std::unique_ptr<Expr>>>();
                 for (auto & ad : ae->attrs) {
                     auto j2 = jAttrs->attrs.find(ad.first);
                     if (j2 != jAttrs->attrs.end()) // Attr already defined in iAttrs, error.
                         return 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->attrs.emplace(ad.first, std::move(ad.second));
                 }
-                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());
-                }
+                std::ranges::move(ae->dynamicAttrs, std::back_inserter(jAttrs->dynamicAttrs));
+                if (ae->inheritFromExprs)
+                    std::ranges::move(*ae->inheritFromExprs, std::back_inserter(*jAttrs->inheritFromExprs));
             } else {
                 return 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);
+            attrs->attrs.emplace(std::piecewise_construct,
+                std::tuple(i->symbol),
+                std::tuple(std::move(e), pos));
         }
     } else {
-        attrs->dynamicAttrs.push_back(ExprAttrs::DynamicAttrDef(i->expr, e, pos));
+        attrs->dynamicAttrs.emplace_back(std::move(i->expr), std::move(e), pos);
     }
 
     return {};
@@ -175,10 +180,10 @@ inline std::optional<ParseError> ParserState::validateFormals(Formals * formals,
     return {};
 }
 
-inline Expr * ParserState::stripIndentation(const PosIdx pos,
-    std::vector<std::pair<PosIdx, std::variant<Expr *, StringToken>>> && es)
+inline std::unique_ptr<Expr> ParserState::stripIndentation(const PosIdx pos,
+    std::vector<std::pair<PosIdx, std::variant<std::unique_ptr<Expr>, StringToken>>> && es)
 {
-    if (es.empty()) return new ExprString("");
+    if (es.empty()) return std::make_unique<ExprString>("");
 
     /* Figure out the minimum indentation.  Note that by design
        whitespace-only final lines are not taken into account.  (So
@@ -216,17 +221,17 @@ inline Expr * ParserState::stripIndentation(const PosIdx pos,
     }
 
     /* Strip spaces from each line. */
-    std::vector<std::pair<PosIdx, Expr *>> es2;
+    std::vector<std::pair<PosIdx, std::unique_ptr<Expr>>> es2;
     atStartOfLine = true;
     size_t curDropped = 0;
     size_t n = es.size();
     auto i = es.begin();
-    const auto trimExpr = [&] (Expr * e) {
+    const auto trimExpr = [&] (std::unique_ptr<Expr> e) {
         atStartOfLine = false;
         curDropped = 0;
-        es2.emplace_back(i->first, e);
+        es2.emplace_back(i->first, std::move(e));
     };
-    const auto trimString = [&] (const StringToken & t) {
+    const auto trimString = [&] (const StringToken t) {
         std::string s2;
         for (size_t j = 0; j < t.l; ++j) {
             if (atStartOfLine) {
@@ -256,17 +261,17 @@ inline Expr * ParserState::stripIndentation(const PosIdx pos,
                 s2 = std::string(s2, 0, p + 1);
         }
 
-        es2.emplace_back(i->first, new ExprString(std::move(s2)));
+        es2.emplace_back(i->first, std::make_unique<ExprString>(std::move(s2)));
     };
     for (; i != es.end(); ++i, --n) {
-        std::visit(overloaded { trimExpr, trimString }, i->second);
+        std::visit(overloaded { trimExpr, trimString }, std::move(i->second));
     }
 
     /* If this is a single string, then don't do a concatenation. */
-    if (es2.size() == 1 && dynamic_cast<ExprString *>(es2[0].second)) {
-        return es2[0].second;
+    if (es2.size() == 1 && dynamic_cast<ExprString *>(es2[0].second.get())) {
+        return std::move(es2[0].second);
     }
-    return new ExprConcatStrings(pos, true, std::move(es2));
+    return std::make_unique<ExprConcatStrings>(pos, true, std::move(es2));
 }
 
 inline PosIdx ParserState::at(const ParserLocation & loc)
diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y
index 5b53720ce..b825f2ed8 100644
--- a/src/libexpr/parser.y
+++ b/src/libexpr/parser.y
@@ -81,6 +81,21 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char *
     });
 }
 
+template<typename T>
+static std::unique_ptr<T> unp(T * e)
+{
+  return std::unique_ptr<T>(e);
+}
+
+template<typename T = std::unique_ptr<nix::Expr>, typename... Args>
+static std::vector<T> vec(Args && ... args)
+{
+  std::vector<T> result;
+  result.reserve(sizeof...(Args));
+  (result.emplace_back(std::forward<Args>(args)), ...);
+  return result;
+}
+
 
 %}
 
@@ -99,8 +114,8 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParserState * state, const char *
   nix::StringToken str;
   std::vector<nix::AttrName> * attrNames;
   std::vector<std::pair<nix::AttrName, nix::PosIdx>> * inheritAttrs;
-  std::vector<std::pair<nix::PosIdx, nix::Expr *>> * string_parts;
-  std::vector<std::pair<nix::PosIdx, std::variant<nix::Expr *, nix::StringToken>>> * ind_string_parts;
+  std::vector<std::pair<nix::PosIdx, std::unique_ptr<nix::Expr>>> * string_parts;
+  std::vector<std::pair<nix::PosIdx, std::variant<std::unique_ptr<nix::Expr>, nix::StringToken>>> * ind_string_parts;
 }
 
 %destructor { delete $$; } <e>
@@ -158,85 +173,86 @@ expr: expr_function;
 
 expr_function
   : ID ':' expr_function
-    { $$ = new ExprLambda(CUR_POS, state->symbols.create($1), 0, $3); }
+    { $$ = new ExprLambda(CUR_POS, state->symbols.create($1), nullptr, unp($3)); }
   | '{' formals '}' ':' expr_function
     { if (auto e = state->validateFormals($2)) THROW(*e);
-      $$ = new ExprLambda(CUR_POS, $2, $5); }
+      $$ = new ExprLambda(CUR_POS, unp($2), unp($5));
+    }
   | '{' formals '}' '@' ID ':' expr_function
     {
       auto arg = state->symbols.create($5);
       if (auto e = state->validateFormals($2, CUR_POS, arg)) THROW(*e, $2, $7);
-      $$ = new ExprLambda(CUR_POS, arg, $2, $7);
+      $$ = new ExprLambda(CUR_POS, arg, unp($2), unp($7));
     }
   | ID '@' '{' formals '}' ':' expr_function
     {
       auto arg = state->symbols.create($1);
       if (auto e = state->validateFormals($4, CUR_POS, arg)) THROW(*e, $4, $7);
-      $$ = new ExprLambda(CUR_POS, arg, $4, $7);
+      $$ = new ExprLambda(CUR_POS, arg, unp($4), unp($7));
     }
   | ASSERT expr ';' expr_function
-    { $$ = new ExprAssert(CUR_POS, $2, $4); }
+    { $$ = new ExprAssert(CUR_POS, unp($2), unp($4)); }
   | WITH expr ';' expr_function
-    { $$ = new ExprWith(CUR_POS, $2, $4); }
+    { $$ = new ExprWith(CUR_POS, unp($2), unp($4)); }
   | LET binds IN expr_function
     { if (!$2->dynamicAttrs.empty())
         THROW(ParseError({
             .msg = HintFmt("dynamic attributes not allowed in let"),
             .pos = state->positions[CUR_POS]
         }), $2, $4);
-      $$ = new ExprLet($2, $4);
+      $$ = new ExprLet(unp($2), unp($4));
     }
   | expr_if
   ;
 
 expr_if
-  : IF expr THEN expr ELSE expr { $$ = new ExprIf(CUR_POS, $2, $4, $6); }
+  : IF expr THEN expr ELSE expr { $$ = new ExprIf(CUR_POS, unp($2), unp($4), unp($6)); }
   | expr_op
   ;
 
 expr_op
-  : '!' expr_op %prec NOT { $$ = new ExprOpNot($2); }
-  | '-' expr_op %prec NEGATE { $$ = new ExprCall(CUR_POS, new ExprVar(state->s.sub), {new ExprInt(0), $2}); }
-  | expr_op EQ expr_op { $$ = new ExprOpEq($1, $3); }
-  | expr_op NEQ expr_op { $$ = new ExprOpNEq($1, $3); }
-  | expr_op '<' expr_op { $$ = new ExprCall(state->at(@2), new ExprVar(state->s.lessThan), {$1, $3}); }
-  | expr_op LEQ expr_op { $$ = new ExprOpNot(new ExprCall(state->at(@2), new ExprVar(state->s.lessThan), {$3, $1})); }
-  | expr_op '>' expr_op { $$ = new ExprCall(state->at(@2), new ExprVar(state->s.lessThan), {$3, $1}); }
-  | expr_op GEQ expr_op { $$ = new ExprOpNot(new ExprCall(state->at(@2), new ExprVar(state->s.lessThan), {$1, $3})); }
-  | expr_op AND expr_op { $$ = new ExprOpAnd(state->at(@2), $1, $3); }
-  | expr_op OR expr_op { $$ = new ExprOpOr(state->at(@2), $1, $3); }
-  | expr_op IMPL expr_op { $$ = new ExprOpImpl(state->at(@2), $1, $3); }
-  | expr_op UPDATE expr_op { $$ = new ExprOpUpdate(state->at(@2), $1, $3); }
-  | expr_op '?' attrpath { $$ = new ExprOpHasAttr($1, std::move(*$3)); delete $3; }
+  : '!' expr_op %prec NOT { $$ = new ExprOpNot(unp($2)); }
+  | '-' expr_op %prec NEGATE { $$ = new ExprCall(CUR_POS, std::make_unique<ExprVar>(state->s.sub), vec(std::make_unique<ExprInt>(0), unp($2))); }
+  | expr_op EQ expr_op { $$ = new ExprOpEq(unp($1), unp($3)); }
+  | expr_op NEQ expr_op { $$ = new ExprOpNEq(unp($1), unp($3)); }
+  | expr_op '<' expr_op { $$ = new ExprCall(state->at(@2), std::make_unique<ExprVar>(state->s.lessThan), vec($1, $3)); }
+  | expr_op LEQ expr_op { $$ = new ExprOpNot(std::make_unique<ExprCall>(state->at(@2), std::make_unique<ExprVar>(state->s.lessThan), vec($3, $1))); }
+  | expr_op '>' expr_op { $$ = new ExprCall(state->at(@2), std::make_unique<ExprVar>(state->s.lessThan), vec($3, $1)); }
+  | expr_op GEQ expr_op { $$ = new ExprOpNot(std::make_unique<ExprCall>(state->at(@2), std::make_unique<ExprVar>(state->s.lessThan), vec($1, $3))); }
+  | expr_op AND expr_op { $$ = new ExprOpAnd(state->at(@2), unp($1), unp($3)); }
+  | expr_op OR expr_op { $$ = new ExprOpOr(state->at(@2), unp($1), unp($3)); }
+  | expr_op IMPL expr_op { $$ = new ExprOpImpl(state->at(@2), unp($1), unp($3)); }
+  | expr_op UPDATE expr_op { $$ = new ExprOpUpdate(state->at(@2), unp($1), unp($3)); }
+  | expr_op '?' attrpath { $$ = new ExprOpHasAttr(unp($1), std::move(*$3)); delete $3; }
   | expr_op '+' expr_op
-    { $$ = new ExprConcatStrings(state->at(@2), false, {{state->at(@1), $1}, {state->at(@3), $3}}); }
-  | expr_op '-' expr_op { $$ = new ExprCall(state->at(@2), new ExprVar(state->s.sub), {$1, $3}); }
-  | expr_op '*' expr_op { $$ = new ExprCall(state->at(@2), new ExprVar(state->s.mul), {$1, $3}); }
-  | expr_op '/' expr_op { $$ = new ExprCall(state->at(@2), new ExprVar(state->s.div), {$1, $3}); }
-  | expr_op CONCAT expr_op { $$ = new ExprOpConcatLists(state->at(@2), $1, $3); }
+    { $$ = new ExprConcatStrings(state->at(@2), false, vec<std::pair<PosIdx, std::unique_ptr<Expr>>>(std::pair(state->at(@1), unp($1)), std::pair(state->at(@3), unp($3)))); }
+  | expr_op '-' expr_op { $$ = new ExprCall(state->at(@2), std::make_unique<ExprVar>(state->s.sub), vec($1, $3)); }
+  | expr_op '*' expr_op { $$ = new ExprCall(state->at(@2), std::make_unique<ExprVar>(state->s.mul), vec($1, $3)); }
+  | expr_op '/' expr_op { $$ = new ExprCall(state->at(@2), std::make_unique<ExprVar>(state->s.div), vec($1, $3)); }
+  | expr_op CONCAT expr_op { $$ = new ExprOpConcatLists(state->at(@2), unp($1), unp($3)); }
   | expr_app
   ;
 
 expr_app
   : expr_app expr_select {
       if (auto e2 = dynamic_cast<ExprCall *>($1)) {
-          e2->args.push_back($2);
+          e2->args.emplace_back($2);
           $$ = $1;
       } else
-          $$ = new ExprCall(CUR_POS, $1, {$2});
+          $$ = new ExprCall(CUR_POS, unp($1), vec(unp($2)));
   }
   | expr_select
   ;
 
 expr_select
   : expr_simple '.' attrpath
-    { $$ = new ExprSelect(CUR_POS, $1, std::move(*$3), nullptr); delete $3; }
+    { $$ = new ExprSelect(CUR_POS, unp($1), std::move(*$3), nullptr); delete $3; }
   | expr_simple '.' attrpath OR_KW expr_select
-    { $$ = new ExprSelect(CUR_POS, $1, std::move(*$3), $5); delete $3; }
+    { $$ = new ExprSelect(CUR_POS, unp($1), std::move(*$3), unp($5)); delete $3; }
   | /* Backwards compatibility: because Nixpkgs has a rarely used
        function named ‘or’, allow stuff like ‘map or [...]’. */
     expr_simple OR_KW
-    { $$ = new ExprCall(CUR_POS, $1, {new ExprVar(CUR_POS, state->s.or_)}); }
+    { $$ = new ExprCall(CUR_POS, unp($1), vec(std::make_unique<ExprVar>(CUR_POS, state->s.or_))); }
   | expr_simple
   ;
 
@@ -252,21 +268,21 @@ expr_simple
   | FLOAT { $$ = new ExprFloat($1); }
   | '"' string_parts '"' { $$ = $2; }
   | IND_STRING_OPEN ind_string_parts IND_STRING_CLOSE {
-      $$ = state->stripIndentation(CUR_POS, std::move(*$2));
+      $$ = state->stripIndentation(CUR_POS, std::move(*$2)).release();
       delete $2;
   }
   | path_start PATH_END
   | path_start string_parts_interpolated PATH_END {
-      $2->insert($2->begin(), {state->at(@1), $1});
+      $2->emplace($2->begin(), state->at(@1), $1);
       $$ = new ExprConcatStrings(CUR_POS, false, std::move(*$2));
       delete $2;
   }
   | SPATH {
       std::string path($1.p + 1, $1.l - 2);
       $$ = new ExprCall(CUR_POS,
-          new ExprVar(state->s.findFile),
-          {new ExprVar(state->s.nixPath),
-           new ExprString(std::move(path))});
+          std::make_unique<ExprVar>(state->s.findFile),
+          vec(std::make_unique<ExprVar>(state->s.nixPath),
+              std::make_unique<ExprString>(std::move(path))));
   }
   | URI {
       static bool noURLLiterals = experimentalFeatureSettings.isEnabled(Xp::NoUrlLiterals);
@@ -281,7 +297,7 @@ expr_simple
   /* Let expressions `let {..., body = ...}' are just desugared
      into `(rec {..., body = ...}).body'. */
   | LET '{' binds '}'
-    { $3->recursive = true; $$ = new ExprSelect(noPos, $3, state->s.body); }
+    { $3->recursive = true; $$ = new ExprSelect(noPos, unp($3), state->s.body); }
   | REC '{' binds '}'
     { $3->recursive = true; $$ = $3; }
   | '{' binds '}'
@@ -302,9 +318,9 @@ string_parts_interpolated
   : string_parts_interpolated STR
   { $$ = $1; $1->emplace_back(state->at(@2), new ExprString(std::string($2))); }
   | string_parts_interpolated DOLLAR_CURLY expr '}' { $$ = $1; $1->emplace_back(state->at(@2), $3); }
-  | DOLLAR_CURLY expr '}' { $$ = new std::vector<std::pair<PosIdx, Expr *>>; $$->emplace_back(state->at(@1), $2); }
+  | DOLLAR_CURLY expr '}' { $$ = new std::vector<std::pair<PosIdx, std::unique_ptr<Expr>>>; $$->emplace_back(state->at(@1), $2); }
   | STR DOLLAR_CURLY expr '}' {
-      $$ = new std::vector<std::pair<PosIdx, Expr *>>;
+      $$ = new std::vector<std::pair<PosIdx, std::unique_ptr<Expr>>>;
       $$->emplace_back(state->at(@1), new ExprString(std::string($1)));
       $$->emplace_back(state->at(@2), $3);
     }
@@ -332,14 +348,14 @@ path_start
 
 ind_string_parts
   : ind_string_parts IND_STR { $$ = $1; $1->emplace_back(state->at(@2), $2); }
-  | ind_string_parts DOLLAR_CURLY expr '}' { $$ = $1; $1->emplace_back(state->at(@2), $3); }
-  | { $$ = new std::vector<std::pair<PosIdx, std::variant<Expr *, StringToken>>>; }
+  | ind_string_parts DOLLAR_CURLY expr '}' { $$ = $1; $1->emplace_back(state->at(@2), unp($3)); }
+  | { $$ = new std::vector<std::pair<PosIdx, std::variant<std::unique_ptr<Expr>, StringToken>>>; }
   ;
 
 binds
   : binds attrpath '=' expr ';'
     { $$ = $1;
-      if (auto e = state->addAttr($$, std::move(*$2), $4, state->at(@2))) THROW(*e, $1, $2);
+      if (auto e = state->addAttr($$, std::move(*$2), unp($4), state->at(@2))) THROW(*e, $1, $2);
       delete $2;
     }
   | binds INHERIT attrs ';'
@@ -349,23 +365,23 @@ binds
               THROW(state->dupAttr(i.symbol, iPos, $$->attrs[i.symbol].pos), $1);
           $$->attrs.emplace(
               i.symbol,
-              ExprAttrs::AttrDef(new ExprVar(iPos, i.symbol), iPos, ExprAttrs::AttrDef::Kind::Inherited));
+              ExprAttrs::AttrDef(std::make_unique<ExprVar>(iPos, i.symbol), iPos, ExprAttrs::AttrDef::Kind::Inherited));
       }
       delete $3;
     }
   | binds INHERIT '(' expr ')' attrs ';'
     { $$ = $1;
       if (!$$->inheritFromExprs)
-          $$->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
-      $$->inheritFromExprs->push_back($4);
-      auto from = new nix::ExprInheritFrom(state->at(@4), $$->inheritFromExprs->size() - 1);
+          $$->inheritFromExprs = std::make_unique<std::vector<std::unique_ptr<Expr>>>();
+      $$->inheritFromExprs->push_back(unp($4));
       for (auto & [i, iPos] : *$6) {
           if ($$->attrs.find(i.symbol) != $$->attrs.end())
               THROW(state->dupAttr(i.symbol, iPos, $$->attrs[i.symbol].pos), $1);
+          auto from = std::make_unique<nix::ExprInheritFrom>(state->at(@4), $$->inheritFromExprs->size() - 1);
           $$->attrs.emplace(
               i.symbol,
               ExprAttrs::AttrDef(
-                  new ExprSelect(iPos, from, i.symbol),
+                  std::make_unique<ExprSelect>(iPos, std::move(from), i.symbol),
                   iPos,
                   ExprAttrs::AttrDef::Kind::InheritedFrom));
       }
@@ -400,7 +416,7 @@ attrpath
           $$->push_back(AttrName(state->symbols.create(str->s)));
           delete str;
       } else
-          $$->push_back(AttrName($3));
+          $$->emplace_back(unp($3));
     }
   | attr { $$ = new std::vector<AttrName>; $$->push_back(AttrName(state->symbols.create($1))); }
   | string_attr
@@ -410,7 +426,7 @@ attrpath
           $$->push_back(AttrName(state->symbols.create(str->s)));
           delete str;
       } else
-          $$->push_back(AttrName($1));
+          $$->emplace_back(unp($1));
     }
   ;
 
@@ -425,15 +441,15 @@ string_attr
   ;
 
 expr_list
-  : expr_list expr_select { $$ = $1; $1->elems.push_back($2); /* !!! dangerous */ }
+  : expr_list expr_select { $$ = $1; $1->elems.emplace_back($2); /* !!! dangerous */ }
   | { $$ = new ExprList; }
   ;
 
 formals
   : formal ',' formals
-    { $$ = $3; $$->formals.emplace_back(*$1); delete $1; }
+    { $$ = $3; $$->formals.emplace_back(std::move(*$1)); delete $1; }
   | formal
-    { $$ = new Formals; $$->formals.emplace_back(*$1); $$->ellipsis = false; delete $1; }
+    { $$ = new Formals; $$->formals.emplace_back(std::move(*$1)); $$->ellipsis = false; delete $1; }
   |
     { $$ = new Formals; $$->ellipsis = false; }
   | ELLIPSIS
@@ -441,8 +457,8 @@ formals
   ;
 
 formal
-  : ID { $$ = new Formal{CUR_POS, state->symbols.create($1), 0}; }
-  | ID '?' expr { $$ = new Formal{CUR_POS, state->symbols.create($1), $3}; }
+  : ID { $$ = new Formal{CUR_POS, state->symbols.create($1), nullptr}; }
+  | ID '?' expr { $$ = new Formal{CUR_POS, state->symbols.create($1), unp($3)}; }
   ;
 
 %%
diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index 720ab1a3f..4b7e61dfe 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -2817,7 +2817,7 @@ static void prim_functionArgs(EvalState & state, const PosIdx pos, Value * * arg
     auto attrs = state.buildBindings(args[0]->lambda.fun->formals->formals.size());
     for (auto & i : args[0]->lambda.fun->formals->formals)
         // !!! should optimise booleans (allocate only once)
-        attrs.alloc(i.name, i.pos).mkBool(i.def);
+        attrs.alloc(i.name, i.pos).mkBool(i.def != nullptr);
     v.mkAttrs(attrs);
 }
 
diff --git a/src/libexpr/value-to-xml.cc b/src/libexpr/value-to-xml.cc
index fda360eff..d77fdf96e 100644
--- a/src/libexpr/value-to-xml.cc
+++ b/src/libexpr/value-to-xml.cc
@@ -140,7 +140,7 @@ static void printValueAsXML(EvalState & state, bool strict, bool location,
                 if (v.lambda.fun->arg) attrs["name"] = state.symbols[v.lambda.fun->arg];
                 if (v.lambda.fun->formals->ellipsis) attrs["ellipsis"] = "1";
                 XMLOpenElement _(doc, "attrspat", attrs);
-                for (auto & i : v.lambda.fun->formals->lexicographicOrder(state.symbols))
+                for (const Formal & i : v.lambda.fun->formals->lexicographicOrder(state.symbols))
                     doc.writeEmptyElement("attr", singletonAttrs("name", state.symbols[i.name]));
             } else
                 doc.writeEmptyElement("varpat", singletonAttrs("name", state.symbols[v.lambda.fun->arg]));