pass Exprs as references, not pointers

almost all places where Exprs are passed as pointers expect the pointers
to be non-null. pass them as references instead to encode this
constraint in types.

src/libcmd/installables.cc

@@ -260,7 +260,7 @@ void SourceExprCommand::completeInstallable(AddCompletions & completions, std::s
 
             evalSettings.pureEval = false;
             auto state = getEvalState();
-            auto e =
+            auto & e =
                 state->parseExprFromFile(
                     resolveExprPath(
                         lookupFileArg(*state, *file)));
@@ -483,7 +483,7 @@ Installables SourceExprCommand::parseInstallables(
         auto vFile = state->allocValue();
 
         if (file == "-") {
-            auto e = state->parseStdin();
+            auto & e = state->parseStdin();
             state->eval(e, *vFile);
         }
         else if (file) {
@@ -491,7 +491,7 @@ Installables SourceExprCommand::parseInstallables(
         }
         else {
             CanonPath dir(CanonPath::fromCwd(getCommandBaseDir()));
-            auto e = state->parseExprFromString(*expr, state->rootPath(dir));
+            auto & e = state->parseExprFromString(*expr, state->rootPath(dir));
             state->eval(e, *vFile);
         }
 

src/libcmd/repl.cc

@@ -89,7 +89,7 @@ struct NixRepl
     void reloadFiles();
     void addAttrsToScope(Value & attrs);
     void addVarToScope(const Symbol name, Value & v);
-    Expr * parseString(std::string s);
+    Expr & parseString(std::string s);
     void evalString(std::string s, Value & v);
     void loadDebugTraceEnv(DebugTrace & dt);
 
@@ -416,9 +416,9 @@ StringSet NixRepl::completePrefix(const std::string & prefix)
             auto expr = cur.substr(0, dot);
             auto cur2 = cur.substr(dot + 1);
 
-            Expr * e = parseString(expr);
+            Expr & e = parseString(expr);
             Value v;
-            e->eval(*state, *env, v);
+            e.eval(*state, *env, v);
             state->forceAttrs(v, noPos, "while evaluating an attrset for the purpose of completion (this error should not be displayed; file an issue?)");
 
             for (auto & i : *v.attrs) {
@@ -779,7 +779,7 @@ bool NixRepl::processLine(std::string line)
             line[p + 1] != '=' &&
             isVarName(name = removeWhitespace(line.substr(0, p))))
         {
-            Expr * e = parseString(line.substr(p + 1));
+            Expr & e = parseString(line.substr(p + 1));
             Value & v(*state->allocValue());
             v.mkThunk(env, e);
             addVarToScope(state->symbols.create(name), v);
@@ -895,7 +895,7 @@ void NixRepl::addVarToScope(const Symbol name, Value & v)
 }
 
 
-Expr * NixRepl::parseString(std::string s)
+Expr & NixRepl::parseString(std::string s)
 {
     return state->parseExprFromString(std::move(s), state->rootPath(CanonPath::fromCwd()), staticEnv);
 }
@@ -903,8 +903,8 @@ Expr * NixRepl::parseString(std::string s)
 
 void NixRepl::evalString(std::string s, Value & v)
 {
-    Expr * e = parseString(s);
+    Expr & e = parseString(s);
-    e->eval(*state, *env, v);
+    e.eval(*state, *env, v);
     state->forceValue(v, v.determinePos(noPos));
 }
 

src/libexpr/eval-inline.hh

@@ -86,11 +86,11 @@ void EvalState::forceValue(Value & v, const PosIdx pos)
 {
     if (v.isThunk()) {
         Env * env = v.thunk.env;
-        Expr * expr = v.thunk.expr;
+        Expr & expr = *v.thunk.expr;
         try {
             v.mkBlackhole();
             //checkInterrupt();
-            expr->eval(*this, *env, v);
+            expr.eval(*this, *env, v);
         } catch (...) {
             v.mkThunk(env, expr);
             tryFixupBlackHolePos(v, pos);

src/libexpr/eval.cc

@@ -907,14 +907,14 @@ void EvalState::mkList(Value & v, size_t size)
 
 unsigned long nrThunks = 0;
 
-static inline void mkThunk(Value & v, Env & env, Expr * expr)
+static inline void mkThunk(Value & v, Env & env, Expr & expr)
 {
     v.mkThunk(&env, expr);
     nrThunks++;
 }
 
 
-void EvalState::mkThunk_(Value & v, Expr * expr)
+void EvalState::mkThunk_(Value & v, Expr & expr)
 {
     mkThunk(v, baseEnv, expr);
 }
@@ -1015,7 +1015,7 @@ void EvalState::mkSingleDerivedPathString(
 Value * Expr::maybeThunk(EvalState & state, Env & env)
 {
     Value * v = state.allocValue();
-    mkThunk(*v, env, this);
+    mkThunk(*v, env, *this);
     return v;
 }
 
@@ -1077,7 +1077,7 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
         e = j->second;
 
     if (!e)
-        e = parseExprFromFile(resolvedPath);
+        e = &parseExprFromFile(resolvedPath);
 
     fileParseCache[resolvedPath] = e;
 
@@ -1096,7 +1096,7 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
         if (mustBeTrivial &&
             !(dynamic_cast<ExprAttrs *>(e)))
             error<EvalError>("file '%s' must be an attribute set", path).debugThrow();
-        eval(e, v);
+        eval(*e, v);
     } catch (Error & e) {
         addErrorTrace(e, "while evaluating the file '%1%':", resolvedPath.to_string());
         throw;
@@ -1114,23 +1114,23 @@ void EvalState::resetFileCache()
 }
 
 
-void EvalState::eval(Expr * e, Value & v)
+void EvalState::eval(Expr & e, Value & v)
 {
-    e->eval(*this, baseEnv, v);
+    e.eval(*this, baseEnv, v);
 }
 
 
-inline bool EvalState::evalBool(Env & env, Expr * e, const PosIdx pos, std::string_view errorCtx)
+inline bool EvalState::evalBool(Env & env, Expr & e, const PosIdx pos, std::string_view errorCtx)
 {
     try {
         Value v;
-        e->eval(*this, env, v);
+        e.eval(*this, env, v);
         if (v.type() != nBool)
             error<TypeError>(
                  "expected a Boolean but found %1%: %2%",
                  showType(v),
                  ValuePrinter(*this, v, errorPrintOptions)
-             ).atPos(pos).withFrame(env, *e).debugThrow();
+             ).atPos(pos).withFrame(env, e).debugThrow();
         return v.boolean;
     } catch (Error & e) {
         e.addTrace(positions[pos], errorCtx);
@@ -1139,16 +1139,16 @@ inline bool EvalState::evalBool(Env & env, Expr * e, const PosIdx pos, std::stri
 }
 
 
-inline void EvalState::evalAttrs(Env & env, Expr * e, Value & v, const PosIdx pos, std::string_view errorCtx)
+inline void EvalState::evalAttrs(Env & env, Expr & e, Value & v, const PosIdx pos, std::string_view errorCtx)
 {
     try {
-        e->eval(*this, env, v);
+        e.eval(*this, env, v);
         if (v.type() != nAttrs)
             error<TypeError>(
                 "expected a set but found %1%: %2%",
                 showType(v),
                 ValuePrinter(*this, v, errorPrintOptions)
-            ).withFrame(env, *e).debugThrow();
+            ).withFrame(env, e).debugThrow();
     } catch (Error & e) {
         e.addTrace(positions[pos], errorCtx);
         throw;
@@ -1221,7 +1221,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
             Value * vAttr;
             if (hasOverrides && i.second.kind != AttrDef::Kind::Inherited) {
                 vAttr = state.allocValue();
-                mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, inheritEnv), i.second.e);
+                mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, inheritEnv), *i.second.e);
             } else
                 vAttr = i.second.e->maybeThunk(state, *i.second.chooseByKind(&env2, &env, inheritEnv));
             env2.values[displ++] = vAttr;
@@ -1817,13 +1817,13 @@ void ExprWith::eval(EvalState & state, Env & env, Value & v)
 void ExprIf::eval(EvalState & state, Env & env, Value & v)
 {
     // We cheat in the parser, and pass the position of the condition as the position of the if itself.
-    (state.evalBool(env, cond, pos, "while evaluating a branch condition") ? then : else_)->eval(state, env, v);
+    (state.evalBool(env, *cond, pos, "while evaluating a branch condition") ? *then : *else_).eval(state, env, v);
 }
 
 
 void ExprAssert::eval(EvalState & state, Env & env, Value & v)
 {
-    if (!state.evalBool(env, cond, pos, "in the condition of the assert statement")) {
+    if (!state.evalBool(env, *cond, pos, "in the condition of the assert statement")) {
         std::ostringstream out;
         cond->show(state.symbols, out);
         state.error<AssertionError>("assertion '%1%' failed", out.str()).atPos(pos).withFrame(env, *this).debugThrow();
@@ -1834,7 +1834,7 @@ void ExprAssert::eval(EvalState & state, Env & env, Value & v)
 
 void ExprOpNot::eval(EvalState & state, Env & env, Value & v)
 {
-    v.mkBool(!state.evalBool(env, e, getPos(), "in the argument of the not operator")); // XXX: FIXME: !
+    v.mkBool(!state.evalBool(env, *e, getPos(), "in the argument of the not operator")); // XXX: FIXME: !
 }
 
 
@@ -1856,27 +1856,27 @@ void ExprOpNEq::eval(EvalState & state, Env & env, Value & v)
 
 void ExprOpAnd::eval(EvalState & state, Env & env, Value & v)
 {
-    v.mkBool(state.evalBool(env, e1, pos, "in the left operand of the AND (&&) operator") && state.evalBool(env, e2, pos, "in the right operand of the AND (&&) operator"));
+    v.mkBool(state.evalBool(env, *e1, pos, "in the left operand of the AND (&&) operator") && state.evalBool(env, *e2, pos, "in the right operand of the AND (&&) operator"));
 }
 
 
 void ExprOpOr::eval(EvalState & state, Env & env, Value & v)
 {
-    v.mkBool(state.evalBool(env, e1, pos, "in the left operand of the OR (||) operator") || state.evalBool(env, e2, pos, "in the right operand of the OR (||) operator"));
+    v.mkBool(state.evalBool(env, *e1, pos, "in the left operand of the OR (||) operator") || state.evalBool(env, *e2, pos, "in the right operand of the OR (||) operator"));
 }
 
 
 void ExprOpImpl::eval(EvalState & state, Env & env, Value & v)
 {
-    v.mkBool(!state.evalBool(env, e1, pos, "in the left operand of the IMPL (->) operator") || state.evalBool(env, e2, pos, "in the right operand of the IMPL (->) operator"));
+    v.mkBool(!state.evalBool(env, *e1, pos, "in the left operand of the IMPL (->) operator") || state.evalBool(env, *e2, pos, "in the right operand of the IMPL (->) operator"));
 }
 
 
 void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v)
 {
     Value v1, v2;
-    state.evalAttrs(env, e1, v1, pos, "in the left operand of the update (//) operator");
+    state.evalAttrs(env, *e1, v1, pos, "in the left operand of the update (//) operator");
-    state.evalAttrs(env, e2, v2, pos, "in the right operand of the update (//) operator");
+    state.evalAttrs(env, *e2, v2, pos, "in the right operand of the update (//) operator");
 
     state.nrOpUpdates++;
 
@@ -2726,39 +2726,39 @@ SourcePath resolveExprPath(SourcePath path)
 }
 
 
-Expr * EvalState::parseExprFromFile(const SourcePath & path)
+Expr & EvalState::parseExprFromFile(const SourcePath & path)
 {
     return parseExprFromFile(path, staticBaseEnv);
 }
 
 
-Expr * EvalState::parseExprFromFile(const SourcePath & path, std::shared_ptr<StaticEnv> & staticEnv)
+Expr & EvalState::parseExprFromFile(const SourcePath & path, std::shared_ptr<StaticEnv> & staticEnv)
 {
     auto buffer = path.resolveSymlinks().readFile();
     // readFile hopefully have left some extra space for terminators
     buffer.append("\0\0", 2);
-    return parse(buffer.data(), buffer.size(), Pos::Origin(path), path.parent(), staticEnv);
+    return *parse(buffer.data(), buffer.size(), Pos::Origin(path), path.parent(), staticEnv);
 }
 
 
-Expr * EvalState::parseExprFromString(std::string s_, const SourcePath & basePath, std::shared_ptr<StaticEnv> & staticEnv)
+Expr & EvalState::parseExprFromString(std::string s_, const SourcePath & basePath, std::shared_ptr<StaticEnv> & staticEnv)
 {
     // NOTE this method (and parseStdin) must take care to *fully copy* their input
     // into their respective Pos::Origin until the parser stops overwriting its input
     // data.
     auto s = make_ref<std::string>(s_);
     s_.append("\0\0", 2);
-    return parse(s_.data(), s_.size(), Pos::String{.source = s}, basePath, staticEnv);
+    return *parse(s_.data(), s_.size(), Pos::String{.source = s}, basePath, staticEnv);
 }
 
 
-Expr * EvalState::parseExprFromString(std::string s, const SourcePath & basePath)
+Expr & EvalState::parseExprFromString(std::string s, const SourcePath & basePath)
 {
     return parseExprFromString(std::move(s), basePath, staticBaseEnv);
 }
 
 
-Expr * EvalState::parseStdin()
+Expr & EvalState::parseStdin()
 {
     // NOTE this method (and parseExprFromString) must take care to *fully copy* their
     // input into their respective Pos::Origin until the parser stops overwriting its
@@ -2768,7 +2768,7 @@ Expr * EvalState::parseStdin()
     auto s = make_ref<std::string>(buffer);
     // drainFD should have left some extra space for terminators
     buffer.append("\0\0", 2);
-    return parse(buffer.data(), buffer.size(), Pos::Stdin{.source = s}, rootPath(CanonPath::fromCwd()), staticBaseEnv);
+    return *parse(buffer.data(), buffer.size(), Pos::Stdin{.source = s}, rootPath(CanonPath::fromCwd()), staticBaseEnv);
 }
 
 

src/libexpr/eval.hh

@@ -338,16 +338,16 @@ public:
     /**
      * Parse a Nix expression from the specified file.
      */
-    Expr * parseExprFromFile(const SourcePath & path);
+    Expr & parseExprFromFile(const SourcePath & path);
-    Expr * parseExprFromFile(const SourcePath & path, std::shared_ptr<StaticEnv> & staticEnv);
+    Expr & parseExprFromFile(const SourcePath & path, std::shared_ptr<StaticEnv> & staticEnv);
 
     /**
      * Parse a Nix expression from the specified string.
      */
-    Expr * parseExprFromString(std::string s, const SourcePath & basePath, std::shared_ptr<StaticEnv> & staticEnv);
+    Expr & parseExprFromString(std::string s, const SourcePath & basePath, std::shared_ptr<StaticEnv> & staticEnv);
-    Expr * parseExprFromString(std::string s, const SourcePath & basePath);
+    Expr & parseExprFromString(std::string s, const SourcePath & basePath);
 
-    Expr * parseStdin();
+    Expr & parseStdin();
 
     /**
      * Evaluate an expression read from the given file to normal
@@ -380,15 +380,15 @@ public:
      *
      * @param [out] v The resulting is stored here.
      */
-    void eval(Expr * e, Value & v);
+    void eval(Expr & e, Value & v);
 
     /**
      * Evaluation the expression, then verify that it has the expected
      * type.
      */
-    inline bool evalBool(Env & env, Expr * e);
+    inline bool evalBool(Env & env, Expr & e);
-    inline bool evalBool(Env & env, Expr * e, const PosIdx pos, std::string_view errorCtx);
+    inline bool evalBool(Env & env, Expr & e, const PosIdx pos, std::string_view errorCtx);
-    inline void evalAttrs(Env & env, Expr * e, Value & v, const PosIdx pos, std::string_view errorCtx);
+    inline void evalAttrs(Env & env, Expr & e, Value & v, const PosIdx pos, std::string_view errorCtx);
 
     /**
      * If `v` is a thunk, enter it and overwrite `v` with the result
@@ -607,7 +607,7 @@ public:
     }
 
     void mkList(Value & v, size_t length);
-    void mkThunk_(Value & v, Expr * expr);
+    void mkThunk_(Value & v, Expr & expr);
     void mkPos(Value & v, PosIdx pos);
 
     /**

src/libexpr/nixexpr.hh

@@ -42,6 +42,11 @@ std::string showAttrPath(const SymbolTable & symbols, const AttrPath & attrPath)
 
 struct Expr
 {
+protected:
+    Expr(Expr &&) = default;
+    Expr & operator=(Expr &&) = default;
+
+public:
     struct AstSymbols {
         Symbol sub, lessThan, mul, div, or_, findFile, nixPath, body;
     };
@@ -51,7 +56,10 @@ struct Expr
     Expr() {
         nrExprs++;
     }
+    Expr(const Expr &) = delete;
+    Expr & operator=(const Expr &) = delete;
     virtual ~Expr() { };
+
     virtual void show(const SymbolTable & symbols, std::ostream & str) const;
     virtual void bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env);
     virtual void eval(EvalState & state, Env & env, Value & v);

src/libexpr/primops.cc

@@ -231,9 +231,9 @@ static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * v
             // args[0]->attrs is already sorted.
 
             printTalkative("evaluating file '%1%'", path);
-            Expr * e = state.parseExprFromFile(resolveExprPath(path), staticEnv);
+            Expr & e = state.parseExprFromFile(resolveExprPath(path), staticEnv);
 
-            e->eval(state, *env, v);
+            e.eval(state, *env, v);
         }
     }
 }
@@ -383,13 +383,13 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v)
     auto output = runProgram(program, true, commandArgs);
     Expr * parsed;
     try {
-        parsed = state.parseExprFromString(std::move(output), state.rootPath(CanonPath::root));
+        parsed = &state.parseExprFromString(std::move(output), state.rootPath(CanonPath::root));
     } catch (Error & e) {
         e.addTrace(state.positions[pos], "while parsing the output from '%1%'", program);
         throw;
     }
     try {
-        state.eval(parsed, v);
+        state.eval(*parsed, v);
     } catch (Error & e) {
         e.addTrace(state.positions[pos], "while evaluating the output from '%1%'", program);
         throw;

src/libexpr/value.hh

@@ -336,11 +336,11 @@ public:
         }
     }
 
-    inline void mkThunk(Env * e, Expr * ex)
+    inline void mkThunk(Env * e, Expr & ex)
     {
         internalType = tThunk;
         thunk.env = e;
-        thunk.expr = ex;
+        thunk.expr = &ex;
     }
 
     inline void mkApp(Value * l, Value * r)

src/nix-build/nix-build.cc

@@ -292,7 +292,7 @@ static void main_nix_build(int argc, char * * argv)
     PackageInfos drvs;
 
     /* Parse the expressions. */
-    std::vector<Expr *> exprs;
+    std::vector<std::reference_wrapper<Expr>> exprs;
 
     if (readStdin)
         exprs = {state->parseStdin()};
@@ -398,7 +398,7 @@ static void main_nix_build(int argc, char * * argv)
         if (!shell) {
 
             try {
-                auto expr = state->parseExprFromString(
+                auto & expr = state->parseExprFromString(
                     "(import <nixpkgs> {}).bashInteractive",
                     state->rootPath(CanonPath::fromCwd()));
 

src/nix-env/nix-env.cc

@@ -413,7 +413,7 @@ static void queryInstSources(EvalState & state,
             loadSourceExpr(state, *instSource.nixExprPath, vArg);
 
             for (auto & i : args) {
-                Expr * eFun = state.parseExprFromString(i, state.rootPath(CanonPath::fromCwd()));
+                Expr & eFun = state.parseExprFromString(i, state.rootPath(CanonPath::fromCwd()));
                 Value vFun, vTmp;
                 state.eval(eFun, vFun);
                 vTmp.mkApp(&vFun, &vArg);

src/nix-instantiate/nix-instantiate.cc

@@ -28,10 +28,10 @@ enum OutputKind { okPlain, okXML, okJSON };
 
 void processExpr(EvalState & state, const Strings & attrPaths,
     bool parseOnly, bool strict, Bindings & autoArgs,
-    bool evalOnly, OutputKind output, bool location, Expr * e)
+    bool evalOnly, OutputKind output, bool location, Expr & e)
 {
     if (parseOnly) {
-        e->show(state.symbols, std::cout);
+        e.show(state.symbols, std::cout);
         std::cout << "\n";
         return;
     }
@@ -176,14 +176,14 @@ static int main_nix_instantiate(int argc, char * * argv)
         }
 
         if (readStdin) {
-            Expr * e = state->parseStdin();
+            Expr & e = state->parseStdin();
             processExpr(*state, attrPaths, parseOnly, strict, autoArgs,
                 evalOnly, outputKind, xmlOutputSourceLocation, e);
         } else if (files.empty() && !fromArgs)
             files.push_back("./default.nix");
 
         for (auto & i : files) {
-            Expr * e = fromArgs
+            Expr & e = fromArgs
                 ? state->parseExprFromString(i, state->rootPath(CanonPath::fromCwd()))
                 : state->parseExprFromFile(resolveExprPath(lookupFileArg(*state, i)));
             processExpr(*state, attrPaths, parseOnly, strict, autoArgs,

tests/unit/libexpr-support/tests/libexpr.hh

@@ -30,8 +30,7 @@ namespace nix {
             }
             Value eval(std::string input, bool forceValue = true) {
                 Value v;
-                Expr * e = state.parseExprFromString(input, state.rootPath(CanonPath::root));
+                Expr & e = state.parseExprFromString(input, state.rootPath(CanonPath::root));
-                assert(e);
                 state.eval(e, v);
                 if (forceValue)
                     state.forceValue(v, noPos);

tests/unit/libexpr/value/print.cc

@@ -91,7 +91,8 @@ TEST_F(ValuePrintingTests, tList)
 TEST_F(ValuePrintingTests, vThunk)
 {
     Value vThunk;
-    vThunk.mkThunk(nullptr, nullptr);
+    ExprInt e(0);
+    vThunk.mkThunk(nullptr, e);
 
     test(vThunk, "«thunk»");
 }
@@ -521,7 +522,7 @@ TEST_F(ValuePrintingTests, ansiColorsAssert)
     ExprAssert expr(noPos, &eFalse, &eInt);
 
     Value v;
-    state.mkThunk_(v, &expr);
+    state.mkThunk_(v, expr);
 
     test(v,
          ANSI_RED "«error: assertion 'false' failed»" ANSI_NORMAL,
@@ -621,7 +622,8 @@ TEST_F(ValuePrintingTests, ansiColorsPrimOpApp)
 TEST_F(ValuePrintingTests, ansiColorsThunk)
 {
     Value v;
-    v.mkThunk(nullptr, nullptr);
+    ExprInt e(0);
+    v.mkThunk(nullptr, e);
 
     test(v,
          ANSI_MAGENTA "«thunk»" ANSI_NORMAL,