Skip to content

fix: attribute expressions with newlines or backslashes were compiled as string literals#197

Open
Kal-Aster wants to merge 4 commits into
riot:mainfrom
Kal-Aster:main
Open

fix: attribute expressions with newlines or backslashes were compiled as string literals#197
Kal-Aster wants to merge 4 commits into
riot:mainfrom
Kal-Aster:main

Conversation

@Kal-Aster

Copy link
Copy Markdown

DISCLAIMER

I met this bug so many times during the usage of RiotJS and it bothered me since. I always worked around it because there is a way to avoid it but it forces me to write attributes in a single line or with weird formatting and I'm quite a perfectionist about formatting >,>
I met this error today as well and I honestly used Claude to investigate it. It initially proposed a minimal fix that was functional but conceptually different from the rest of the code, so I checked how we could make the same fix while keeping the same structure of the rest of the repo and found that we could have just mirror the structure of mergeNodeExpressions with some minimal changes.

I wanted to share my process since I read that last PR on riot (3077) where AI was used in a bad and arrogant way.
That PR made me think to open a discussion about the usage of AI in this project, however this bug came first.
Let's consider to discuss it more thoroughly!

Anyway, this bug is real and the fix is working.
I'll just provide the in-depth analysis and steps produced by the AI.


The bug

An expression inside an attribute value is compiled to a plain string literal
instead of being evaluated
when it contains a newline or a backslash, as long
as there is static text next to it in the same attribute
.

<!-- broken: rendered class is the literal `card {expanded ? ...}`, unevaluated -->
<div class="card {expanded
  ? 'card--open'
  : 'card--closed'}"></div>

<!-- broken too: any backslash trips it, e.g. a regex -->
<label title="user {/^\d+$/.test(id) ? 'numeric' : 'named'}"></label>

Live reproduction: https://plnkr.co/edit/CcNWzxN313oDoB3b

A single-line expression, or a multi-line expression that is the whole value
(no surrounding static text), both work — the latter takes a different branch.

Root cause

mergeAttributeExpressions rebuilds the value from node.parts and re-associates
each part to its expression with e.text.trim() === part. But the parser stores
expression code in parts escaped (backslashes, then EOLs — this is
documented parser behaviour), while e.text stays raw. So any expression
carrying a newline or backslash fails the match and falls through to the
builders.literal(...) branch, i.e. it is emitted as a string.

The sibling text-node path (mergeNodeExpressions) doesn't have this problem: it
slices the static chunks from the source by expression offset and interleaves them
positionally, without any string matching.

The fix

Rebuild the attribute value the same way the text-node path does — a new
generateLiteralStringChunksFromAttributeNode helper slices the static chunks by
expression offset, and the default branch interleaves them with
transformExpression, dropping parts and the fragile match (and all escaping)
entirely.

Tests

Two cases added to test/generators/template.spec.js (a multi-line ternary and a
regex-bearing expression, each beside static text). Both fail on main and pass
with the fix; the full suite stays green.

…mpiled as string literals

- mergeAttributeExpressions rebuilt the value by pairing the parser's `parts`
  to their expressions via `e.text.trim() === part`, but `parts` stores the
  expression code escaped (backslashes and EOLs) while `e.text` stays raw
- any attribute with static text plus an expression containing a newline or a
  backslash therefore failed the match and was emitted as a plain string
  literal instead of being evaluated
- rebuild the value by slicing the static chunks from the source by expression
  offset and interleaving them, the way mergeNodeExpressions does for text nodes
- a multi-line ternary and a regex-bearing expression, each alongside static
  text so they hit the merge path that used to demote them to string literals
@GianlucaGuarini
GianlucaGuarini self-requested a review July 20, 2026 06:15

@GianlucaGuarini GianlucaGuarini left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you this looks like a good patch, there are small improvements that are needed otherwise we can merge it.
Regarding the other PR request closed, that was to me social engineering attack, that user is gaining access to several repos by sneaking in AI PRs

Comment thread src/generators/template/utils.js Outdated
Comment thread src/generators/template/utils.js
Kal-Aster added a commit to Kal-Aster/riot-compiler that referenced this pull request Jul 20, 2026
…mutation

Addresses review feedback on riot#197.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Kal-Aster added a commit to Kal-Aster/riot-compiler that referenced this pull request Jul 20, 2026
…romAttributeNode

Addresses review feedback on riot#197.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@GianlucaGuarini

Copy link
Copy Markdown
Member

@Kal-Aster please rebase the commit co-authored with claude. I'd like to merge commits only authored by you

@Kal-Aster

Copy link
Copy Markdown
Author

Is that fair? I actually haven't write a single line of that code, I just ensured that the produced code was matching the style of the rest of the repo, since initially it made a fix that worked but missed the general structure of the rest of the code.
However if you really want I'll remove the co-authoring right away

@GianlucaGuarini

Copy link
Copy Markdown
Member

Please remove the co-authoring. I have reviewed "your" code so that's enough, Anthropic ToS might change at any time so I prefer to protect this project from external influences.

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.

2 participants