Scaffolding for gradual migration to `AST::Expression` #240

amomchilov · 2024-09-17T22:59:30Z

Motivation

Test plan

See included automated tests.

amomchilov

Notes to self and future readers :)

amomchilov · 2024-09-18T03:47:08Z

parser/Node.h

-    static_assert(std::is_final<To>::value, "To is not final");
+    // static_assert(std::is_final<To>::value, "To is not final");


I had to temporarily disable this, since the classes we're checking against are no longer final :/

amomchilov · 2024-09-18T03:48:05Z

ast/desugar/Desugar.cc

@@ -704,6 +704,12 @@ ExpressionPtr node2TreeImpl(DesugarContext dctx, unique_ptr<parser::Node> what)
        if (what.get() == nullptr) {
            return MK::EmptyTree();
        }
+
+        if (auto expr = what->getCachedDesugaredExpr()) {


The translations we implement in the Translator will fill this in, which we can use here to short-circuit the logic below.

Thinking goes that once we've implemented it for all nodes, none of the code below will be used anymore, and we can just delete and inline all this stuff.

amomchilov · 2024-09-18T03:48:40Z

main/pipeline/pipeline.cc

-    return Prism::Translator(parser, gs).translate(std::move(root));
+    return Prism::Translator(std::move(parser), gs).translate(std::move(root));


This just saves one reference count bump on the shared_ptr within the parser. Won't someone think of those poor poor clock cycles?!

amomchilov · 2024-09-18T03:49:01Z

parser/Node.h

@@ -1,6 +1,7 @@
 #ifndef SORBET_PARSER_NODE_H
 #define SORBET_PARSER_NODE_H

+#include "ast/Trees.h"


Needed for ast::ExpressionPtr

amomchilov · 2024-09-18T03:49:30Z

parser/tools/generate_ast.cc

-    out << "class " << node.name << " final : public Node {" << '\n';
+    out << "class " << node.name << " : public Node {" << '\n';


For us to be able to make the NodeAndExpr subclasses of these code-genned classes, they can't be final anymore. We can undo this once our migration is done.

amomchilov · 2024-09-18T03:50:19Z

parser/prism/Translator.cc

+// Allocates a new `NodeAndExpr`, that's a subclass of `SorbetNode`, with no pre-computed `ExpressionPtr` AST.
+template <typename SorbetNode, typename... TArgs> unique_ptr<NodeAndExpr<SorbetNode>> make_node(TArgs &&...args) {
+    return std::make_unique<NodeAndExpr<SorbetNode>>(std::forward<TArgs>(args)...);
+}


This helper doesn't pull much weight, but it saved me from needing to change all the call-sites below as I tweak all the details of exactly how NodeAndExpr works.

amomchilov · 2024-09-18T03:52:39Z

parser/prism/Translator.cc

-            return translateSimpleKeyword<pm_true_node, parser::True>(node);
+            auto sorbetNode = translateSimpleKeyword<pm_true_node, parser::True>(node);
+
+            sorbetNode->cacheDesugaredExpr(MK::True(sorbetNode->loc));


Here's a simple example of how migrating a Prism Node to an ast:ExpressionPtr would look like. This is a simple case because it's just a leaf node, with not much other information.

We stash this expr into the NodeAndExpr sorbetNode, which will later be consumed by either a parent Node (which uses it to construct its own AST expression), or node2TreeImpl in Desugar.cc.

amomchilov · 2024-09-18T03:54:09Z

parser/prism/Translator.cc

@@ -1036,15 +1079,25 @@ std::unique_ptr<parser::Node> Translator::translateConstantPath(pm_constant_path
        parent = translate(node->parent);
    } else {                                                // This is a fully qualified constant reference, like `::A`.
        pm_location_t *delimiterLoc = &node->delimiter_loc; // The location of the `::`
-        parent = make_unique<parser::Cbase>(parser.translateLocation(delimiterLoc));
+        parent = make_node<parser::Cbase>(parser.translateLocation(delimiterLoc));


In retrospect, parent is a kind of ambiguous term here. To clarify, it's a parent part of the path, e.g. the A in A::B, but it's a child node, as far as the Prism AST structure goes.

amomchilov · 2024-09-18T03:58:22Z

parser/prism/Translator.cc

    }
+
+    sorbetNode->cacheDesugaredExpr(MK::UnresolvedConstant(sorbetNode->loc, std::move(parentExpr), sorbetName));


This is an example where producing one Node's AST Expression requires consuming its children's AST Expressions. In this case, we can only make the AST Expression for the C in ::A::B::C after we've made it for its child node that represents ::A::B

::A::B:C // <- Making this guy steals the `desugarExpr` out of the `::A::B` node below it. / \ ::A::B C / \ ::A B / \ CBase A

amomchilov force-pushed the Alex/translate-into-ast-expr-attempt-3 branch 2 times, most recently from bfecfca to ce43b1a Compare September 18, 2024 03:46

amomchilov commented Sep 18, 2024

View reviewed changes

egiurleo force-pushed the prism branch from 09b0114 to 5df4484 Compare September 18, 2024 16:17

amomchilov force-pushed the Alex/translate-into-ast-expr-attempt-3 branch from ce43b1a to 6248b0b Compare September 30, 2024 13:53

amomchilov mentioned this pull request Oct 4, 2024

[Exploration] Translate Prism nodes directly into AST::Expression nodes #238

Closed

egiurleo force-pushed the prism branch from c8235f6 to 7314c02 Compare October 4, 2024 20:43

amomchilov added 4 commits October 21, 2024 13:57

Add infra for translating to AST::Expression

c3e3f53

Generate AST expressions for some example nodes

6ae39dd

Translate another example

22a6993

Remove unnecessary deepCopy()

8b3ee1f

amomchilov force-pushed the Alex/translate-into-ast-expr-attempt-3 branch from 6248b0b to 8b3ee1f Compare October 21, 2024 17:58

amomchilov mentioned this pull request Oct 22, 2024

Crash when handing dynamic constant assignments #293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scaffolding for gradual migration to `AST::Expression` #240

Scaffolding for gradual migration to `AST::Expression` #240

amomchilov commented Sep 17, 2024

amomchilov left a comment

amomchilov Sep 18, 2024

amomchilov Sep 18, 2024

amomchilov Sep 18, 2024

amomchilov Sep 18, 2024

amomchilov Sep 18, 2024

amomchilov Sep 18, 2024

amomchilov Sep 18, 2024

amomchilov Sep 18, 2024

amomchilov Sep 18, 2024

		static_assert(std::is_final<To>::value, "To is not final");
		// static_assert(std::is_final<To>::value, "To is not final");

		return Prism::Translator(parser, gs).translate(std::move(root));
		return Prism::Translator(std::move(parser), gs).translate(std::move(root));

		out << "class " << node.name << " final : public Node {" << '\n';
		out << "class " << node.name << " : public Node {" << '\n';

		}

		sorbetNode->cacheDesugaredExpr(MK::UnresolvedConstant(sorbetNode->loc, std::move(parentExpr), sorbetName));

Scaffolding for gradual migration to AST::Expression #240

Are you sure you want to change the base?

Scaffolding for gradual migration to AST::Expression #240

Conversation

amomchilov commented Sep 17, 2024

Motivation

Test plan

amomchilov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scaffolding for gradual migration to `AST::Expression` #240

Scaffolding for gradual migration to `AST::Expression` #240