feat: support [...] lookups in @{...} interpolation - #4488
Merged
matthew-dean merged 9 commits intoJul 30, 2026
Conversation
Backports the Less 5.x behaviour of allowing a lookup chain inside variable
interpolation, so `@{map[key]}` resolves wherever `@{name}` already does:
selectors, property names, at-rule preludes and names, quoted strings, `url()`
and `@import` paths. Chained (`@{map[a][b]}`), variable (`@{map[@k]}`),
indirect (`@{map[@@k]}`) and empty (`@{map[]}`) keys are all supported.
Previously these either failed to parse or — inside strings and `url()` —
were emitted verbatim into the output with no error and no warning.
The parser does not gain a lookup regex: `entities.variableCurly` now matches
`@{` plus the name and delegates the chain to `parsers.mixin.ruleLookups()`,
the same production the bare `@map[key]` form uses. Only the two eval-time
string paths (`Quoted`, inline JS) need a pattern, and they share one
definition rather than three hand-maintained copies — that duplication is what
let `quoted.js` drift from `lookupValue` and cause the silent pass-through.
`${...}` is deliberately left narrow. Properties have no lookup grammar
(`entities.property` parses `$name` with no chaining) and nothing can hold a
ruleset to look into, since `prop: { ... }` is a parse error in every scope.
Also fixes three related defects:
* A bare lookup in a structural position was exempt from the
`variable-in-at-rule-prelude` deprecation. `entities.variable()` parses
`@map[key]` into a `NamespaceValue`, and both warning sites tested only for
`Variable`, so `@keyframes @Map[key]`, `@supports @Map[key]` and
`@layer @Map[key]` warned for a plain `@var` but stayed silent for a lookup.
A lookup in a value position — `@supports (width: @Map[key])` — correctly
remains undeprecated.
* `@supports (width: @Map[key])` rendered as `(width: [key])`, dropping the
variable. Unknown at-rule preludes are scanned as text, so a bare lookup
reached the permissive regex, which matched only `@map` and left `[key]`
behind. The resulting condition never parsed, so the block was dead in every
browser; it now resolves.
* Interpolation inside an unquoted `url(@{path}/x.png)` was never substituted.
The body was raw text in an `Anonymous` node; text containing interpolation
is now handed to an escaped `Quoted`, matching the quoted spelling.
The `variable-in-unknown-value` notice is now tested against the text with
interpolations stripped, so a variable key inside `@{map[@key]}` is no longer
misreported as a bare use of the syntax the notice recommends adopting.
dosubot
Bot
added
the
size:XL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLess now shares lookup grammar and interpolation resolvers across parser and evaluation paths. Lookup chains work in variable interpolation, URLs, property names, quoted values, JavaScript expressions, at-rules, selectors, custom properties, and associated warning and error fixtures. ChangesLookup interpolation
CI runtime and fixture loading
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LessParser
participant InterpolatedVariable
participant AST
participant QuotedOrJsEval
LessParser->>InterpolatedVariable: parse lookup interpolation
InterpolatedVariable->>AST: create Variable or NamespaceValue
QuotedOrJsEval->>InterpolatedVariable: resolve interpolation
InterpolatedVariable->>AST: evaluate resolved node
AST-->>QuotedOrJsEval: return substituted value
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/less/lib/less/parser/parser.js`:
- Line 26: Update RULE_PROPERTY_PARTICLE so lookup chains are permitted only for
@{...} interpolations, while ${...} accepts only the lookup-free variable form.
Preserve the existing plain word/property matching and ensure
resolveInterpolatedProperty() cannot receive map[key] through the ${...} path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 57057712-f69a-4a8f-b778-56a04c1e18ec
📒 Files selected for processing (10)
packages/less/lib/less/parser/lookup-pattern.jspackages/less/lib/less/parser/parser.jspackages/less/lib/less/tree/interpolated-variable.jspackages/less/lib/less/tree/js-eval-node.jspackages/less/lib/less/tree/quoted.jspackages/less/test/less-test.jspackages/test-data/tests-unit/at-rule-variable-deprecated/at-rule-variable-deprecated.csspackages/test-data/tests-unit/at-rule-variable-deprecated/at-rule-variable-deprecated.lesspackages/test-data/tests-unit/lookup-interpolation/lookup-interpolation.csspackages/test-data/tests-unit/lookup-interpolation/lookup-interpolation.less
Addresses two review findings.
`URL.eval` escapes a rewritten rootpath only when the value is unquoted, so
wrapping an interpolated unquoted `url()` body in a `Quoted` carrying a real
quote character suppressed that escaping. With a rootpath containing `(`, `)`
or whitespace it emitted `url(a(b)/x.png)` — malformed — where the
non-interpolated spelling correctly produced `url(a\(b\)/x.png)`.
The body is now built with an empty quote string, which reads as unquoted while
still resolving interpolation. That alone was not enough: `Quoted.eval`
rebuilt its result as `this.quote + value + this.quote` and re-derived the
quote from that string, so an empty quote picked up the first character of the
substituted value and read as quoted again. The quote is now carried across
explicitly — a no-op for real quote characters, where the two already agree.
`RULE_PROPERTY_PARTICLE` shared `[@$]` across both sigils, so `${name[key]}`
was accepted as a property-name particle and reached
`resolveInterpolatedProperty` with `name[key]`, producing a misleading
undefined-property error for syntax the language does not define. The two
sigils are now spelled out separately: lookup chains for `@{...}` only.
Adds `tests-config/rootpath-escape-interpolation`, whose rootpath deliberately
contains `(`, `)` and a space so quoted and unquoted forms cannot look alike,
covering plain, interpolated and lookup bodies in both spellings.
The glob branch of `runTestSetInternal` admitted a `.less` file only when a
sibling `.css` existed. Error fixtures declare their expectation as a `.txt`
and never produce CSS, so every one of them was silently skipped — the file was
globbed, failed the existence check, and was dropped without being counted.
That covered 96 fixtures: 27 in tests-error/parse, 67 in tests-error/eval, and
the js-type-errors and no-js-errors sets. The suite reported 136 passing tests
and stayed green even when an expectation was edited to something impossible,
so parse and eval error regressions went uncaught entirely.
The check now accepts either expectation form. Verified both directions:
breaking an expectation now fails the run (exit 1 with a named FAIL), where
before it passed silently.
Re-enabling the suite surfaced two stale expectations, both pre-existing drift
rather than behaviour changes — `property-undefined` and `recursive-property`
carried a trailing blank line that `err.toString()` does not produce. Stock
4.8.1 emits the same text as this branch for both, confirming the fixtures had
drifted while dormant. Their trailing newline is corrected here.
Also adds the parse fixture for `${name[key]}`, which is now enforceable.
Test count goes from 136 to 233.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/less/lib/less/parser/parser.js (2)
131-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestrict
NamespaceValueto variable lookups.
mixin.call(true)also returnsNamespaceValuefor mixin lookups (Lines 1144-1169), so this helper incorrectly emits a “bare@variable” warning for structural mixin lookups. Check the wrapped node is aVariableCallbefore classifying it as a variable reference.Proposed fix
- return e.type === 'Variable' || e.type === 'VariableCall' || e.type === 'NamespaceValue'; + return e.type === 'Variable' || + e.type === 'VariableCall' || + (e.type === 'NamespaceValue' && e.value && e.value.type === 'VariableCall');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/less/lib/less/parser/parser.js` around lines 131 - 145, Update isBareVariableReference to classify NamespaceValue only when its wrapped node is a VariableCall, while continuing to accept direct Variable and VariableCall nodes. Do not treat NamespaceValue results from mixin.call(true) as bare variable references.
851-852: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMark lookup interpolation as a dynamic selector.
variableCurly()now returnsNamespaceValuefor@{map[key]}, butelement()still setsisVariableonly fore instanceof tree.Variable(Line 1554). The evaluator uses that flag to reparse evaluated selectors; lookup values containing combinators or compound selectors can therefore remain a single element instead of following normal interpolation behavior. (fossies.org)Proposed fix
- if (e) { return new(tree.Element)(c, e, e instanceof tree.Variable, index + currentIndex, fileInfo); } + if (e) { + const isVariable = e instanceof tree.Variable || e instanceof tree.NamespaceValue; + return new(tree.Element)(c, e, isVariable, index + currentIndex, fileInfo); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/less/lib/less/parser/parser.js` around lines 851 - 852, Update the selector element handling in element() so NamespaceValue results produced by variableCurly(), including @&`#123`;map[key]&`#125`;, set isVariable like tree.Variable results. Preserve the existing dynamic-selector reparse path so evaluated lookup values containing combinators or compound selectors are interpreted normally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/less/lib/less/parser/parser.js`:
- Around line 768-769: Update the synthetic tree.Quoted construction in the
interpolated string branch to store the parser’s currentIndex-adjusted position,
matching entities.quoted() and preserving correct diagnostics and source
locations when parseNode() uses a non-zero offset.
---
Outside diff comments:
In `@packages/less/lib/less/parser/parser.js`:
- Around line 131-145: Update isBareVariableReference to classify NamespaceValue
only when its wrapped node is a VariableCall, while continuing to accept direct
Variable and VariableCall nodes. Do not treat NamespaceValue results from
mixin.call(true) as bare variable references.
- Around line 851-852: Update the selector element handling in element() so
NamespaceValue results produced by variableCurly(), including
@&`#123`;map[key]&`#125`;, set isVariable like tree.Variable results. Preserve the
existing dynamic-selector reparse path so evaluated lookup values containing
combinators or compound selectors are interpreted normally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b5e11f4e-e62a-4005-b905-9525de46b7b9
📒 Files selected for processing (5)
packages/less/lib/less/parser/parser.jspackages/less/lib/less/tree/quoted.jspackages/test-data/tests-config/rootpath-escape-interpolation/rootpath-escape-interpolation.csspackages/test-data/tests-config/rootpath-escape-interpolation/rootpath-escape-interpolation.lesspackages/test-data/tests-config/rootpath-escape-interpolation/styles.config.cjs
`packages/less` declares `engines.node: >=18`, but the oldest CI job tracked `lts/-3`. A relative selector drifts upward as new LTS lines ship, so the declared minimum stops being exercised the moment another line reaches LTS — silently, since nothing ties the matrix to the engines field. Pins that job to `18` so the supported floor is actually tested.
`testTypeErrors` selected between two expectations with `semver.gte(process.version, 'v16.9.0')`. The two differ only in V8's wording change at 16.9 — `Cannot read property 'x' of undefined` became `Cannot read properties of undefined (reading 'x')` — so with `engines.node: >=18` the older `.txt` was unreachable on every supported runtime. The `-2` variant becomes the only expectation and the branch goes away. That leaves `semver` unused, so it is dropped from devDependencies too. Verified the fixture is genuinely enforced after the rename: editing its expectation fails the run, where the whole set was skipped entirely before the preceding commit re-enabled it.
…appings `entities.quoted()` and the `URL` node both build with `index + currentIndex`, but the synthetic `Quoted` for an interpolated unquoted body stored only the local `index`. Aligned for consistency. This is currently unobservable: `currentIndex` is non-zero only inside `parseNode`, and none of its three callers can reach `entities.url()` — two parse `['selector']`/`['selectors']`, and the third re-parses declaration values that were stored as `Anonymous`, which `anonymousValue` cannot produce for text containing `(`. The offset is correct regardless, and stops the node from being the odd one out if another caller appears. Adds a sourcemap fixture for url() values, which is the part that could have regressed: interpolated unquoted bodies now build a `Quoted` where they used to build an `Anonymous`, and the two differ in `genCSS` — `Anonymous` passes fileInfo and index to `output.add`, an escaped `Quoted` does not. Measured before adding it, the mapping structure is unchanged (an escaped `Quoted` contributes no segment of its own, and the enclosing declaration already carries the position). The fixture pins that, covering literal, interpolated, lookup and quoted-interpolated bodies side by side; the harness validates all four mappings against source.
`sourcemaps/basic` and `sourcemaps/custom-props` were skipped for the same
reason the error fixtures were: the glob branch required a sibling `.css`, and
these keep their expectation in `test/sourcemaps/*.json` via the set's
`getFilename`. Neither had run since that gate was introduced.
Generalises the previous fix rather than extending it — a fixture opts in by
declaring an expectation in any of the three supported ways: a sibling `.css`
for the default compile-and-diff, a sibling `.txt` for the error sets, or a
`getFilename` that resolves one elsewhere.
Both fixtures then failed on stale metadata, with mappings byte-identical:
- Their config had drifted to `sourceMap: true`. The harness only fills in
`sourceMapRootpath`/`sourceMapOutputFilename` when `sourceMap` is an object,
so the `testweb/` prefix the expectations were written against disappeared.
Restored to `{}`, which also matches the sibling sourcemaps-* sets.
- The expectations still carried pre-monorepo paths, missing the `tests-config/`
segment added when fixtures moved to `packages/test-data`. Regenerated; only
`sources` and `file` change, the mappings are unchanged.
`basic` validates 52 mappings and `custom-props` 1.
Two fixtures remain skipped and are left alone: sourcemaps-disable-annotation
and sourcemaps-variable-selector read `test/<name>.json`, a path layout that no
longer exists, and both are named `basic.less`, so they would collide under the
`test/sourcemaps/` convention. Fixing them needs a rename.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/less/lib/less/parser/parser.js (1)
142-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not classify every
NamespaceValueas a variable reference.
mixin.call()also returnsNamespaceValue(around Lines 1168-1169), so structural mixin lookups will now incorrectly emit the bare-variable deprecation warning. Restrict this check toNamespaceValueinstances whose wrapped value is aVariableCall.Proposed fix
function isBareVariableReference(e) { if (!e) { return false; } - return e.type === 'Variable' || e.type === 'VariableCall' || e.type === 'NamespaceValue'; + return e.type === 'Variable' || + e.type === 'VariableCall' || + (e.type === 'NamespaceValue' && e.value?.type === 'VariableCall'); }Also applies to: 1912-1914, 2347-2349
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/less/lib/less/parser/parser.js` around lines 142 - 145, Update isBareVariableReference to classify NamespaceValue only when its wrapped value is a VariableCall, while retaining direct Variable and VariableCall matches. Apply the same narrowed condition wherever this helper’s equivalent logic appears in the additional affected locations, without treating structural mixin.call() NamespaceValue results as bare variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/less/lib/less/parser/parser.js`:
- Around line 142-145: Update isBareVariableReference to classify NamespaceValue
only when its wrapped value is a VariableCall, while retaining direct Variable
and VariableCall matches. Apply the same narrowed condition wherever this
helper’s equivalent logic appears in the additional affected locations, without
treating structural mixin.call() NamespaceValue results as bare variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0944f1b1-c9a3-4207-bcbf-8e14e2ac697e
📒 Files selected for processing (5)
packages/less/lib/less/parser/parser.jspackages/less/test/sourcemaps/url-interpolation.jsonpackages/test-data/tests-config/sourcemaps/url-interpolation/styles.config.cjspackages/test-data/tests-config/sourcemaps/url-interpolation/url-interpolation.csspackages/test-data/tests-config/sourcemaps/url-interpolation/url-interpolation.less
Correcting the previous commit's note: the expectations for
sourcemaps-disable-annotation and sourcemaps-variable-selector were not missing,
and no rename was needed. They sit in their own directories under
`packages/less/test/`, so there was never a collision — I had looked in
`test/*.json` and `test/tests-config/` but not `test/sourcemaps-*/`.
The real fault was path resolution. Both verifiers read
`path.join('test/', name)`, but fixture names gained a leading `tests-config/`
when the suite moved to packages/test-data, while the expectations stayed put.
Resolution now strips that segment via a shared helper.
With the paths fixed, the gate no longer needs to special-case them: a set with
its own verifyFunction is trusted to locate and report its own expectation, so
the `.css` requirement applies only to the default compile-and-diff.
Both then failed on the same stale metadata as basic/custom-props, mappings
byte-identical: `sourcemaps-variable-selector` had drifted to `sourceMap: true`,
which skips the harness's `testweb/` defaults, and both expectations carried
pre-monorepo paths. Config restored to `{}` and expectations regenerated —
`sources` and `file` change, mappings do not.
This also picks up the sourcemaps-variable-selector `vars` fixture.
`name` is assembled from `path.relative`, so its separators are platform native. Stripping the `tests-config/` prefix with a forward-slash-only pattern left it in place on Windows, and the two fixtures looked for an expectation under a path that has never existed — reported as an empty expected value rather than a missing file. Separators are normalised before the prefix is stripped. Verified against both spellings, including the mixed form `path.relative` actually produces there (`tests-config\set/basic`).
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
1 participant
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.
What:
Backports the Less 5.x behaviour of allowing
[...]lookups inside@{...}interpolation, plus fixes found alongside it.Lookups in interpolation
@{map[key]}now resolves anywhere@{name}does — selectors, property names, at-rule preludes/names, quoted strings,url(),@importpaths. Chained@{map[a][b]}, variable@{map[@k]}, indirect@{map[@@k]}and empty@{map[]}keys all work.@map[key]already worked in value positions, and 5.x accepts the interpolated form everywhere.@media @q[tablet]warned "use@{...}", but@media @{q[tablet]}was aSyntaxError.Defects fixed
variable-in-at-rule-preludedeprecation:@map[key]parses toNamespaceValue, but both warning sites tested only forVariable. Value positions correctly stay undeprecated.@supports (width: @map[key])rendered as(width: [key]), dropping the variable. That condition is unparseable, so the block was dead in every browser; it now resolves.url(@{path}/x.png)never interpolated — its body was raw text in anAnonymousnode.Quoted.evalcouldn't round-trip an unquoted body: it re-derived the quote fromquote + value + quote, so an empty quote picked up the value's first character and read as quoted, suppressing rootpath escaping inURL.eval.The first two produced wrong CSS with no error and no warning.
Test suite
Fixtures were being globbed and then silently discarded, because the glob branch admitted a
.lessfile only when a sibling.cssexisted. Sets with their own verifyFunction keep expectations elsewhere, so they never ran. Test count 136 → 279.tests-error/parse,tests-error/eval,js-type-errors,no-js-errors) declare a.txt— 96 fixtures, none running..jsonundertest/—basic,custom-props,disable-annotation,variable-selectorand itsvars, none running. Two of the verifiers also resolved the wrong path: fixture names gained atests-config/prefix when the suite moved topackages/test-data, while the expectations stayed put.Re-enabling surfaced only stale metadata, never a behaviour change:
property-undefined/recursive-property: a trailing blank lineerr.toString()does not produce. Stock 4.8.1 emits identical text.basic/custom-props/variable-selector: configs had drifted tosourceMap: true, which skips the harness'stestweb/defaults (they only apply to an object), and expectations carried pre-monorepo paths. Configs restored to{}, expectations regenerated — mappings unchanged in every case.engines.node >=18) and the now-unusedsemverdep.New coverage:
tests-unit/lookup-interpolation, bare-lookup cases inat-rule-variable-deprecated,tests-config/rootpath-escape-interpolation(rootpath with(,)and a space, so quoted and unquoted cannot look alike),tests-config/sourcemaps/url-interpolation, and a${name[key]}parse fixture.CI
lts/-3, which drifts upward as new LTS lines ship. Pinned to the declared floor,18.Implementation
variableCurlydelegates the chain toparsers.mixin.ruleLookups(), the same production the bare form uses, so both spellings share one grammar.Quoted, inline JS) now share a single pattern definition instead of three hand-maintained copies — that drift is what caused the silent pass-through.${...}stays lookup-free: properties have no lookup grammar, andprop: { … }is a parse error in every scope.Compatibility
@{name},${prop}, bare lookups in value positions). Only intended cases changed.@supportsfix switches on feature-query blocks that never executed in any browser — may warrant a minor rather than a patch release.Checklist: