Repository navigation
Point a write of a template at the template, not its placeholder - #258
Merged
NullVoxPopuli merged 2 commits intoOct 9, 2026
Merged
Conversation
Contributor
🏎️ Benchmark ComparisonParse
Full mitata output
Full mitata output |
| // ember-tooling/ember-eslint-parser#257: scope analysis runs on the | ||
| // placeholder JS, so a write of a template kept the placeholder, which is | ||
| // not in the AST, as its writeExpr. | ||
| it.each([ |
Member
There was a problem hiding this comment.
What's a deliberate error look like?
Contributor
Author
There was a problem hiding this comment.
Added one in 67fd500: the same function now also does count += await store.fetch() on a module-level count, and the test expects exactly that report (Possible race condition: \count` might be reassigned based on an outdated value of `count`.`). So the rule still reports with templates around, rather than only not crashing.
A race where the template itself is the assigned value (Comp = <template>…</template> after an await on a stale read) still isn't reported: the rule checks assignments on :expression:exit, and :expression doesn't match GlimmerTemplate. That's in the PR description under "Not covered".
Scope analysis runs on the placeholder JS, where each template is a template literal. After the GlimmerTemplate is spliced in, a write whose value is a template (`const Foo = <template>`, an assignment, a compound or logical assignment, a default parameter) keeps the placeholder as its `writeExpr`. The placeholder is no longer in the AST, so ESLint never sets its `parent`, and require-atomic-updates crashes reading `writeExpr.parent.right` or `writeExpr.parent.operator` inside an async function. Swap those references' `writeExpr` for the template that replaced the placeholder, as esTreeNodeToTSNodeMap is already forwarded. Fixes ember-tooling#257
aklkv
force-pushed
the
fix/template-placeholder-write-expr
branch
from
October 9, 2026 19:20
67fd500 to
6edd52c
Compare
NullVoxPopuli
approved these changes
Oct 9, 2026
Merged
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
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.
Fixes #257.
ESLint's
require-atomic-updatescrashes on a template written inside an async function:Scope analysis runs on the placeholder JS, where each template is a template literal. After the
GlimmerTemplateis spliced in, a write whose value is a template keeps the placeholder as itswriteExpr: aconst/letinitializer, an assignment, a compound or logical assignment (??=,+=), or a default parameter. The placeholder is no longer in the AST, so ESLint never sets itsparent, and the rule readswriteExpr.parent.right(or.operatorfor a compound assignment).Fix
After
toTree, map each placeholder to itsGlimmerTemplatefromtemplateInfosand point any reference whosewriteExpris a placeholder at the template, the wayesTreeNodeToTSNodeMapis already forwarded. The pass runs over the scope manager's references once per parse; ontests/bench/large.gtsit takes about 1.5 µs of a ~4.7 ms parse.Tests
points a write of a template at the templaterunsrequire-atomic-updatesover a const, an assignment, a??=and a default parameter for.gjs,.gtsand.gjswithuseBabel, and checks each write reference'swriteExpris theGlimmerTemplate. All three fail without the change.test-projects/gjs/src/require-atomic-updates.gjswith the rule on for that file, and atest:check:require-atomic-updatesscript that requires it to lint clean. Onmainit crashes.Also run: the full suite, lint, the test projects, eslint-plugin-ember's suite (
scripts/eslint-plugin-ember-test.mjs) and the CI matrix's^6/^7overrides.Not covered
:expression:exit, and esquery's:expressiondoesn't matchGlimmerTemplate, soshared = <template>…</template>after anawaitgoes unreported where the same code with a string is reported.static {}block) still have the placeholder as theirblock. No rule I tried crashes on them, so they're left as they are.