Crash when handing dynamic constant assignments

[amomchilov]

This code crashes Sorbet, and block us from type-checking the monolith:

def m
  C = 1
end

It should raise a SyntaxError at runtime, but it shouldn't crash Sorbet statically.

stacktrace

+ exec test/pipeline_test_runner --single_test=test/prism_regression/dynamic_constant_assignment.rb --parser=prism
[doctest] doctest version is "2.4.9"
[doctest] run with "--help" for options
Parsing with prism
[2024-10-22 17:02:41.276] [fatalFallback] [error] Exception::raise(): ast/verifier/Verifier.cc:29 enforced condition methodDepth == 0 has failed: Found constant definition inside method definition

[2024-10-22 17:02:41.705] [fatalFallback] [error] Backtrace:
sorbet::ast::VerifierWalker::postTransformAssign(sorbet::core::Context, sorbet::ast::Assign const&) (in pipeline_test_runner) + 272
decltype(auto) sorbet::ast::MapFunctions<(sorbet::ast::TreeMapKind)2>::CALL_MEMBER_impl_postTransformAssign<sorbet::ast::VerifierWalker, true>::call<sorbet::core::Context&, sorbet::ast::Assign const&>(sorbet::ast::VerifierWalker&, sorbet::core::Context&, sorbet::ast::Assign const&) (in pipeline_test_runner) + 64
sorbet::ast::TreeMapper<sorbet::ast::VerifierWalker, sorbet::core::Context, (sorbet::ast::TreeMapKind)2, (sorbet::ast::TreeMapDepthKind)0>::mapAssign(sorbet::ast::ExpressionPtr const&, sorbet::core::Context) (in pipeline_test_runner) + 172
sorbet::ast::TreeMapper<sorbet::ast::VerifierWalker, sorbet::core::Context, (sorbet::ast::TreeMapKind)2, (sorbet::ast::TreeMapDepthKind)0>::mapIt(sorbet::ast::ExpressionPtr const&, sorbet::core::Context) (in pipeline_test_runner) + 1524
sorbet::ast::TreeMapper<sorbet::ast::VerifierWalker, sorbet::core::Context, (sorbet::ast::TreeMapKind)2, (sorbet::ast::TreeMapDepthKind)0>::mapMethodDef(sorbet::ast::ExpressionPtr const&, sorbet::core::Context) (in pipeline_test_runner) + 412
sorbet::ast::TreeMapper<sorbet::ast::VerifierWalker, sorbet::core::Context, (sorbet::ast::TreeMapKind)2, (sorbet::ast::TreeMapDepthKind)0>::mapIt(sorbet::ast::ExpressionPtr const&, sorbet::core::Context) (in pipeline_test_runner) + 344
sorbet::ast::TreeMapper<sorbet::ast::VerifierWalker, sorbet::core::Context, (sorbet::ast::TreeMapKind)2, (sorbet::ast::TreeMapDepthKind)0>::mapClassDef(sorbet::ast::ExpressionPtr const&, sorbet::core::Context) (in pipeline_test_runner) + 228
sorbet::ast::TreeMapper<sorbet::ast::VerifierWalker, sorbet::core::Context, (sorbet::ast::TreeMapKind)2, (sorbet::ast::TreeMapDepthKind)0>::mapIt(sorbet::ast::ExpressionPtr const&, sorbet::core::Context) (in pipeline_test_runner) + 288
void sorbet::ast::ConstTreeWalk::apply<sorbet::core::Context, sorbet::ast::VerifierWalker>(sorbet::core::Context, sorbet::ast::VerifierWalker&, sorbet::ast::ExpressionPtr const&) (in pipeline_test_runner) + 84
sorbet::ast::Verifier::run(sorbet::core::Context, sorbet::ast::ExpressionPtr) (in pipeline_test_runner) + 72
sorbet::ast::desugar::node2Tree(sorbet::core::MutableContext, std::__1::unique_ptr<sorbet::parser::Node, std::__1::default_delete<sorbet::parser::Node>>, bool) (in pipeline_test_runner) + 444
sorbet::test::index(std::__1::unique_ptr<sorbet::core::GlobalState, std::__1::default_delete<sorbet::core::GlobalState>>&, absl::lts_20240722::Span<sorbet::core::FileRef>, sorbet::test::ExpectationHandler&, sorbet::test::Expectations&) (in pipeline_test_runner) + 4608
sorbet::test::DOCTEST_ANON_FUNC_10() (in pipeline_test_runner) + 3996
doctest::Context::run() (in pipeline_test_runner) + 4388
main (in pipeline_test_runner) + 1772
start (in dyld) + 2840

===============================================================================
test/pipeline_test_runner.cc:347:
TEST CASE:  PerPhaseTest

test/pipeline_test_runner.cc:347: ERROR: test case THREW exception: ast/verifier/Verifier.cc:29 enforced condition methodDepth == 0 has failed: Found constant definition inside method definition

===============================================================================
[doctest] test cases: 1 | 0 passed | 1 failed | 0 skipped
[doctest] assertions: 8 | 8 passed | 0 failed |
[doctest] Status: FAILURE!
================================================================================
Target //test:test_PosTests/prism_regression/dynamic_constant_assignment up-to-date:
  bazel-bin/test/test_PosTests/prism_regression/dynamic_constant_assignment

Sorbet's handling

Sorbet's parser handles by diagnosing a syntax error early on, and rewriting the dynamic constant assignment into a fake write to a dummy local variable called dynamic-const-assign (core::Names::dynamicConstAssign()).

sorbet/parser/Builder.cc

Lines 348 to 354 in 97d7fff

	} else if (auto *c = parser::cast_node<Const>(node.get())) {
	if (driver_->lex.context.inDef) {
	error(ruby_parser::dclass::DynamicConst, node->loc);
	// Error recovery: lie and say it was an assign to something else so that the parse can continue
	auto name = core::Names::dynamicConstAssign();
	driver_->lex.declare(name.shortName(gs_));
	return make_unique<LVarLhs>(c->loc, name);

This is the expected parse tree:

DefMethod {
  name = <U m>
  args = NULL
  body = Assign {
    lhs = LVarLhs {
      name = <U <dynamic-const-assign>>
    }
    rhs = Integer {
      val = "1"
    }
  }
}

Thus, by the time this tree reaches the Verifier, there's no more dynamic constant assignment, and this ENFORCE doesn't trip.

sorbet/ast/verifier/Verifier.cc

Lines 27 to 31 in 97d7fff

	void postTransformAssign(core::Context ctx, const Assign &assign) {
	if (ast::isa_tree<ast::UnresolvedConstantLit>(assign.lhs)) {
	ENFORCE(methodDepth == 0, "Found constant definition inside method definition");
	}
	}

How Prism models this

Prism doesn't model this in the AST itself, but in the Prism::Result#errors field.

Prism.parse "def x; C = 123; end"
#<Prism::ParseResult:0x0000000149c796c0 @value=@ ProgramNode (location: (1,0)-(1,19))
├── flags: ∅
├── locals: []
└── statements:
    @ StatementsNode (location: (1,0)-(1,19))
    ├── flags: ∅
    └── body: (length: 1)
        └── @ DefNode (location: (1,0)-(1,19))
            ├── flags: newline
            ├── name: :x
            ├── name_loc: (1,4)-(1,5) = "x"
            ├── receiver: ∅
            ├── parameters: ∅
            ├── body:
            │   @ StatementsNode (location: (1,7)-(1,14))
            │   ├── flags: ∅
            │   └── body: (length: 1)
            │       └── @ ConstantWriteNode (location: (1,7)-(1,14))
            │           ├── flags: newline
            │           ├── name: :C
            │           ├── name_loc: (1,7)-(1,8) = "C"
            │           ├── value:
            │           │   @ IntegerNode (location: (1,11)-(1,14))
            │           │   ├── flags: static_literal, decimal
            │           │   └── value: 123
            │           └── operator_loc: (1,9)-(1,10) = "="
            ├── locals: []
            ├── def_keyword_loc: (1,0)-(1,3) = "def"
            ├── operator_loc: ∅
            ├── lparen_loc: ∅
            ├── rparen_loc: ∅
            ├── equal_loc: ∅
            └── end_keyword_loc: (1,16)-(1,19) = "end"
, @comments=[], @magic_comments=[], @data_loc=nil, @errors=[#<Prism::ParseError @type=:write_target_in_method @message="dynamic constant assignment" @location=#<Prism::Location @start_offset=7 @length=7 start_line=1> @level=:syntax>], @warnings=[], @source=#<Prism::ASCIISource:0x00000001491d9be8 @source="def x; C = 123; end", @start_line=1, @offsets=[0]>>

Solution ideas

1. Add a flag to Prism's ConstantWriteNode and ConstantTargetNode

We could define new a new flag that's tagged on the relevant node:

/**
 * Flags for constant write nodes.
 */
typedef enum pm_integer_base_flags {
    /** This constant write will cause "SyntaxError: dynamic constant assignment" */
    PM_CONSTANT_WRITE_FLAGS_DYNAMIC_ASSIGNMENT = 4,
}  pm_constant_write_flags_t


/**
 * Flags for constant target nodes.
 */
typedef enum pm_integer_base_flags {
    /** This constant target will cause "SyntaxError: dynamic constant assignment" */
    PM_CONSTANT_TARGET_FLAGS_DYNAMIC_ASSIGNMENT = 4,
}  pm_constant_target_flags_t

This would let Sorbet's translator inspect the node directly for this information, without needing any other parser/lexing context.

2. Track lexical scopes in Prism::Translator

There's no direct way to infer the currently lexical scope for a Prism AST node, because the nodes only have one-way references "down" the tree, and no upwards references to their parent.

So if we want the lexical information, we'll need to track it ourselves. We can add state to Prism::Translator that we update any time we enter/exit methods and classes, to keep track of which lexical scope we're in.

3. Correlate to the Parser errors

We can pass the Parser errors array to the Translator. Every time we do a constant assignment, we can search the errors (by the location information) to see if that assignment is a dynamic constant assignment.

[amomchilov]

We decided on option 2, because keeping track of a parsing context will be necessary anyway, once we start removing our translation layer (related: #240). Have a look at against DesugarContext for comparison.

Implemented in #295.