Follow-up to #4462: migrate remaining @var fixtures, DRY the prelude bare-@var scan, add warnings coverage

[matthew-dean]

Follow-up to #4462 (bare-@variable at-rule deprecation). Four pieces:

1. Fixture migration — feat: deprecate bare @variable in non-value at-rule positions #4462 left two fixtures on the bare form (layer.less, import-reference.less); migrated to @{…} (byte-identical render).

2. Warnings-assertion coverage (new) — less.js asserted errors (tests-error/*.txt) but never that warnings fire, so deprecations were unguarded. Adds testWarnings() capturing logger warnings and asserting each render-reachable one, incl. the prelude semantics (bar[@v]→warns, (x:@v)→no warn). Warnings are now suppressed from normal output (LESS_TEST_SHOW_WARNINGS=1 to see them). Documented gaps: variable-in-unknown-value (only fires for an inconsistent bracket edge), property-in-unknown-value (not render-reachable), math-always/dumpLineNumbers (registered but never emitted).

3. DRY the prelude bare-@var detection — replaced the standalone hasTopLevelBareVariable() (which re-scanned text $parseUntil had already walked, with a hand-rolled ()-only counter and no string/comment handling) by folding an opt-in detectBareVar into $parseUntil's single pass (it already skips strings/comments and tracks brackets; $parseUntil has one caller). Guarded green-to-green by the new tests.

4. Lockfile repair — master's pnpm-lock.yaml had a dangling minimatch: 3.1.2 edge failing --frozen-lockfile for every PR; repinned to 3.1.5.

Full grunt test green; all 7 CI jobs pass.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Less parser fallback deprecation handling was adjusted, integration coverage was added for at-rule prelude variable warnings, and test fixtures now interpolate variables in @keyframes and @layer names.

Changes

At-rule interpolation and deprecation parsing

Layer / File(s)	Summary
Parser deprecation handling packages/less/lib/less/parser/parser.js	Removes top-level bare-variable scanning and suppresses the unknown-value variable warning when variable deprecations are enabled.
At-rule prelude warning tests packages/less/test/less-test.js, packages/less/test/index.js	Adds asynchronous warning-count assertions for at-rule preludes and runs them in the main test sequence.
Structural at-rule fixture updates packages/test-data/tests-unit/import/import/import-reference.less, packages/test-data/tests-unit/layer/layer.less	Keyframe and layer names now use Less interpolation syntax; the keyframe block formatting is expanded.

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

Possibly related PRs

• less/less.js#4462: Related parser and test changes for variable deprecation warnings in at-rule preludes.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: fixture migration, parser scan cleanup, and added warning coverage.

✨ 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 completes the bare-variable at-rule migration and adds warning coverage. The main changes are:

• Migrates the remaining @keyframes and @layer fixtures to interpolation syntax.
• Detects bare at-rule variables during the permissive parser scan.
• Adds assertions for render-reachable warnings and suppresses routine warning output.
• Repairs the dangling minimatch lockfile reference.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.

_{Reviews (8): Last reviewed commit: "refactor(parser): fold at-rule prelude b..." | Re-trigger Greptile}

[coderabbitai]

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`:
- Around line 118-132: Update hasTopLevelBareVariable to accept the incoming
parenthesis depth and return both the updated depth and whether a top-level bare
variable was found. In permissiveValue, initialize parenDepth before iterating
parsed fragments, pass it into hasTopLevelBareVariable for each unquoted item,
retain the returned depth for the next fragment, and use the returned found flag
for deprecation warnings.

🪄 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

Run ID: 6119c9cc-611f-4b83-a8c4-c4a23aa61e21

📥 Commits

Reviewing files that changed from the base of the PR and between 4d6e67e and 31ee756.

📒 Files selected for processing (11)

• packages/less/lib/less/deprecation.js
• packages/less/lib/less/parser/parser.js
• packages/test-data/tests-config/at-rules-compressed-evaluation/at-rules-compressed-evaluation.less
• packages/test-data/tests-unit/at-rule-variable-deprecated/at-rule-variable-deprecated.css
• packages/test-data/tests-unit/at-rule-variable-deprecated/at-rule-variable-deprecated.less
• packages/test-data/tests-unit/container/container.less
• packages/test-data/tests-unit/import/import/import-reference.less
• packages/test-data/tests-unit/layer/layer.less
• packages/test-data/tests-unit/media/media.less
• packages/test-data/tests-unit/permissive-parse/permissive-parse.less
• packages/test-data/tests-unit/variables-in-at-rules/variables-in-at-rules.less

[coderabbitai]

🧹 Nitpick comments (1)

packages/less/test/less-test.js (1)

841-865: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Ensure the logger listener is removed if an unexpected error occurs.

If an unexpected exception is thrown inside the test loop (e.g., from an assertion failure or an unhandled bug), the function will exit early and leave the listener attached to the global less.logger. This can pollute subsequent tests. Wrapping the logic in a try...finally block guarantees that the listener is always removed.

♻️ Proposed refactor

         less.logger.addListener(listener);
-        const isPrelude = m => /A bare `@variable` in an at-rule prelude is deprecated/.test(m);
-        const cases = [
-            ['`@bar`: x;\n@foo `@bar` { a: b }', 1, 'bare `@var` in an unknown at-rule prelude warns'],
-            ['`@a`: 1; `@b`: 2;\n@foo `@a` and `@b` { a: b }', 2, 'bare `@vars` around a keyword both warn'],
-            ['`@v`: 1px;\n@foo (x: `@v`) { a: b }', 0, '`@var` inside (...) is a declaration value — no warning'],
-            ['`@v`: x;\n@foo bar[`@v`] { a: b }', 0, '`@var` inside [...] is a lookup, not structural — no warning'],
-        ];
-        for (const [src, expected, label] of cases) {
-            totalTests++;
-            captured.length = 0;
-            try {
-                await less.render(src);
-            } catch (e) {
-                fail('- Integration - at-rule prelude `@var` warning: ' + label + ' threw ' + e + '\n');
-                continue;
-            }
-            const n = captured.filter(isPrelude).length;
-            if (n === expected) {
-                ok('- Integration - at-rule prelude `@var` warning: ' + label + ' OK\n');
-            } else {
-                fail('- Integration - at-rule prelude `@var` warning: ' + label + ' — expected ' + expected + ', got ' + n + '\n');
-            }
-        }
-        less.logger.removeListener(listener);
+        try {
+            const isPrelude = m => /A bare `@variable` in an at-rule prelude is deprecated/.test(m);
+            const cases = [
+                ['`@bar`: x;\n@foo `@bar` { a: b }', 1, 'bare `@var` in an unknown at-rule prelude warns'],
+                ['`@a`: 1; `@b`: 2;\n@foo `@a` and `@b` { a: b }', 2, 'bare `@vars` around a keyword both warn'],
+                ['`@v`: 1px;\n@foo (x: `@v`) { a: b }', 0, '`@var` inside (...) is a declaration value — no warning'],
+                ['`@v`: x;\n@foo bar[`@v`] { a: b }', 0, '`@var` inside [...] is a lookup, not structural — no warning'],
+            ];
+            for (const [src, expected, label] of cases) {
+                totalTests++;
+                captured.length = 0;
+                try {
+                    await less.render(src);
+                } catch (e) {
+                    fail('- Integration - at-rule prelude `@var` warning: ' + label + ' threw ' + e + '\n');
+                    continue;
+                }
+                const n = captured.filter(isPrelude).length;
+                if (n === expected) {
+                    ok('- Integration - at-rule prelude `@var` warning: ' + label + ' OK\n');
+                } else {
+                    fail('- Integration - at-rule prelude `@var` warning: ' + label + ' — expected ' + expected + ', got ' + n + '\n');
+                }
+            }
+        } finally {
+            less.logger.removeListener(listener);
+        }

🤖 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/test/less-test.js` around lines 841 - 865, Wrap the at-rule
prelude warning test setup and loop, beginning after
less.logger.addListener(listener), in a try...finally block, and move
less.logger.removeListener(listener) into the finally clause. Preserve the
existing test counting, rendering, and pass/fail behavior while ensuring the
listener is removed on every exit path.

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

Nitpick comments:
In `@packages/less/test/less-test.js`:
- Around line 841-865: Wrap the at-rule prelude warning test setup and loop,
beginning after less.logger.addListener(listener), in a try...finally block, and
move less.logger.removeListener(listener) into the finally clause. Preserve the
existing test counting, rendering, and pass/fail behavior while ensuring the
listener is removed on every exit path.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 84edc41c-caed-4727-917f-fc2b315264eb

📥 Commits

Reviewing files that changed from the base of the PR and between f999e46 and 14e4a6e.

📒 Files selected for processing (3)

• packages/less/lib/less/parser/parser.js
• packages/less/test/index.js
• packages/less/test/less-test.js

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/less/lib/less/parser/parser.js