[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 whose values require early evaluation.
  • Fixed detached ruleset variable scoping so values resolve from the defining scope, even when callers declare variables with the same names.
  • Ensured detached rulesets can access mixins defined outside their callers.

• Tests

  • Added coverage for detached ruleset scoping, including nested rulesets and caller-local variable shadows.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The Declaration constructor now preserves the wrapped value node’s evalFirst flag. Detached-ruleset fixtures cover global-variable capture, mixin scope, and nested scope, with corresponding expected CSS output.

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 scope 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, detached mixin scope, and nested detached-ruleset scope. The CSS fixture includes expected output for the added cases.

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

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: Minimal · up to 7b7c4

No actionable merge-blocking risk is established from the supplied evidence; normal test checks remain appropriate.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title "fix #2868: preserve detached ruleset definition scope" matches the main change. The PR objectives say Declaration does not read the evalFirst flag from DetachedRuleset, so the detached rule…
Linked Issues check		Issue #2868 requires detached rulesets to use values and mixins from their definition scope instead of caller-local values. Declaration now copies this.value.evalFirst, which preserves early evalu…
Out of Scope Changes check		The reviewed changes contain one parser fix and related detached-ruleset test fixtures. The trigger case and the mixin and nested-scope cases directly support issue #2868 and regression coverage. No u…
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…

Full details: Title check

Explanation

The title "fix #2868: preserve detached ruleset definition scope" matches the main change. The PR objectives say Declaration does not read the evalFirst flag from DetachedRuleset, so the detached ruleset ignores its definition scope. The change copies evalFirst in Declaration and adds detached-ruleset scope tests. The title names the issue and the behavior the fix preserves.

Full details: Linked Issues check

Explanation

Issue #2868 requires detached rulesets to use values and mixins from their definition scope instead of caller-local values. Declaration now copies this.value.evalFirst, which preserves early evaluation. The added fixtures verify the reported variable case, definition-scope mixin lookup, and nested definition scope. The expected CSS confirms global, used, definition, and global values.

Full details: Out of Scope Changes check

Explanation

The reviewed changes contain one parser fix and related detached-ruleset test fixtures. The trigger case and the mixin and nested-scope cases directly support issue #2868 and regression coverage. No unrelated change is established by the supplied file evidence.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.)

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Recent review info

Run configuration

• Source mode: Auto
• Repository: less/less.js
• Branch: master
• File: .coderabbit.yaml
• Commit: c303718
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: cbdc22c4-5bc2-43c7-b508-ffc353829f96

Commits

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

Files selected for processing: 2

• packages/test-data/tests-unit/detached-rulesets/detached-rulesets.css
• packages/test-data/tests-unit/detached-rulesets/detached-rulesets.less

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

[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.

[ShirayukiRin]

Sorry for the late update.
I've pushed the new commit that add the requested coverage.

Could you please take another look? Thank you!