fix #2868: preserve detached ruleset definition scope

[ShirayukiRin]

What:

Fix: #2868

Why:

The DetachedRuleset class has its evalFirst property set to true, but when it gets passed to the Declaration class in the parser, the Declaration class doesn't read and pass down the evalFirst property.
As a result, the evalFirst property is ignored.

Checklist:

• Documentation
• Added/updated unit tests
• Code complete

I added a unit test according to the contributiing guildelines
Link to the original doc: https://lesscss.org/features/#detached-rulesets-feature-scoping

Summary by CodeRabbit

• Bug Fixes

  • Improved evaluation of declarations when values require early evaluation.
  • Fixed detached ruleset scoping so captured global variables are resolved consistently, even when callers define variables with the same name.
  • Corrected variable-based width evaluation in detached ruleset scenarios.

• Tests

  • Added coverage for detached ruleset variable scoping and evaluation behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a01cd67e-72fd-4815-9337-f15a240134e6

📥 Commits

Reviewing files that changed from the base of the PR and between c303718 and 564979a.

📒 Files selected for processing (3)

• packages/less/lib/less/tree/declaration.js
• packages/test-data/tests-unit/detached-rulesets/detached-rulesets.css
• packages/test-data/tests-unit/detached-rulesets/detached-rulesets.less

---

📝 Walkthrough

Walkthrough

The Declaration constructor now preserves the wrapped value node’s evalFirst flag. Detached-ruleset fixtures test global-variable capture and caller-scope shadowing, with expected CSS output for the added cases.

Changes

Detached ruleset evaluation

Layer / File(s)	Summary
Preserve declaration evaluation order packages/less/lib/less/tree/declaration.js	The Declaration constructor initializes evalFirst from its value node.
Cover detached-ruleset variable scoping packages/test-data/tests-unit/detached-rulesets/detached-rulesets.less, packages/test-data/tests-unit/detached-rulesets/detached-rulesets.css	The fixtures cover global-variable capture and caller-scope shadowing. The expected CSS includes the added trigger and detached-ruleset scope rules.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies issue #2868 and the primary change: preserving detached ruleset definition scope.
Linked Issues check	✅ Passed	The code propagates evalFirst and adds tests that verify definition-scope variables are not overridden by caller variables, satisfying #2868.
Out of Scope Changes check	✅ Passed	All changes are limited to Declaration handling and detached-ruleset regression tests, which directly support #2868.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR preserves detached-ruleset definition scope by propagating the value node's early-evaluation flag to its containing declaration.

• Copies evalFirst from a declaration's value during construction.
• Adds a fixture verifying that caller-local variables do not override variables captured at detached-ruleset definition time.

Confidence Score: 5/5

The PR appears safe to merge, with no actionable correctness or security issues identified.

The propagated evaluation flag moves detached-ruleset declarations into the intended early evaluation pass while preserving the same definition-frame environment, and the added fixture covers the reported scoping behavior.

_{Reviews (1): Last reviewed commit: "fix #2868: preserve detached ruleset def..." | Re-trigger Greptile}

[matthew-dean]

@coderabbitai Any concerns / possible unexpected behaviors that this change could cause? That maybe is not covered by tests?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Changed files and patch ---'
git diff --find-renames origin/master...HEAD -- \
  packages/less/lib/less/tree/declaration.js \
  packages/test-data/tests-unit/detached-rulesets/detached-rulesets.less \
  packages/test-data/tests-unit/detached-rulesets/detached-rulesets.css || true

printf '%s\n' '--- Source outlines ---'
ast-grep outline packages/less/lib/less/tree/declaration.js --items all || true
fd -t f -a 'detached-ruleset|detached-rulesets|ruleset' packages/less/lib/less | sort

printf '%s\n' '--- evalFirst references with context ---'
rg -n -C 5 '\bevalFirst\b' packages/less/lib/less packages/test-data/tests-unit

printf '%s\n' '--- DetachedRuleset references with context ---'
rg -n -C 5 '\bDetachedRuleset\b' packages/less/lib/less packages/test-data/tests-unit

printf '%s\n' '--- Detached-ruleset fixture ---'
cat -n packages/test-data/tests-unit/detached-rulesets/detached-rulesets.less
printf '%s\n' '--- Expected CSS ---'
cat -n packages/test-data/tests-unit/detached-rulesets/detached-rulesets.css

printf '%s\n' '--- Declaration implementation ---'
cat -n packages/less/lib/less/tree/declaration.js

Length of output: 35775

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Parser declaration construction ---'
sed -n '1460,1560p' packages/less/lib/less/parser/parser.js
sed -n '1690,1760p' packages/less/lib/less/parser/parser.js

printf '%s\n' '--- Ruleset evaluation order ---'
sed -n '160,270p' packages/less/lib/less/tree/ruleset.js

printf '%s\n' '--- Variable lookup implementation ---'
ast-grep outline packages/less/lib/less/tree/variable.js --items all || true
cat -n packages/less/lib/less/tree/variable.js

printf '%s\n' '--- Mixin lookup and detached-ruleset call paths ---'
ast-grep outline packages/less/lib/less/tree/mixin-call.js --items all || true
rg -n -C 4 '\bfind\b|\bframes\b|callEval' packages/less/lib/less/tree/mixin-call.js packages/less/lib/less/tree/mixin-definition.js

printf '%s\n' '--- Relevant test fixtures ---'
rg -n -i -C 3 'definition scope|caller.*scope|scope.*caller|detached.*mixin|@.*:\s*\{' \
  packages/test-data/tests-unit \
  -g '*.less' -g '*.css' | head -n 500

printf '%s\n' '--- Linked issue ---'
gh issue view 2868 --repo less/less.js --comments

Length of output: 41690

---

@matthew-dean I do not see a clear correctness concern in the implementation. It makes Declaration participate in the existing evalFirst mechanism. That causes a detached ruleset assignment to capture its frames before later rules execute. This is consistent with MixinDefinition.

I see two useful coverage gaps:

1. Mixin lookup from the definition scope is not tested.
   The new test covers a variable only. The stated behavior also applies to mixins. Add a detached ruleset that calls a mixin defined in its definition scope, then define a same-named mixin in the caller scope. The output should use only the definition-scope mixin.

2. A nested definition scope is not tested.
   The new detached ruleset is defined at root scope. Add a case where a mixin or nested ruleset defines the detached ruleset, and a different caller invokes it. This validates that the complete captured frame stack has priority over the caller frame stack.

The only expected behavior change is intentional: a declaration whose value has evalFirst now evaluates in the early pass. At present, DetachedRuleset is the relevant value type. Therefore, a detached ruleset can no longer observe variables or mixins that exist only in the eventual caller scope. That is the required fix for #2868.

_{You are interacting with an AI system.}

[matthew-dean]

@ShirayukiRin Can you add this coverage before/after?

I see two useful coverage gaps:

Mixin lookup from the definition scope is not tested.
The new test covers a variable only. The stated behavior also applies to mixins. Add a detached ruleset that calls a mixin defined in its definition scope, then define a same-named mixin in the caller scope. The output should use only the definition-scope mixin.

A nested definition scope is not tested.
The new detached ruleset is defined at root scope. Add a case where a mixin or nested ruleset defines the detached ruleset, and a different caller invokes it. This validates that the complete captured frame stack has priority over the caller frame stack.

[ShirayukiRin]

Can you add this coverage before/after?

First, thanks for your review.

And yeah, sure! I can do that.
But I'm quite busy these days, so it might take me some time.