-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle dynamic constant assignment #295
Conversation
b31b872
to
d384577
Compare
// Enter the name of the constant so that it's available for the rest of the pipeline | ||
gs.enterNameConstant(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have extracted this up out of the conditional, like this:
auto prismName = parser.resolveConstant(node->name);
auto sorbetName = gs.enterNameConstant(name);
if (isInMethodDef) {
return make_unique<LVarLhs>(location, core::Names::dynamicConstAssign());
}
return make_unique<SorbetLHSNode>(location, move(parent), sorbetName);
... but I opted not to, because it makes it look like sorbetName
is an unused variable in the early-return. Repeating it here gives me a place to explicit document the side-effect of enterNameConstant
that we're relying on.
|
||
Translator(Translator &&) = delete; // Move constructor | ||
Translator(const Translator &) = delete; // Copy constructor | ||
Translator &operator=(Translator &&) = delete; // Move assignment | ||
Translator &operator=(const Translator &) = delete; // Copy assignment | ||
public: | ||
Translator(Parser parser, core::GlobalState &gs) : parser(parser), gs(gs) {} // Default constructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe this was never the default constructor, since it had parameters. Such are the perils of comments.
@@ -63,13 +69,16 @@ class Translator final { | |||
std::unique_ptr<SorbetAssignmentNode> translateOpAssignment(pm_node_t *node); | |||
|
|||
template <typename PrismLhsNode, typename SorbetLHSNode> | |||
std::unique_ptr<SorbetLHSNode> translateConst(PrismLhsNode *node); | |||
std::unique_ptr<parser::Node> translateConst(PrismLhsNode *node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to be less specific about the return type, to account for the case where if SorbetLHSNode
is parser::CVarLHS
, where we need to return a parser::LVarLhs
for the workaround instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo! Looks like you might need a rebase.
d384577
to
029a23c
Compare
Motivation
Closes #293 by reimplementing this workaround done by Sorbet's parser:
sorbet/parser/Builder.cc
Lines 348 to 354 in 97d7fff