Decouple parser errors with parser (#425)

* Pass parse errors to Translator through ProgramNodeContainer

Instead of storing them in the Parser.

* Rename ProgramNodeContainer to ParseResult

Author: st0012

main/pipeline/pipeline.cc

@@ -138,15 +138,13 @@ unique_ptr<parser::Node> runPrismParser(core::GlobalState &gs, core::FileRef fil
     core::UnfreezeNameTable nameTableAccess(gs);
 
     Prism::Parser parser{source};
-    Prism::ProgramNodeContainer root = parser.parse_root();
+    Prism::ParseResult parseResult = parser.parse_root();
 
     if (stopAfterParser) {
         return std::unique_ptr<parser::Node>();
     }
 
-    // Needs to be called after `parse_root()` otherwise there will be no errors to collect
-    parser.collectErrors();
-    auto nodes = Prism::Translator(parser, gs, file).translate(std::move(root));
+    auto nodes = Prism::Translator(parser, gs, file).translate(std::move(parseResult));
 
     if (print.ParseTree.enabled) {
         print.ParseTree.fmt("{}\n", nodes->toStringWithTabs(gs, 0));

parser/prism/Parser.cc

@@ -6,9 +6,9 @@ pm_parser_t *Parser::getRawParserPointer() {
     return &storage->parser;
 }
 
-ProgramNodeContainer Parser::parse_root() {
+ParseResult Parser::parse_root() {
     pm_node_t *root = pm_parse(getRawParserPointer());
-    return ProgramNodeContainer{*this, root};
+    return ParseResult{*this, root, collectErrors()};
 };
 
 core::LocOffsets Parser::translateLocation(pm_location_t location) {
@@ -28,7 +28,8 @@ std::string_view Parser::extractString(pm_string_t *string) {
     return std::string_view(reinterpret_cast<const char *>(pm_string_source(string)), pm_string_length(string));
 }
 
-void Parser::collectErrors() {
+std::vector<ParseError> Parser::collectErrors() {
+    std::vector<ParseError> parseErrors;
     parseErrors.reserve(storage->parser.error_list.size);
 
     auto error_list = storage->parser.error_list;
@@ -42,5 +43,7 @@ void Parser::collectErrors() {
 
         parseErrors.push_back(parseError);
     }
+
+    return parseErrors;
 }
 }; // namespace sorbet::parser::Prism

parser/prism/Parser.h

@@ -13,7 +13,7 @@ extern "C" {
 
 namespace sorbet::parser::Prism {
 
-class ProgramNodeContainer;
+class ParseResult;
 
 class ParseError {
 public:
@@ -51,33 +51,28 @@ struct ParserStorage {
 };
 
 class Parser final {
-    friend class ProgramNodeContainer;
+    friend class ParseResult;
     friend struct NodeDeleter;
 
     std::shared_ptr<ParserStorage> storage;
 
 public:
-    std::vector<ParseError> parseErrors;
-
     Parser(std::string_view source_code) : storage(std::make_shared<ParserStorage>(source_code)) {}
 
     Parser(const Parser &) = default;
     Parser &operator=(const Parser &) = default;
 
-    ProgramNodeContainer parse_root();
+    ParseResult parse_root();
     core::LocOffsets translateLocation(pm_location_t location);
     std::string_view resolveConstant(pm_constant_id_t constant_id);
     std::string_view extractString(pm_string_t *string);
 
-    void collectErrors();
-
 private:
+    std::vector<ParseError> collectErrors();
     pm_parser_t *getRawParserPointer();
 };
 
-// This class holds a pointer to the parser and a pointer to the root node of Prism's AST
-// It's main purpose is to provide a way to clean up the AST nodes
-class ProgramNodeContainer final {
+class ParseResult final {
     struct NodeDeleter {
         Parser parser;
 
@@ -91,11 +86,13 @@ class ProgramNodeContainer final {
 
     Parser parser;
     std::unique_ptr<pm_node_t, NodeDeleter> node;
+    std::vector<ParseError> parseErrors;
 
-    ProgramNodeContainer(Parser parser, pm_node_t *node) : parser{parser}, node{node, NodeDeleter{parser}} {}
+    ParseResult(Parser parser, pm_node_t *node, std::vector<ParseError> parseErrors)
+        : parser{parser}, node{node, NodeDeleter{parser}}, parseErrors{parseErrors} {}
 
-    ProgramNodeContainer(const ProgramNodeContainer &) = delete;            // Copy constructor
-    ProgramNodeContainer &operator=(const ProgramNodeContainer &) = delete; // Copy assignment
+    ParseResult(const ParseResult &) = delete;            // Copy constructor
+    ParseResult &operator=(const ParseResult &) = delete; // Copy assignment
 
     pm_node_t *getRawNodePointer() const {
         return node.get();

parser/prism/Translator.cc

@@ -1339,7 +1339,7 @@ unique_ptr<parser::Node> Translator::translate(pm_node_t *node) {
             // For now, we only report errors when we hit a missing node because we don't want to always report dynamic
             // constant assignment errors
             // TODO: We will improve this in the future when we handle more errored cases
-            for (auto &error : parser.parseErrors) {
+            for (auto &error : parseErrors) {
                 // EOF error is always pointed to the very last line of the file, which can't be expressed in Sorbet's
                 // error comments
                 if (error.id != PM_ERR_UNEXPECTED_TOKEN_CLOSE_CONTEXT) {
@@ -1350,8 +1350,9 @@ unique_ptr<parser::Node> Translator::translate(pm_node_t *node) {
     }
 }
 
-unique_ptr<parser::Node> Translator::translate(const ProgramNodeContainer &container) {
-    return translate(container.getRawNodePointer());
+unique_ptr<parser::Node> Translator::translate(const ParseResult &parseResult) {
+    this->parseErrors = parseResult.parseErrors;
+    return translate(parseResult.getRawNodePointer());
 }
 
 core::LocOffsets Translator::translateLoc(pm_location_t loc) {
@@ -1969,7 +1970,7 @@ template <typename PrismNode> std::unique_ptr<parser::Mlhs> Translator::translat
 // Context management methods
 Translator Translator::enterMethodDef() {
     auto isInMethodDef = true;
-    return Translator(parser, gs, file, isInMethodDef, uniqueCounter);
+    return Translator(parser, gs, file, parseErrors, isInMethodDef, uniqueCounter);
 }
 
 void Translator::reportError(core::LocOffsets loc, const std::string &message) {

parser/prism/Translator.h

@@ -23,6 +23,9 @@ class Translator final {
     // Needed for reporting diagnostics
     core::FileRef file;
 
+    // The parse errors that occurred while parsing the root node
+    std::vector<ParseError> parseErrors;
+
     // Context variables
     bool isInMethodDef = false;
 
@@ -47,13 +50,14 @@ class Translator final {
 
     // Translates the given AST from Prism's node types into the equivalent AST in Sorbet's legacy parser node types.
     std::unique_ptr<parser::Node> translate(pm_node_t *node);
-    std::unique_ptr<parser::Node> translate(const ProgramNodeContainer &container);
+    std::unique_ptr<parser::Node> translate(const ParseResult &parseResult);
 
 private:
     // Private constructor used only for creating child translators
     // uniqueCounterStorage is passed as the minimum integer value and is never used
-    Translator(Parser parser, core::GlobalState &gs, core::FileRef file, bool isInMethodDef, int *uniqueCounter)
-        : parser(parser), gs(gs), file(file), isInMethodDef(isInMethodDef),
+    Translator(Parser parser, core::GlobalState &gs, core::FileRef file, std::vector<ParseError> parseErrors,
+               bool isInMethodDef, int *uniqueCounter)
+        : parser(parser), gs(gs), file(file), parseErrors(parseErrors), isInMethodDef(isInMethodDef),
           uniqueCounterStorage(std::numeric_limits<int>::min()), uniqueCounter(uniqueCounter) {}
     void reportError(core::LocOffsets loc, const std::string &message);