Skip to content

Rewrite Struct.new assignments inside blocks - #10637

Open
fly1d wants to merge 5 commits into
sorbet:masterfrom
fly1d:codex/struct-rewriter-nested-block
Open

fly1d wants to merge 5 commits into
sorbet:masterfrom
fly1d:codex/struct-rewriter-nested-block

Conversation

@fly1d

@fly1d fly1d commented Sep 2, 2026

Copy link
Copy Markdown

Motivation

Fixes #10631.

Constant assignments such as Generated = Struct.new(:value) normally go through the Struct rewriter, which gives the generated class its members and constructor type. The old entry point only considered direct class-body statements, so assignments nested under another expression skipped the rewrite and strict files received error 7027 with a T.let(..., Struct) autocorrect that fails at runtime.

This change invokes the existing Struct rewriter from postTransformAssign, allowing the tree walk to handle matching assignments wherever they occur in the AST. It does not special-case blocks. For assignments whose value is used, the generated classes are emitted as InsSeq statements and a rebuilt reference to the target constant remains as the result, preserving Ruby assignment-expression semantics. The reference builder handles lexical, root-qualified, relative-qualified, and self:: paths without adding a generic AST deepCopy.

Direct class-body replacements are still spliced into the class RHS to preserve the existing AST shape. Local-variable Struct.new calls remain unchanged because they do not match the Struct rewriter's constant-lhs requirement.

The regression fixture covers direct and nested blocks, immediate use of the generated class, an unchanged local-variable path, a class nested inside a block, a conditional assignment outside a block, and assignment values returned from blocks, including a self::-qualified target. The rewrite-tree snapshot also verifies that direct class-body assignments are not rewritten twice.

Test plan

  • tools/scripts/format_cxx.sh -t rewriter/rewriter.cc
  • Legacy and Prism positive tests for struct_nested_block
  • Legacy and Prism LSP tests for struct_nested_block
  • All 304 Legacy and Prism test_PosTests/testdata/rewriter/* targets
  • git diff --check

Prepared with OpenAI Codex assistance.

Apply the existing Struct rewriter to constant assignments nested in blocks while keeping the generated classes at the original expression location.

Add strict and rewrite-tree coverage for direct and nested blocks, including the unchanged local-variable path.

Assisted-by: OpenAI Codex.
@fly1d
fly1d requested a review from a team as a code owner September 2, 2026 06:43
@fly1d
fly1d requested review from froydnj and removed request for a team September 2, 2026 06:43
Track the block depth at each class boundary so direct assignments inside a class nested in a block continue through the existing class-body rewriter path.

Cover an invalid Struct member once to catch duplicate rewrites.

Assisted-by: OpenAI Codex.
Keep generated class definitions as statements and return the original constant from rewritten assignments so block expressions retain Ruby assignment semantics.

Add a map-based type regression for the generated class value.

Assisted-by: OpenAI Codex.

@jez jez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't believe that this change should need to look at blocks at all—blocks is a red herring. It should simply traverse all assignments inside the tree.

The change should not need to deepCopy anything.

@froydnj froydnj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with jez's comments.

Move Struct rewriting to postTransformAssign so nested assignments are handled regardless of their enclosing AST node. Splice direct class-body replacements back into the class RHS to preserve the existing tree shape, and add conditional-assignment coverage.

Assisted-by: OpenAI Codex.
@fly1d

fly1d commented Sep 3, 2026

Copy link
Copy Markdown
Author

Thanks, that makes sense. I removed the block-depth tracking and moved the Struct rewrite into postTransformAssign, so it now applies uniformly to assignments throughout the tree. Direct class-body replacements are still spliced into the class RHS to preserve the existing AST shape, and the new code does not add any deepCopy.

I also added a conditional-assignment case to cover nesting outside blocks. All 304 Legacy and Prism rewriter targets pass locally, along with both positive and LSP variants for the fixture. Could you take another look when convenient?

Return a reconstructed constant reference after emitting the generated classes so rewritten assignments keep Ruby expression semantics. Normalize self-qualified paths to lexical reads and keep direct class-body expansion unchanged.

Restore block-value regression coverage for ordinary and self-qualified constants.

Assisted-by: OpenAI Codex.
@fly1d

fly1d commented Sep 8, 2026

Copy link
Copy Markdown
Author

I found one return-value regression while rechecking the latest revision: the generated ClassDef had become InsSeq.expr, so a Struct assignment used as a block result was inferred as T::Array[NilClass].

Commit 5a3c809 keeps both generated classes in InsSeq.stats and returns a reconstructed reference to the target constant. Direct class-body splicing still preserves the previous two-ClassDef shape. self::-qualified paths are normalized to lexical reads, and the change does not add a deepCopy.

I restored the block-result regression and added self::-qualified coverage. The Legacy and Prism positive and LSP fixture targets pass, as do all 304 Legacy and Prism rewriter positive targets.

@fly1d

fly1d commented Sep 11, 2026

Copy link
Copy Markdown
Author

@jez, could you unblock Buildkite for the latest commit when convenient? The local Legacy and Prism rewriter suites are green.

@jez
jez self-requested a review September 11, 2026 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error 7027 autocorrect emits T.let annotation that fails at runtime for Struct.new

3 participants