Merge changes I4771fe87,Ifc7a9516,I1f5e05ab,I63f2fbcd into main

* changes:
  libexpr: Rework error messages on ExprSelct::eval
  libexpr: Track position information in attrpaths
  libexpr: Assert: Don't print assertion in error message
  libexpr: Split ExprAttrs and ExprLet

doc/manual/rl-next/error-message-improvements.md

@@ -0,0 +1,21 @@
+---
+synopsis: Small error message improvements
+issues: []
+cls: [2185, 2187]
+category: Improvements
+credits: [piegames]
+---
+
+When an attribute selection fails, the error message now correctly points to the attribute in the chain that failed instead of at the beginning of the entire chain.
+```diff
+ error: attribute 'x' missing
+-       at /pwd/lang/eval-fail-remove.nix:4:3:
++       at /pwd/lang/eval-fail-remove.nix:4:29:
+             3| in
+             4|   (removeAttrs attrs ["x"]).x
+-             |   ^
++             |                             ^
+             5|
+```
+
+Failed asserts don't print the failed assertion expression anymore in the error message. That code was buggy and the information was redundant anyways, given that the error position already more accurately shows what exactly failed.

lix/libexpr/eval.cc

@@ -1071,12 +1071,13 @@ Env * ExprAttrs::buildInheritFromEnv(EvalState & state, Env & up)
     return &inheritEnv;
 }
 
-void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
+void ExprSet::eval(EvalState & state, Env & env, Value & v)
 {
     v.mkAttrs(state.ctx.buildBindings(attrs.size() + dynamicAttrs.size()).finish());
     auto dynamicEnv = &env;
 
     if (recursive) {
+
         /* Create a new environment that contains the attributes in
            this `rec'. */
         Env & env2(state.ctx.mem.allocEnv(attrs.size()));
@@ -1084,7 +1085,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
         dynamicEnv = &env2;
         Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, env2) : nullptr;
 
-        AttrDefs::iterator overrides = attrs.find(state.ctx.s.overrides);
+        ExprAttrs::AttrDefs::iterator overrides = attrs.find(state.ctx.s.overrides);
         bool hasOverrides = overrides != attrs.end();
 
         /* The recursive attributes are evaluated in the new
@@ -1093,7 +1094,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
         Displacement displ = 0;
         for (auto & i : attrs) {
             Value * vAttr;
-            if (hasOverrides && i.second.kind != AttrDef::Kind::Inherited) {
+            if (hasOverrides && i.second.kind != ExprAttrs::AttrDef::Kind::Inherited) {
                 vAttr = state.ctx.mem.allocValue();
                 vAttr->mkThunk(i.second.chooseByKind(&env2, &env, inheritEnv), *i.second.e);
                 state.ctx.stats.nrThunks++;
@@ -1118,7 +1119,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
             for (auto & i : *v.attrs)
                 newBnds->push_back(i);
             for (auto & i : *vOverrides->attrs) {
-                AttrDefs::iterator j = attrs.find(i.name);
+                ExprAttrs::AttrDefs::iterator j = attrs.find(i.name);
                 if (j != attrs.end()) {
                     (*newBnds)[j->second.displ] = i;
                     env2.values[j->second.displ] = i.value;
@@ -1167,16 +1168,16 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
 {
     /* Create a new environment that contains the attributes in this
        `let'. */
-    Env & env2(state.ctx.mem.allocEnv(attrs->attrs.size()));
+    Env & env2(state.ctx.mem.allocEnv(attrs.size()));
     env2.up = &env;
 
-    Env * inheritEnv = attrs->inheritFromExprs ? attrs->buildInheritFromEnv(state, env2) : nullptr;
+    Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, env2) : nullptr;
 
     /* The recursive attributes are evaluated in the new environment,
        while the inherited attributes are evaluated in the original
        environment. */
     Displacement displ = 0;
-    for (auto & i : attrs->attrs) {
+    for (auto & i : attrs) {
         env2.values[displ++] = i.second.e->maybeThunk(
             state,
             *i.second.chooseByKind(&env2, &env, inheritEnv));
@@ -1234,9 +1235,7 @@ static std::string showAttrPath(EvalState & state, Env & env, const AttrPath & a
             out << state.ctx.symbols[getName(i, state, env)];
         } catch (Error & e) {
             assert(!i.symbol);
-            out << "\"${";
-            i.expr->show(state.ctx.symbols, out);
-            out << "}\"";
+            out << "\"${...}\"";
         }
     }
     return out.str();
@@ -1251,6 +1250,8 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v)
     Value * vCurrent = &vFirst;
     // Position for the current attrset Value in this select chain.
     PosIdx posCurrent;
+    // Position for the current selector in this select chain.
+    PosIdx posCurrentSyntax;
 
     try {
         e->eval(state, env, vFirst);
@@ -1258,8 +1259,7 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v)
         assert(this->e != nullptr);
         e.addTrace(
             state.ctx.positions[getPos()],
-            "while evaluating '%s' to select '%s' on it",
-            ExprPrinter(state, *this->e),
+            "while evaluating an expression to select '%s' on it",
             showAttrPath(state.ctx.symbols, this->attrPath)
         );
         throw;
@@ -1281,36 +1281,12 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v)
 
             Symbol const name = getName(currentAttrName, state, env);
 
-            // For formatting errors, which should be done only when needed.
-            auto partsSoFar = [&]() -> std::string {
-                std::stringstream ss;
-                // We start with the base thing this ExprSelect is selecting on.
-                assert(this->e != nullptr);
-                this->e->show(state.ctx.symbols, ss);
-
-                // Then grab each part of the attr path up to this one.
-                assert(partIdx < attrPath.size());
-                std::span<AttrName> const parts(
-                    attrPath.begin(),
-                    attrPath.begin() + partIdx
-                );
-
-                // And convert them to strings and join them.
-                for (auto const & part : parts) {
-                    auto const partName = getName(part, state, env);
-                    ss << "." << state.ctx.symbols[partName];
-                }
-
-                return ss.str();
-            };
-
             try {
                 state.forceValue(*vCurrent, pos);
             } catch (Error & e) {
                 e.addTrace(
-                    state.ctx.positions[getPos()],
-                    "while evaluating '%s' to select '%s' on it",
-                    partsSoFar(),
+                    state.ctx.positions[currentAttrName.pos],
+                    "while evaluating an expression to select '%s' on it",
                     state.ctx.symbols[name]
                 );
                 throw;
@@ -1331,8 +1307,8 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v)
                     showType(*vCurrent),
                     ValuePrinter(state, *vCurrent, errorPrintOptions)
                 ).addTrace(
-                    pos,
-                    HintFmt("while selecting '%s' on '%s'", state.ctx.symbols[name], partsSoFar())
+                    currentAttrName.pos,
+                    HintFmt("while selecting '%s'", state.ctx.symbols[name])
                 ).debugThrow();
             }
 
@@ -1354,7 +1330,7 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v)
                 }
                 auto suggestions = Suggestions::bestMatches(allAttrNames, state.ctx.symbols[name]);
                 state.ctx.errors.make<EvalError>("attribute '%s' missing", state.ctx.symbols[name])
-                    .atPos(pos)
+                    .atPos(currentAttrName.pos)
                     .withSuggestions(suggestions)
                     .withFrame(env, *this)
                     .debugThrow();
@@ -1364,10 +1340,11 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v)
             // Set our currently operated-on attrset to this one, and keep going.
             vCurrent = attrIt->value;
             posCurrent = attrIt->pos;
+            posCurrentSyntax = currentAttrName.pos;
             if (state.ctx.stats.countCalls) state.ctx.stats.attrSelects[posCurrent]++;
         }
 
-        state.forceValue(*vCurrent, (posCurrent ? posCurrent : this->pos));
+        state.forceValue(*vCurrent, (posCurrent ? posCurrent : posCurrentSyntax));
 
     } catch (Error & e) {
         auto pos2r = state.ctx.positions[posCurrent];
@@ -1826,9 +1803,10 @@ void ExprIf::eval(EvalState & state, Env & env, Value & v)
 void ExprAssert::eval(EvalState & state, Env & env, Value & v)
 {
     if (!state.evalBool(env, *cond, pos, "in the condition of the assert statement")) {
-        std::ostringstream out;
-        cond->show(state.ctx.symbols, out);
-        state.ctx.errors.make<AssertionError>("assertion '%1%' failed", out.str()).atPos(pos).withFrame(env, *this).debugThrow();
+        state.ctx.errors.make<AssertionError>("assertion failed")
+            .atPos(pos)
+            .withFrame(env, *this)
+            .debugThrow();
     }
     body->eval(state, env, v);
 }

lix/libexpr/eval.hh

@@ -748,7 +748,7 @@ private:
     inline Value * lookupVar(Env * env, const ExprVar & var, bool noEval);
 
     friend struct ExprVar;
-    friend struct ExprAttrs;
+    friend struct ExprSet;
     friend struct ExprLet;
 
     /**

lix/libexpr/nixexpr.cc

@@ -21,11 +21,11 @@ std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol)
     return printIdentifier(str, s);
 }
 
-AttrName::AttrName(Symbol s) : symbol(s)
+AttrName::AttrName(PosIdx pos, Symbol s) : pos(pos), symbol(s)
 {
 }
 
-AttrName::AttrName(std::unique_ptr<Expr> e) : expr(std::move(e))
+AttrName::AttrName(PosIdx pos, std::unique_ptr<Expr> e) : pos(pos), expr(std::move(e))
 {
 }
 
@@ -139,7 +139,7 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co
     }
 }
 
-void ExprAttrs::show(const SymbolTable & symbols, std::ostream & str) const
+void ExprSet::show(const SymbolTable & symbols, std::ostream & str) const
 {
     if (recursive) str << "rec ";
     str << "{ ";
@@ -202,7 +202,7 @@ void ExprCall::show(const SymbolTable & symbols, std::ostream & str) const
 void ExprLet::show(const SymbolTable & symbols, std::ostream & str) const
 {
     str << "(let ";
-    attrs->showBindings(symbols, str);
+    showBindings(symbols, str);
     str << "in ";
     body->show(symbols, str);
     str << ")";
@@ -423,7 +423,7 @@ std::shared_ptr<const StaticEnv> ExprAttrs::bindInheritSources(
     return inner;
 }
 
-void ExprAttrs::bindVars(Evaluator & es, const std::shared_ptr<const StaticEnv> & env)
+void ExprSet::bindVars(Evaluator & es, const std::shared_ptr<const StaticEnv> & env)
 {
     if (es.debug)
         es.debug->exprEnvs.insert(std::make_pair(this, env));
@@ -511,18 +511,18 @@ void ExprCall::bindVars(Evaluator & es, const std::shared_ptr<const StaticEnv> &
 void ExprLet::bindVars(Evaluator & es, const std::shared_ptr<const StaticEnv> & env)
 {
     auto newEnv = [&] () -> std::shared_ptr<const StaticEnv> {
-        auto newEnv = std::make_shared<StaticEnv>(nullptr, env.get(), attrs->attrs.size());
+        auto newEnv = std::make_shared<StaticEnv>(nullptr, env.get(), attrs.size());
 
         Displacement displ = 0;
-        for (auto & i : attrs->attrs)
+        for (auto & i : attrs)
             newEnv->vars.emplace_back(i.first, i.second.displ = displ++);
         return newEnv;
     }();
 
-    // No need to sort newEnv since attrs->attrs is in sorted order.
+    // No need to sort newEnv since attrs is in sorted order.
 
-    auto inheritFromEnv = attrs->bindInheritSources(es, newEnv);
-    for (auto & i : attrs->attrs)
+    auto inheritFromEnv = bindInheritSources(es, newEnv);
+    for (auto & i : attrs)
         i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, inheritFromEnv));
 
     if (es.debug)

lix/libexpr/nixexpr.hh

@@ -28,10 +28,11 @@ struct StaticEnv;
  */
 struct AttrName
 {
+    PosIdx pos;
     Symbol symbol;
     std::unique_ptr<Expr> expr;
-    AttrName(Symbol s);
-    AttrName(std::unique_ptr<Expr> e);
+    AttrName(PosIdx pos, Symbol s);
+    AttrName(PosIdx pos, std::unique_ptr<Expr> e);
 };
 
 typedef std::vector<AttrName> AttrPath;
@@ -182,7 +183,7 @@ struct ExprSelect : Expr
     AttrPath attrPath;
 
     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)); };
+    ExprSelect(const PosIdx & pos, std::unique_ptr<Expr> e, const PosIdx namePos, Symbol name) : pos(pos), e(std::move(e)) { attrPath.push_back(AttrName(namePos, name)); };
     PosIdx getPos() const override { return pos; }
     COMMON_METHODS
 };
@@ -196,10 +197,16 @@ struct ExprOpHasAttr : Expr
     COMMON_METHODS
 };
 
-struct ExprAttrs : Expr
+/* Helper struct to contain the data shared across lets and sets */
+struct ExprAttrs
 {
-    bool recursive;
-    PosIdx pos;
+    ExprAttrs() = default;
+    ExprAttrs(const ExprAttrs &) = delete;
+    ExprAttrs & operator=(const ExprAttrs &) = delete;
+    ExprAttrs(ExprAttrs &&) = default;
+    ExprAttrs & operator=(ExprAttrs &&) = default;
+    virtual ~ExprAttrs() = default;
+
     struct AttrDef {
         enum class Kind {
             /** `attr = expr;` */
@@ -243,10 +250,6 @@ struct ExprAttrs : Expr
     };
     typedef std::vector<DynamicAttrDef> DynamicAttrDefs;
     DynamicAttrDefs dynamicAttrs;
-    ExprAttrs(const PosIdx &pos) : recursive(false), pos(pos) { };
-    ExprAttrs() : recursive(false) { };
-    PosIdx getPos() const override { return pos; }
-    COMMON_METHODS
 
     std::shared_ptr<const StaticEnv> bindInheritSources(
         Evaluator & es, const std::shared_ptr<const StaticEnv> & env);
@@ -254,6 +257,16 @@ struct ExprAttrs : Expr
     void showBindings(const SymbolTable & symbols, std::ostream & str) const;
 };
 
+struct ExprSet : Expr, ExprAttrs {
+    PosIdx pos;
+    bool recursive = false;
+
+    ExprSet(const PosIdx &pos, bool recursive = false) : pos(pos), recursive(recursive) { };
+    ExprSet() { };
+    PosIdx getPos() const override { return pos; }
+    COMMON_METHODS
+};
+
 struct ExprList : Expr
 {
     std::vector<std::unique_ptr<Expr>> elems;
@@ -368,11 +381,9 @@ struct ExprCall : Expr
     COMMON_METHODS
 };
 
-struct ExprLet : Expr
+struct ExprLet : Expr, ExprAttrs
 {
-    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
 };
 

lix/libexpr/parser/parser-impl1.inc.cc

@@ -260,10 +260,10 @@ template<> struct BuildAST<grammar::v1::formals> : change_head<FormalsState> {
 struct AttrState : SubexprState {
     using SubexprState::SubexprState;
 
-    std::vector<AttrName> attrs;
+    AttrPath attrs;
 
     template <typename T>
-    void pushAttr(T && attr, PosIdx) { attrs.emplace_back(std::forward<T>(attr)); }
+    void pushAttr(T && attr, PosIdx pos) { attrs.emplace_back(pos, std::forward<T>(attr)); }
 };
 
 template<> struct BuildAST<grammar::v1::attr::simple> {
@@ -285,22 +285,38 @@ template<> struct BuildAST<grammar::v1::attr::string> {
 template<> struct BuildAST<grammar::v1::attr::expr> : BuildAST<grammar::v1::attr::string> {};
 
 struct BindingsState : SubexprState {
-    using SubexprState::SubexprState;
+    explicit BindingsState(ExprState & up, ExprAttrs & attrs) : SubexprState(up), attrs(attrs) {}
 
-    ExprAttrs attrs;
+    ExprAttrs & attrs;
+    PosIdx pos;
     AttrPath path;
     std::unique_ptr<Expr> value;
 };
 
+struct BindingsStateSet : BindingsState {
+    ExprSet set = {};
+    BindingsStateSet(ExprState & up, State & ps, auto &...) : BindingsState(up, set) { }
+};
+
+struct BindingsStateRecSet : BindingsState {
+    ExprSet set = { PosIdx{}, true };
+    BindingsStateRecSet(ExprState & up, State & ps, auto &...) : BindingsState(up, set) { }
+};
+
+struct BindingsStateLet : BindingsState {
+    ExprLet let = {};
+    BindingsStateLet(ExprState & up, State & ps, auto &...) : BindingsState(up, let) { }
+};
+
 struct InheritState : SubexprState {
     using SubexprState::SubexprState;
 
-    std::vector<std::pair<AttrName, PosIdx>> attrs;
+    std::vector<AttrName> attrs;
     std::unique_ptr<Expr> from;
     PosIdx fromPos;
 
     template <typename T>
-    void pushAttr(T && attr, PosIdx pos) { attrs.emplace_back(std::forward<T>(attr), pos); }
+    void pushAttr(T && attr, PosIdx pos) { attrs.emplace_back(pos, std::forward<T>(attr)); }
 };
 
 template<> struct BuildAST<grammar::v1::inherit::from> {
@@ -314,15 +330,15 @@ template<> struct BuildAST<grammar::v1::inherit> : change_head<InheritState> {
     static void success0(InheritState & s, BindingsState & b, State & ps) {
         auto & attrs = b.attrs.attrs;
         // TODO this should not reuse generic attrpath rules.
-        for (auto & [i, iPos] : s.attrs) {
+        for (auto & i : s.attrs) {
             if (i.symbol)
                 continue;
             if (auto str = dynamic_cast<ExprString *>(i.expr.get()))
-                i = AttrName(ps.symbols.create(str->s));
+                i = AttrName(i.pos, ps.symbols.create(str->s));
             else {
                 throw ParseError({
                     .msg = HintFmt("dynamic attributes not allowed in inherit"),
-                    .pos = ps.positions[iPos]
+                    .pos = ps.positions[i.pos]
                 });
             }
         }
@@ -331,9 +347,9 @@ template<> struct BuildAST<grammar::v1::inherit> : change_head<InheritState> {
                 b.attrs.inheritFromExprs = std::make_unique<std::vector<ref<Expr>>>();
             auto fromExpr = ref<Expr>(std::move(s.from));
             b.attrs.inheritFromExprs->push_back(fromExpr);
-            for (auto & [i, iPos] : s.attrs) {
+            for (auto & i : s.attrs) {
                 if (attrs.find(i.symbol) != attrs.end())
-                    ps.dupAttr(i.symbol, iPos, attrs[i.symbol].pos);
+                    ps.dupAttr(i.symbol, i.pos, attrs[i.symbol].pos);
                 auto inheritFrom = std::make_unique<ExprInheritFrom>(
                     s.fromPos,
                     b.attrs.inheritFromExprs->size() - 1,
@@ -342,19 +358,19 @@ template<> struct BuildAST<grammar::v1::inherit> : change_head<InheritState> {
                 attrs.emplace(
                     i.symbol,
                     ExprAttrs::AttrDef(
-                        std::make_unique<ExprSelect>(iPos, std::move(inheritFrom), i.symbol),
-                        iPos,
+                        std::make_unique<ExprSelect>(i.pos, std::move(inheritFrom), i.pos, i.symbol),
+                        i.pos,
                         ExprAttrs::AttrDef::Kind::InheritedFrom));
             }
         } else {
-            for (auto & [i, iPos] : s.attrs) {
+            for (auto & i : s.attrs) {
                 if (attrs.find(i.symbol) != attrs.end())
-                    ps.dupAttr(i.symbol, iPos, attrs[i.symbol].pos);
+                    ps.dupAttr(i.symbol, i.pos, attrs[i.symbol].pos);
                 attrs.emplace(
                     i.symbol,
                     ExprAttrs::AttrDef(
-                        std::make_unique<ExprVar>(iPos, i.symbol),
-                        iPos,
+                        std::make_unique<ExprVar>(i.pos, i.symbol),
+                        i.pos,
                         ExprAttrs::AttrDef::Kind::Inherited));
             }
         }
@@ -663,8 +679,8 @@ template<> struct BuildAST<grammar::v1::expr::uri> {
     }
 };
 
-template<> struct BuildAST<grammar::v1::expr::ancient_let> : change_head<BindingsState> {
-    static void success(const auto & in, BindingsState & b, ExprState & s, State & ps) {
+template<> struct BuildAST<grammar::v1::expr::ancient_let> : change_head<BindingsStateRecSet> {
+    static void success(const auto & in, BindingsStateRecSet & b, ExprState & s, State & ps) {
         // Added 2024-09-18. Turn into an error at some point in the future.
         // See the documentation on deprecated features for more details.
         if (!ps.featureSettings.isEnabled(Dep::AncientLet))
@@ -675,30 +691,23 @@ template<> struct BuildAST<grammar::v1::expr::ancient_let> : change_head<Binding
                 "--extra-deprecated-features ancient-let"
             );
 
-        b.attrs.pos = ps.at(in);
-        b.attrs.recursive = true;
-        s.pushExpr<ExprSelect>(b.attrs.pos, b.attrs.pos, std::make_unique<ExprAttrs>(std::move(b.attrs)), ps.s.body);
+        auto pos = ps.at(in);
+        b.set.pos = pos;
+        s.pushExpr<ExprSelect>(pos, pos, std::make_unique<ExprSet>(std::move(b.set)), pos, ps.s.body);
     }
 };
 
-template<> struct BuildAST<grammar::v1::expr::rec_set> : change_head<BindingsState> {
-    static void success(const auto & in, BindingsState & b, ExprState & s, State & ps) {
-        // Before inserting new attrs, check for __override and throw an error
-        // (the error will initially be a warning to ease migration)
-        if (!featureSettings.isEnabled(Dep::RecSetOverrides) && b.attrs.attrs.contains(ps.s.overrides)) {
-            ps.overridesFound(ps.at(in));
-        }
-
-        b.attrs.pos = ps.at(in);
-        b.attrs.recursive = true;
-        s.pushExpr<ExprAttrs>(b.attrs.pos, std::move(b.attrs));
+template<> struct BuildAST<grammar::v1::expr::rec_set> : change_head<BindingsStateRecSet> {
+    static void success(const auto & in, BindingsStateRecSet & b, ExprState & s, State & ps) {
+        b.set.pos = ps.at(in);
+        s.pushExpr<ExprSet>(ps.at(in), std::move(b.set));
     }
 };
 
-template<> struct BuildAST<grammar::v1::expr::set> : change_head<BindingsState> {
-    static void success(const auto & in, BindingsState & b, ExprState & s, State & ps) {
-        b.attrs.pos = ps.at(in);
-        s.pushExpr<ExprAttrs>(b.attrs.pos, std::move(b.attrs));
+template<> struct BuildAST<grammar::v1::expr::set> : change_head<BindingsStateSet> {
+    static void success(const auto & in, BindingsStateSet & b, ExprState & s, State & ps) {
+        b.set.pos = ps.at(in);
+        s.pushExpr<ExprSet>(ps.at(in), std::move(b.set));
     }
 };
 
@@ -835,15 +844,15 @@ template<> struct BuildAST<grammar::v1::expr::with> {
     }
 };
 
-template<> struct BuildAST<grammar::v1::expr::let> : change_head<BindingsState> {
-    static void success(const auto & in, BindingsState & b, ExprState & s, State & ps) {
-        if (!b.attrs.dynamicAttrs.empty())
+template<> struct BuildAST<grammar::v1::expr::let> : change_head<BindingsStateLet> {
+    static void success(const auto & in, BindingsStateLet & b, ExprState & s, State & ps) {
+        if (!b.let.dynamicAttrs.empty())
             throw ParseError({
                 .msg = HintFmt("dynamic attributes not allowed in let"),
                 .pos = ps.positions[ps.at(in)]
             });
-
-        s.pushExpr<ExprLet>(ps.at(in), std::make_unique<ExprAttrs>(std::move(b.attrs)), b->popExprOnly());
+        b.let.body = b->popExprOnly();
+        s.pushExpr<ExprLet>(ps.at(in), std::move(b.let));
     }
 };
 

lix/libexpr/parser/state.hh

@@ -102,7 +102,7 @@ inline void State::addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_
             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.get());
+                    ExprSet * attrs2 = dynamic_cast<ExprSet *>(j->second.e.get());
                     if (!attrs2) {
                         attrPath.erase(i + 1, attrPath.end());
                         dupAttr(attrPath, pos, j->second.pos);
@@ -115,12 +115,12 @@ inline void State::addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_
             } else {
                 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());
+                    std::tuple(std::make_unique<ExprSet>(), pos));
+                attrs = static_cast<ExprSet *>(next.first->second.e.get());
             }
         } else {
-            auto & next = attrs->dynamicAttrs.emplace_back(std::move(i->expr), std::make_unique<ExprAttrs>(), pos);
-            attrs = static_cast<ExprAttrs *>(next.valueExpr.get());
+            auto & next = attrs->dynamicAttrs.emplace_back(std::move(i->expr), std::make_unique<ExprSet>(), pos);
+            attrs = static_cast<ExprSet *>(next.valueExpr.get());
         }
     }
     // Expr insertion.
@@ -132,8 +132,8 @@ inline void State::addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_
             // 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.get());
-            auto * jAttrs = dynamic_cast<ExprAttrs *>(j->second.e.get());
+            auto * ae = dynamic_cast<ExprSet *>(e.get());
+            auto * jAttrs = dynamic_cast<ExprSet *>(j->second.e.get());
             if (jAttrs && ae) {
                 if (ae->inheritFromExprs && !jAttrs->inheritFromExprs)
                     jAttrs->inheritFromExprs = std::make_unique<std::vector<ref<Expr>>>();
@@ -157,8 +157,9 @@ inline void State::addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_
         } else {
             // Before inserting new attrs, check for __override and throw an error
             // (the error will initially be a warning to ease migration)
-            if (attrs->recursive && !featureSettings.isEnabled(Dep::RecSetOverrides) && i->symbol == s.overrides) {
-                overridesFound(pos);
+            if (!featureSettings.isEnabled(Dep::RecSetOverrides) && i->symbol == s.overrides) {
+                if (auto set = dynamic_cast<ExprSet *>(attrs); set && set->recursive)
+                    overridesFound(pos);
             }
 
             // This attr path is not defined. Let's create it.

lix/libexpr/print.cc

@@ -581,10 +581,4 @@ fmt_internal::HintFmt & fmt_internal::HintFmt::operator%(const ValuePrinter & va
         return *this;
 }
 
-std::ostream & operator<<(std::ostream & output, ExprPrinter const & printer)
-{
-    printer.expr.show(printer.state.ctx.symbols, output);
-    return output;
-}
-
 }

lix/libexpr/print.hh

@@ -81,26 +81,4 @@ std::ostream & operator<<(std::ostream & output, const ValuePrinter & printer);
 template<>
 fmt_internal::HintFmt & fmt_internal::HintFmt::operator%(const ValuePrinter & value);
 
-/**
- * A partially-applied form of Expr::show(), which can be formatted using `<<`
- * without allocating an intermediate string.
- * This class should not outlive the eval state or it will UAF.
- * FIXME: This should take `nix::ref`s, to avoid that, but our eval methods all have
- * EvalState &, not ref<EvalState>, and constructing a new shared_ptr to data that
- * already has a shared_ptr is a much bigger footgun. In the current architecture of
- * libexpr, using an ExprPrinter after an EvalState has been destroyed would be
- * pretty hard.
- */
-class ExprPrinter
-{
-    /** The eval state used to get symbols. */
-    EvalState const & state;
-    /** The expression to print. */
-    Expr const & expr;
-
-public:
-    ExprPrinter(EvalState const & state, Expr const & expr) : state(state), expr(expr) { }
-    friend std::ostream & operator << (std::ostream & output, ExprPrinter const & printer);
-};
-
 }

lix/libexpr/value.cc

@@ -55,8 +55,8 @@ bool Value::isTrivial() const
         internalType != tApp
         && internalType != tPrimOpApp
         && (internalType != tThunk
-            || (dynamic_cast<ExprAttrs *>(thunk.expr)
-                && static_cast<ExprAttrs *>(thunk.expr)->dynamicAttrs.empty())
+            || (dynamic_cast<ExprSet *>(thunk.expr)
+                && static_cast<ExprSet *>(thunk.expr)->dynamicAttrs.empty())
             || dynamic_cast<ExprLambda *>(thunk.expr)
             || dynamic_cast<ExprList *>(thunk.expr));
 }

tests/functional/lang/eval-fail-assert.err.exp

@@ -13,7 +13,7 @@ error:
              |       ^
             3| in
 
-       error: assertion '(arg == "y")' failed
+       error: assertion failed
        at /pwd/lang/eval-fail-assert.nix:2:12:
             1| let
             2|   x = arg: assert arg == "y"; 123;

tests/functional/lang/eval-fail-attr-name-type.err.exp

@@ -1,5 +1,5 @@
 error:
-       … while evaluating the attribute 'puppy."${key}"'
+       … while evaluating the attribute 'puppy."${...}"'
          at /pwd/lang/eval-fail-attr-name-type.nix:3:5:
             2|   attrs = {
             3|     puppy.doggy = {};

tests/functional/lang/eval-fail-recursion.err.exp

@@ -1,5 +1,5 @@
 error:
-       … while evaluating 'a' to select 'foo' on it
+       … while evaluating an expression to select 'foo' on it
          at /pwd/lang/eval-fail-recursion.nix:1:21:
             1| let a = {} // a; in a.foo
              |                     ^

tests/functional/lang/eval-fail-remove.err.exp

@@ -1,7 +1,7 @@
 error: attribute 'x' missing
-       at /pwd/lang/eval-fail-remove.nix:4:3:
+       at /pwd/lang/eval-fail-remove.nix:4:29:
             3| in
             4|   (removeAttrs attrs ["x"]).x
-             |   ^
+             |                             ^
             5|
        Did you mean y?

tests/functional/lang/eval-fail-select-err.err.exp

@@ -6,11 +6,11 @@ error:
              |   ^
             3| in somepkg.src.meta
 
-       … while evaluating 'somepkg.src' to select 'meta' on it
-         at /pwd/lang/eval-fail-select-err.nix:3:4:
+       … while evaluating an expression to select 'meta' on it
+         at /pwd/lang/eval-fail-select-err.nix:3:16:
             2|   somepkg.src = throw "invalid foo bar";
             3| in somepkg.src.meta
-             |    ^
+             |                ^
             4|
 
        … caused by explicit throw

tests/functional/lang/parse-okay-rec-set-override-warning.err.exp

@@ -1,2 +1,2 @@
 warning: __overrides found at «stdin»:3:16. This feature is deprecated and will be removed in the future. Use --extra-deprecated-features rec-set-overrides to silence this warning.
-warning: __overrides found at «stdin»:4:2. This feature is deprecated and will be removed in the future. Use --extra-deprecated-features rec-set-overrides to silence this warning.
+warning: __overrides found at «stdin»:4:8. This feature is deprecated and will be removed in the future. Use --extra-deprecated-features rec-set-overrides to silence this warning.

tests/unit/libexpr/value/print.cc

@@ -514,7 +514,7 @@ TEST_F(ValuePrintingTests, ansiColorsAssert)
 
     ASSERT_EQ(v.type(), nAttrs);
     test(*v.attrs->begin()->value,
-         ANSI_RED "«error: assertion 'false' failed»" ANSI_NORMAL,
+         ANSI_RED "«error: assertion failed»" ANSI_NORMAL,
          PrintOptions {
              .ansiColors = true,
              .force = true