nixd/Parser: make parsing memory-safe for errors, using RAII

nixd/include/nixd/Expr/CallbackExpr.h

@@ -22,7 +22,7 @@ using ExprCallback = std::function<void(nix::Expr *, nix::EvalState &,
     void eval(nix::EvalState &State, nix::Env &Env, nix::Value &V) override;   \
     std::string getName();                                                     \
   };
-#include "NixASTNodes.inc"
+#include "Nodes.inc"
 #undef NIX_EXPR
 
 /// Rewrite the AST, rooted at \p Root, \returns the root of the result tree.

nixd/include/nixd/Expr/Expr.h

@@ -6,29 +6,35 @@
 
 namespace nixd {
 
-/// Holds AST Nodes.
-class ASTContext {
+/// RAII Pool that holds Nodes.
+template <class T> class Context {
 public:
-  std::vector<std::unique_ptr<nix::Expr>> Nodes;
-  template <class T> T *addNode(std::unique_ptr<T> Node) {
+  std::vector<std::unique_ptr<T>> Nodes;
+  template <class U> U *addNode(std::unique_ptr<U> Node) {
     Nodes.push_back(std::move(Node));
-    return dynamic_cast<T *>(Nodes.back().get());
+    return dynamic_cast<U *>(Nodes.back().get());
+  }
+  template <class U> U *record(U *Node) {
+    Nodes.emplace_back(std::unique_ptr<U>(Node));
+    return Node;
   }
 };
 
+using ASTContext = Context<nix::Expr>;
+
 template <class Derived> struct RecursiveASTVisitor {
 
   bool shouldTraversePostOrder() { return false; }
 
   bool visitExpr(const nix::Expr *) { return true; }
 
 #define NIX_EXPR(EXPR) bool traverse##EXPR(const nix::EXPR *E);
-#include "NixASTNodes.inc"
+#include "Nodes.inc"
 #undef NIX_EXPR
 
 #define NIX_EXPR(EXPR)                                                         \
   bool visit##EXPR(const nix::EXPR *E) { return getDerived().visitExpr(E); }
-#include "NixASTNodes.inc"
+#include "Nodes.inc"
 #undef NIX_EXPR
 
   Derived &getDerived() { return *static_cast<Derived *>(this); }
@@ -40,7 +46,7 @@ template <class Derived> struct RecursiveASTVisitor {
   if (auto CE = dynamic_cast<const nix::EXPR *>(E)) {                          \
     return getDerived().traverse##EXPR(CE);                                    \
   }
-#include "NixASTNodes.inc"
+#include "Nodes.inc"
     assert(false && "We are missing some nix AST Nodes!");
     return true;
 #undef NIX_EXPR
@@ -65,7 +71,7 @@ template <class Derived> struct RecursiveASTVisitor {
       TRY_TO(visit##TYPE(T));                                                  \
     return true;                                                               \
   }
-#include "NixASTTraverse.inc"
+#include "Traverse.inc"
 #undef DEF_TRAVERSE_TYPE
 #undef TRY_TO_TRAVERSE
 #undef TRY_TO
@@ -75,10 +81,9 @@ inline const char *getExprName(const nix::Expr *E) {
   if (dynamic_cast<const nix::EXPR *>(E)) {                                    \
     return #EXPR;                                                              \
   }
-#include "NixASTNodes.inc"
-  assert(
-      false &&
-      "Cannot dynamic-cast to nix::Expr*, missing entries in NixASTNodes.inc?");
+#include "Nodes.inc"
+  assert(false &&
+         "Cannot dynamic-cast to nix::Expr*, missing entries in Nodes.inc?");
   return nullptr;
 #undef NIX_EXPR
 }

nixd/include/nixd/Parser/Epilogue.cpp

@@ -43,44 +43,4 @@ std::unique_ptr<ParseData> parse(char *text, size_t length, Pos::Origin origin,
   return data; // NRVO
 }
 
-void tryFree(auto *&Ptr, std::set<void *> &Freed) {
-  if (Freed.contains(Ptr))
-    return;
-
-  delete Ptr;
-  Freed.insert(Ptr);
-  Ptr = nullptr;
-}
-
-void destroyAST(auto *&Root, std::set<void *> &Freed) {
-  if (Freed.contains(Root))
-    return;
-#define TRY_TO_TRAVERSE(CHILD)                                                 \
-  do {                                                                         \
-    destroyAST(CHILD, Freed);                                                  \
-    (CHILD) = nullptr;                                                         \
-  } while (false)
-
-#define DEF_TRAVERSE_TYPE(TYPE, CODE)                                          \
-  if (auto *T = dynamic_cast<nix::TYPE *>(Root)) {                             \
-    { CODE; }                                                                  \
-  }
-#include "nixd/Expr/NixASTTraverse.inc"
-#undef TRY_TO_TRAVERSE
-#undef DEF_TRAVERSE_TYPE
-  if (auto *E = dynamic_cast<nix::ExprConcatStrings *>(Root)) {
-    tryFree(E->es, Freed);
-  }
-  if (auto *E = dynamic_cast<nix::ExprLambda *>(Root)) {
-    tryFree(E->formals, Freed);
-  }
-  tryFree(Root, Freed);
-}
-
-ParseData::~ParseData() {
-  // Destroy AST nodes;
-  std::set<void *> Freed;
-  destroyAST(result, Freed);
-}
-
 } // namespace nixd

nixd/include/nixd/Parser/Prologue.h

@@ -6,6 +6,7 @@
 #include "Lexer.tab.h"
 
 #include "Provides.h"
+#include "Require.h"
 
 YY_DECL;
 
@@ -43,18 +44,24 @@ static void addAttr(ExprAttrs *attrs, AttrPath &&attrPath, Expr *e,
       if (j != attrs->attrs.end()) {
         if (!j->second.inherited) {
           ExprAttrs *attrs2 = dynamic_cast<ExprAttrs *>(j->second.e);
-          if (!attrs2)
+          if (!attrs2) {
+
             dupAttr(data, attrPath, pos, j->second.pos);
+            return;
+          }
           attrs = attrs2;
-        } else
+        } else {
+
           dupAttr(data, attrPath, pos, j->second.pos);
+          return;
+        }
       } else {
-        ExprAttrs *nested = new ExprAttrs;
+        ExprAttrs *nested = data.ctx.record(new ExprAttrs);
         attrs->attrs[i->symbol] = ExprAttrs::AttrDef(nested, pos);
         attrs = nested;
       }
     } else {
-      ExprAttrs *nested = new ExprAttrs;
+      ExprAttrs *nested = data.ctx.record(new ExprAttrs);
       attrs->dynamicAttrs.push_back(
           ExprAttrs::DynamicAttrDef(i->expr, nested, pos));
       attrs = nested;
@@ -74,13 +81,18 @@ static void addAttr(ExprAttrs *attrs, AttrPath &&attrPath, Expr *e,
       if (jAttrs && ae) {
         for (auto &ad : ae->attrs) {
           auto j2 = jAttrs->attrs.find(ad.first);
-          if (j2 !=
-              jAttrs->attrs.end()) // Attr already defined in iAttrs, error.
+          if (j2 != jAttrs->attrs.end()) {
+            // Attr already defined in iAttrs, error.
+
             dupAttr(data, ad.first, j2->second.pos, ad.second.pos);
+            return;
+          }
           jAttrs->attrs.emplace(ad.first, ad.second);
         }
       } else {
+
         dupAttr(data, attrPath, pos, j->second.pos);
+        return;
       }
     } else {
       // This attr path is not defined. Let's create it.
@@ -122,15 +134,14 @@ static Formals *toFormals(ParseData &data, ParserFormals *formals,
                        data.state.symbols[arg]),
         .errPos = data.state.positions[pos]});
 
-  delete formals;
-  return new Formals(std::move(result));
+  return data.FsCtx.record(new Formals(std::move(result)));
 }
 
 static Expr *stripIndentation(
-    const PosIdx pos, SymbolTable &symbols,
+    ParseData &data, const PosIdx pos, SymbolTable &symbols,
     std::vector<std::pair<PosIdx, std::variant<Expr *, StringToken>>> &&es) {
   if (es.empty())
-    return new ExprString("");
+    return data.ctx.record(new ExprString(""));
 
   /* Figure out the minimum indentation.  Note that by design
      whitespace-only final lines are not taken into account.  (So
@@ -172,6 +183,7 @@ static Expr *stripIndentation(
 
   /* Strip spaces from each line. */
   auto *es2 = new std::vector<std::pair<PosIdx, Expr *>>;
+  data.SPCtx.record(es2);
   atStartOfLine = true;
   size_t curDropped = 0;
   size_t n = es.size();
@@ -212,7 +224,7 @@ static Expr *stripIndentation(
         s2 = std::string(s2, 0, p + 1);
     }
 
-    es2->emplace_back(i->first, new ExprString(std::move(s2)));
+    es2->emplace_back(i->first, data.ctx.record(new ExprString(std::move(s2))));
   };
   for (; i != es.end(); ++i, --n) {
     std::visit(overloaded{trimExpr, trimString}, i->second);
@@ -221,10 +233,9 @@ static Expr *stripIndentation(
   /* If this is a single string, then don't do a concatenation. */
   if (es2->size() == 1 && dynamic_cast<ExprString *>((*es2)[0].second)) {
     auto *const result = (*es2)[0].second;
-    delete es2;
     return result;
   }
-  return new ExprConcatStrings(pos, true, es2);
+  return data.ctx.record(new ExprConcatStrings(pos, true, es2));
 }
 
 } // namespace nixd

nixd/include/nixd/Parser/Require.h

@@ -1,48 +1,65 @@
 #pragma once
 
+#include "nixd/Expr/Expr.h"
+
 #include <nix/eval.hh>
 #include <nix/globals.hh>
 #include <nix/nixexpr.hh>
+#include <nix/types.hh>
 #include <nix/util.hh>
 
 #include <variant>
 
+// using C a struct allows us to avoid having to define the special
+// members that using string_view here would implicitly delete.
+struct StringToken {
+  const char *p;
+  size_t l;
+  bool hasIndentation;
+  operator std::string_view() const { return {p, l}; }
+};
+
 namespace nixd {
 
 using namespace nix;
 
+struct ParserFormals {
+  std::vector<nix::Formal> formals;
+  bool ellipsis = false;
+};
 struct ParseState {
   SymbolTable &symbols;
   PosTable &positions;
 };
 
 struct ParseData {
+  using IndStringParts = std::vector<
+      std::pair<nix::PosIdx, std::variant<nix::Expr *, StringToken>>>;
+  using StringParts = std::vector<std::pair<nix::PosIdx, nix::Expr *>>;
+  using AttrNames = std::vector<nix::AttrName>;
+
   ParseState state;
   Expr *result;
   SourcePath basePath;
   PosTable::Origin origin;
   std::vector<ErrorInfo> error;
   std::map<PosIdx, PosIdx> end;
   std::map<const void *, PosIdx> locations;
-  ~ParseData();
-};
 
-struct ParserFormals {
-  std::vector<Formal> formals;
-  bool ellipsis = false;
-};
+  ASTContext ctx;
 
-} // namespace nixd
+  Context<ParserFormals> PFCtx;
+  Context<nix::Formal> FCtx;
+  Context<nix::Formals> FsCtx;
+  Context<nix::AttrPath> APCtx;
 
-// using C a struct allows us to avoid having to define the special
-// members that using string_view here would implicitly delete.
-struct StringToken {
-  const char *p;
-  size_t l;
-  bool hasIndentation;
-  operator std::string_view() const { return {p, l}; }
+  Context<AttrNames> ANCtx;
+  Context<StringParts> SPCtx;
+  Context<IndStringParts> ISPCtx;
 };
 
+} // namespace nixd
+
 #define YY_DECL                                                                \
   int yylex(YYSTYPE *yylval_param, YYLTYPE *yylloc_param, yyscan_t yyscanner,  \
             nixd::ParseData *data)

nixd/lib/Expr/CallbackExpr.cpp

@@ -14,7 +14,7 @@ namespace nixd {
     return Cxt.addNode<Callback##EXPR>(                                        \
         std::make_unique<Callback##EXPR>(E, ECB));                             \
   }
-#include "nixd/Expr/NixASTNodes.inc"
+#include "nixd/Expr/Nodes.inc"
 #undef NIX_EXPR
 
 nix::Expr *
@@ -33,7 +33,7 @@ rewriteCallback(ASTContext &Cxt, ExprCallback ECB, const nix::Expr *Root,
     { CODE; }                                                                  \
     return T;                                                                  \
   }
-#include "nixd/Expr/NixASTTraverse.inc"
+#include "nixd/Expr/Traverse.inc"
   return nullptr;
 #undef TRY_TO_TRAVERSE
 #undef DEF_TRAVERSE_TYPE
@@ -44,7 +44,7 @@ rewriteCallback(ASTContext &Cxt, ExprCallback ECB, const nix::Expr *Root,
     nix::EXPR::eval(State, Env, V);                                            \
     ECB(this, State, Env, V);                                                  \
   }
-#include "nixd/Expr/NixASTNodes.inc"
+#include "nixd/Expr/Nodes.inc"
 #undef NIX_EXPR
 
 } // namespace nixd