Fix interpolated symbol shorthand hash keys producing invalid Ruby#858
Merged
reese merged 2 commits intofables-tales:trunkfrom May 6, 2026
Merged
Conversation
A hash with an interpolated shorthand symbol key like `{ "#{x}": 1 }`
was rewritten to `{:"#{x}": 1}`, which is invalid Ruby — a second pass
through rubyfmt rejects it as a syntax error.
`format_interpolated_symbol_node` was hard-coding the opener as `:"`
and the closer as `"`, ignoring prism's `opening_loc`/`closing_loc`.
That works for the standalone literal `:"#{x}"` but corrupts the
shorthand-key form, where prism reports `opening_loc=`"`` and
`closing_loc=`":``. The function emitted `:"#{x}"` and then
`format_assoc_node` appended its own `:` separator.
Fix this by mirroring the pattern `format_symbol_node` already uses
for plain `SymbolNode`s: emit the actual opening/closing locations and
strip a trailing `:` from the closer (since `format_assoc_node`
re-emits that as the key/value separator). A `debug_assert!` on the
locations being present documents the invariant; the previous `:"`/`"`
defaults are kept as a graceful fallback.
Fixtures cover the bare hash form, the keyword-arg call site
`foo("#{x}": 1)`, multi-entry hashes, and a mixed shorthand /
interpolated-shorthand form.
Closes fables-tales#857.
dduugg
force-pushed
the
fix/interpolated-symbol-shorthand-hash-key
branch
from
f792b12 to
8b726ad
Compare
reese
approved these changes
May 6, 2026
Collaborator
reese
left a comment
reese
left a comment
There was a problem hiding this comment.
LGTM. I think a refactor would also be fine if you wanted to move some of that mirrored logic into format_assoc_node (in this PR or in a follow-on), up to you.
`format_symbol_node` and `format_interpolated_symbol_node` each carried
a copy of the same `strip_suffix(b":")` logic for shorthand hash-key
closers. Extract it into a single helper (`emit_symbol_key_closer`)
placed next to `format_assoc_node`, with a doc comment that names
`format_assoc_node` as the owner of the `:` separator (re-emitted as
`:` for shorthand or replaced with ` =>` in the rocket branch on mixed
hashes).
No behavior change. All 379 fixture tests pass, including the
mixed-form path where `as_symbol=false` but source used shorthand
(`{foo: 1, 'bar' => 2}` → `{:foo => 1, "bar" => 2}`), which exercises
the strip in the rocket-rendering direction.
Contributor
Author
|
@reese TY for the quick review. I added a second commit to de-duplicate the logic, PTaL. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #857.
Bug
A hash with an interpolated shorthand symbol key was rewritten to invalid Ruby:
Cause
format_interpolated_symbol_nodehard-coded the opener as:"and the closer as", ignoring prism'sopening_loc/closing_loc. That works for the standalone literal:"#{x}"but corrupts the shorthand-key form, where prism reportsopening_loc = "andclosing_loc = ":. The function emitted:"#{x}", thenformat_assoc_nodeappended its own:separator.Fix
Mirror the pattern that
format_symbol_nodealready uses for plainSymbolNodes: emit the actual opening / closing locations and strip a trailing:from the closer (sinceformat_assoc_nodere-emits that as the key/value separator). Adebug_assert!documents that the locations are always present in practice; the previous:"/"defaults remain as a graceful release-mode fallback.Test
Fixtures cover:
{ "#{x}": 1 }— bare hash, the original failure modefoo("#{x}": 1)— keyword-arg call site (distinct caller offormat_assoc_node){ "#{x}": 1, "#{y}": 2 }— multi-entry, distinguishes a per-first-key fix from a general one{ a: 1, "#{x}": 2 }— mixed plain-shorthand and interpolated-shorthandThe existing
:"#{name}="literal coverage instring_first_item_is_embedstill passes (regression check).cargo fmt --checkandcargo clippy -D warningsare clean.Follow-on refactor
Per the suggestion to consider centralizing the mirrored strip logic, commit
4d00438extractsemit_symbol_key_closernext toformat_assoc_node. Bothformat_symbol_nodeandformat_interpolated_symbol_nodenow route through it, so thestrip_suffix(b":")pattern lives in one place with a doc comment namingformat_assoc_nodeas the owner of the:separator. No behavior change — verified by the full fixtures suite (379 tests) and an extra round-trip on the mixed-form path ({foo: 1, 'bar' => 2}→{:foo => 1, "bar" => 2}), which exercises the strip in the rocket-rendering branch whereas_symbol=false.