Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix bugs in translation of PM_RESCUE_NODE #317

Merged
merged 5 commits into from
Nov 5, 2024
Merged

Fix bugs in translation of PM_RESCUE_NODE #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f86eda5
Fix edge case where `PM_RESCUE_NODE` has empty body
egiurleo Nov 1, 2024
92c51ee
Only do dynamic constant assignment workaround in specific cases
egiurleo Nov 1, 2024
88d6459
Test dynamic constant workaround for all constant assignment types
egiurleo Nov 1, 2024
8f8bc8f
Add more test cases to rescue prism regression tests
egiurleo Nov 5, 2024
fcc8a72
Handle all constant path assignment node types
egiurleo Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 27 additions & 10 deletions parser/prism/Translator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ unique_ptr<SorbetAssignmentNode> Translator::translateOpAssignment(pm_node_t *un
is_same_v<PrismAssignmentNode, pm_constant_path_and_write_node> ||
is_same_v<PrismAssignmentNode, pm_constant_path_or_write_node>) {
// Handle operator assignment to a constant path, like `A::B::C += 1` or `::C += 1`
lhs = translateConst<pm_constant_path_node, parser::ConstLhs>(node->target);
bool skipDynamicConstantWorkaround = true;
lhs = translateConst<pm_constant_path_node, parser::ConstLhs>(node->target, skipDynamicConstantWorkaround);
} else if constexpr (is_same_v<SorbetLHSNode, parser::Send>) {
// Handle operator assignment to the result of a method call, like `a.b += 1`
auto name = parser.resolveConstant(node->read_name);
Expand Down Expand Up @@ -1585,6 +1586,7 @@ unique_ptr<parser::Node> Translator::translateCallWithBlock(pm_node_t *prismBloc
// (and its linked `subsequent` nodes) and assembling the corresponding `Rescue` and `Resbody` nodes in Sorbet's AST.
unique_ptr<parser::Node> Translator::translateRescue(pm_rescue_node *prismRescueNode, unique_ptr<parser::Node> bodyNode,
unique_ptr<parser::Node> elseNode) {
auto rescueLoc = translateLoc(prismRescueNode->base.location);
NodeVec rescueBodies;

// Each `rescue` clause generates a `Resbody` node, which is a child of the `Rescue` node.
Expand All @@ -1608,7 +1610,7 @@ unique_ptr<parser::Node> Translator::translateRescue(pm_rescue_node *prismRescue
}

// The `Rescue` node combines the main body, the rescue clauses, and the else clause.
return make_unique<parser::Rescue>(bodyNode->loc, move(bodyNode), move(rescueBodies), move(elseNode));
return make_unique<parser::Rescue>(rescueLoc, move(bodyNode), move(rescueBodies), move(elseNode));
}

// Translates the given Prism Statements Node into a `parser::Begin` node or an inlined `parser::Node`.
Expand All @@ -1630,10 +1632,20 @@ unique_ptr<parser::Node> Translator::translateStatements(pm_statements_node *stm

// Handles any one of the Prism nodes that models any kind of assignment to a constant or constant path.
//
// Dynamic constant assignment inside of a method definition will raise a SyntaxError at runtime. In the
// Sorbet validator, there is a check that will crash Sorbet if this is detected statically.
// To work around this, we substitute dynamic constant assignments with a write to a fake local variable
// called `dynamicConstAssign`.
//
// The only exception is that dynamic constant path *operator* assignments inside of a method definition
// do not raise a SyntaxError at runtime, so we want to skip the workaround in that case.
// However, within this method, both regular constant path assignments and constant path operator assignments
// are passed in as `pm_constant_path_node` types, so we need an extra boolean flag to know when to skip the workaround.
//
// Usually returns the `SorbetLHSNode`, but for constant writes and targets,
// it can can return an `LVarLhs` as a workaround in the case of a dynamic constant assignment.
template <typename PrismLhsNode, typename SorbetLHSNode>
unique_ptr<parser::Node> Translator::translateConst(PrismLhsNode *node) {
unique_ptr<parser::Node> Translator::translateConst(PrismLhsNode *node, bool skipDynamicConstantWorkaround) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a blurb to the function docs that explains what this is (and why constant path op assignments need it)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amomchilov Let me know what you think of the explanation above!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

static_assert(is_same_v<SorbetLHSNode, parser::Const> || is_same_v<SorbetLHSNode, parser::ConstLhs>,
"Invalid LHS type. Must be one of `parser::Const` or `parser::ConstLhs`.");

Expand Down Expand Up @@ -1668,14 +1680,20 @@ unique_ptr<parser::Node> Translator::translateConst(PrismLhsNode *node) {
auto location = translateLoc(node->base.location);
auto name = parser.resolveConstant(node->name);

if (isInMethodDef) { // Check if this is a dynamic constant assignment (SyntaxError at runtime)
// This is a copy of a workaround from `Desugar.cc`, which substitues in a fake assignment,
// so the parsing can continue. See other usages of `dynamicConstAssign` for more details.
if constexpr (is_same_v<PrismLhsNode, pm_constant_write_node> || is_same_v<PrismLhsNode, pm_constant_path_node> ||
is_same_v<PrismLhsNode, pm_constant_operator_write_node> ||
is_same_v<PrismLhsNode, pm_constant_or_write_node> ||
is_same_v<PrismLhsNode, pm_constant_and_write_node>) {
if (isInMethodDef &&
!skipDynamicConstantWorkaround) { // Check if this is a dynamic constant assignment (SyntaxError at runtime)
// This is a copy of a workaround from `Desugar.cc`, which substitues in a fake assignment,
// so the parsing can continue. See other usages of `dynamicConstAssign` for more details.

// Enter the name of the constant so that it's available for the rest of the pipeline
gs.enterNameConstant(name);
// Enter the name of the constant so that it's available for the rest of the pipeline
gs.enterNameConstant(name);

return make_unique<LVarLhs>(location, core::Names::dynamicConstAssign());
return make_unique<LVarLhs>(location, core::Names::dynamicConstAssign());
}
}

return make_unique<SorbetLHSNode>(location, move(parent), gs.enterNameConstant(name));
Expand Down Expand Up @@ -1768,5 +1786,4 @@ void Translator::reportError(core::LocOffsets loc, const std::string &message) {
e.setHeader("{}", message);
}
}

}; // namespace sorbet::parser::Prism
2 changes: 1 addition & 1 deletion parser/prism/Translator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class Translator final {
std::unique_ptr<SorbetAssignmentNode> translateOpAssignment(pm_node_t *node);

template <typename PrismLhsNode, typename SorbetLHSNode>
std::unique_ptr<parser::Node> translateConst(PrismLhsNode *node);
std::unique_ptr<parser::Node> translateConst(PrismLhsNode *node, bool skipDynamicConstantWorkaround = false);
egiurleo marked this conversation as resolved.
Show resolved Hide resolved

// Pattern-matching
// ... variations of the main translation functions for pattern-matching related nodes.
Expand Down
296 changes: 282 additions & 14 deletions test/prism_regression/assign_to_constant.parse-tree.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,25 +398,293 @@ Begin {
DefMethod {
name = <U method1>
args = NULL
body = Assign {
lhs = LVarLhs {
name = <U <dynamic-const-assign>>
}
rhs = String {
val = <U This should raise SyntaxError at runtime>
}
body = Begin {
stmts = [
Assign {
lhs = LVarLhs {
name = <U <dynamic-const-assign>>
}
rhs = String {
val = <U These should all raise SyntaxErrors at runtime>
}
}
OpAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
op = <U &>
right = Integer {
val = "2"
}
}
OpAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
op = <U ^>
right = Integer {
val = "4"
}
}
OpAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
op = <U >>>
right = Integer {
val = "5"
}
}
OpAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
op = <U <<>
right = Integer {
val = "6"
}
}
OpAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
op = <U ->
right = Integer {
val = "7"
}
}
OpAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
op = <U %>
right = Integer {
val = "8"
}
}
OpAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
op = <U |>
right = Integer {
val = "9"
}
}
OpAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
op = <U />
right = Integer {
val = "10"
}
}
OpAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
op = <U *>
right = Integer {
val = "11"
}
}
OpAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
op = <U **>
right = Integer {
val = "12"
}
}
AndAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
right = Integer {
val = "13"
}
}
OrAsgn {
left = LVarLhs {
name = <U <dynamic-const-assign>>
}
right = Integer {
val = "14"
}
}
]
}
}
DefMethod {
name = <U method2>
args = NULL
body = Assign {
lhs = LVarLhs {
name = <U <dynamic-const-assign>>
}
rhs = String {
val = <U This should raise SyntaxError at runtime>
}
body = Begin {
stmts = [
Assign {
lhs = LVarLhs {
name = <U <dynamic-const-assign>>
}
rhs = String {
val = <U This should raise a SyntaxError at runtime>
}
}
OpAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantBitwiseAnd>>
}
op = <U &>
right = Integer {
val = "2"
}
}
OpAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantBitwiseXor>>
}
op = <U ^>
right = Integer {
val = "4"
}
}
OpAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantShiftRight>>
}
op = <U >>>
right = Integer {
val = "5"
}
}
OpAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantShiftLeft>>
}
op = <U <<>
right = Integer {
val = "6"
}
}
OpAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantSubtractAssign>>
}
op = <U ->
right = Integer {
val = "7"
}
}
OpAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantModuleAssign>>
}
op = <U %>
right = Integer {
val = "8"
}
}
OpAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantBitwiseOr>>
}
op = <U |>
right = Integer {
val = "9"
}
}
OpAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantDivideAssign>>
}
op = <U />
right = Integer {
val = "10"
}
}
OpAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantMultiplyAssign>>
}
op = <U *>
right = Integer {
val = "11"
}
}
OpAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantExponentiateAssign>>
}
op = <U **>
right = Integer {
val = "12"
}
}
AndAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantLazyAndAssign>>
}
right = Integer {
val = "13"
}
}
OrAsgn {
left = ConstLhs {
scope = Const {
scope = NULL
name = <C <U ConstantPath>>
}
name = <C <U DynamicConstantLazyOrAssgin>>
}
right = Integer {
val = "14"
}
}
]
}
}
]
Expand Down
Loading
Loading