fix(issue:4331) and keyword treated as function#4405
Closed
matthew-dean wants to merge 1 commit intoless:masterfrom
Closed
fix(issue:4331) and keyword treated as function#4405matthew-dean wants to merge 1 commit intoless:masterfrom
matthew-dean wants to merge 1 commit intoless:masterfrom
Conversation
* Fixes issue where and keyword was being treated as a function call. * Adds definition for size function call.
3 tasks
dosubot
bot
added
the
size:M
📝 WalkthroughWalkthroughIntroduces a new "size" LESS function with parser validation to prevent keyword entities from being misinterpreted as unregistered function calls, and adds test cases for media query handling with responsive breakpoints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/less/src/less/functions/size.js`:
- Around line 1-2: The import for Anonymous is wrong: currently both Variable
and Anonymous are imported from '../tree/variable', causing new Anonymous(...)
in size() to actually construct a Variable node; change the Anonymous import to
import Anonymous from '../tree/anonymous' (keeping Variable from
'../tree/variable') so size() returns an actual Anonymous AST node and
evaluation/serialization behave correctly.
- Around line 10-14: The current implementation in size() incorrectly forces
only args[0] through new Variable(...) and drops other arguments; instead
evaluate all passed nodes and serialize them, e.g. replace the single-variable
construction with evaluating every arg (use each arg's eval(this.context)), then
map those evaluated nodes to toCSS(this.context) and join them before returning
the Anonymous(`size(${...})`); reference the Variable usage, entityList/args
mapping, and the Anonymous return in size() when making the change.
In `@packages/less/src/less/parser/parser.js`:
- Around line 477-489: The fallback path in mediaFeature() that reparses a
keyword + paren (using entities.keyword(), parserInput.$re(/^[\w]+\(/) and the
functionRegistry.get check) still emits "and(" instead of "and ("; fix it by
normalizing spacing when you build the fallback node that combines the
keywordEntity and the following '(' — e.g. when creating the combined token or
text for the fallback, ensure you insert a space between the keyword text and
'(' (or otherwise set the node's raw/spacing metadata so output becomes "keyword
("), so the same spacing rule applied by the earlier guard is enforced in the
fallback path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13b82a0d-3956-4db2-89c4-7a36e5ad7b30
📒 Files selected for processing (5)
packages/less/src/less/functions/index.jspackages/less/src/less/functions/size.jspackages/less/src/less/parser/parser.jspackages/test-data/tests-unit/media/media.csspackages/test-data/tests-unit/media/media.less
Comment on lines
+1
to
+2
| import Variable from '../tree/variable'; | ||
| import Anonymous from '../tree/variable'; |
There was a problem hiding this comment.
Fix the Anonymous import.
Line 2 imports Anonymous from ../tree/variable, so new Anonymous(...) is constructing the wrong node type. That makes size() return a Variable AST node instead of an Anonymous node and will break evaluation/serialization when this path is hit.
Suggested fix
import Variable from '../tree/variable';
-import Anonymous from '../tree/variable';
+import Anonymous from '../tree/anonymous';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import Variable from '../tree/variable'; | |
| import Anonymous from '../tree/variable'; | |
| import Variable from '../tree/variable'; | |
| import Anonymous from '../tree/anonymous'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/less/src/less/functions/size.js` around lines 1 - 2, The import for
Anonymous is wrong: currently both Variable and Anonymous are imported from
'../tree/variable', causing new Anonymous(...) in size() to actually construct a
Variable node; change the Anonymous import to import Anonymous from
'../tree/anonymous' (keeping Variable from '../tree/variable') so size() returns
an actual Anonymous AST node and evaluation/serialization behave correctly.
Comment on lines
+10
to
+14
| const entityList = [new Variable(args[0].value, this.index, this.currentFileInfo).eval(this.context)]; | ||
|
|
||
| args = entityList.map(a => { return a.toCSS(this.context); }).join(this.context.compress ? ',' : ', '); | ||
|
|
||
| return new Anonymous(`size(${args})`); |
There was a problem hiding this comment.
Don't re-wrap size() args as a single variable.
Line 10 forces only args[0] through new Variable(...), and Lines 12-14 serialize only that one value. This means non-variable inputs are evaluated as variable lookups, and any extra arguments are silently dropped. Either constrain size() to one variable argument explicitly, or serialize all args as normal evaluated nodes.
Suggested direction
- const entityList = [new Variable(args[0].value, this.index, this.currentFileInfo).eval(this.context)];
-
- args = entityList.map(a => { return a.toCSS(this.context); }).join(this.context.compress ? ',' : ', ');
-
- return new Anonymous(`size(${args})`);
+ const serializedArgs = args
+ .map(arg => (arg && arg.eval ? arg.eval(this.context) : arg))
+ .map(arg => arg.toCSS(this.context))
+ .join(this.context.compress ? ',' : ', ');
+
+ return new Anonymous(`size(${serializedArgs})`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/less/src/less/functions/size.js` around lines 10 - 14, The current
implementation in size() incorrectly forces only args[0] through new
Variable(...) and drops other arguments; instead evaluate all passed nodes and
serialize them, e.g. replace the single-variable construction with evaluating
every arg (use each arg's eval(this.context)), then map those evaluated nodes to
toCSS(this.context) and join them before returning the
Anonymous(`size(${...})`); reference the Variable usage, entityList/args
mapping, and the Anonymous return in size() when making the change.
Comment on lines
477
to
+489
| parserInput.save(); | ||
|
|
||
|
|
||
| let keywordEntity = this.entities.keyword(); | ||
| parserInput.restore(); | ||
| parserInput.save(); | ||
|
|
||
| validCall = parserInput.$re(/^[\w]+\(/); | ||
| if (!validCall) { | ||
| parserInput.forget(); | ||
| return; | ||
| } else if (validCall && keywordEntity && !functionRegistry.get(validCall.substring(0, validCall.length - 1))) { | ||
| parserInput.restore(); | ||
| return; |
There was a problem hiding this comment.
This still leaves the and( formatting regression unfixed.
Lines 487-489 stop and( from being treated as a function, but the fallback path in mediaFeature() reparses it as keyword + paren and still emits and( instead of and (. The failing media test is already showing that, so this guard needs a matching spacing normalization on the fallback path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/less/src/less/parser/parser.js` around lines 477 - 489, The fallback
path in mediaFeature() that reparses a keyword + paren (using
entities.keyword(), parserInput.$re(/^[\w]+\(/) and the functionRegistry.get
check) still emits "and(" instead of "and ("; fix it by normalizing spacing when
you build the fallback node that combines the keywordEntity and the following
'(' — e.g. when creating the combined token or text for the fallback, ensure you
insert a space between the keyword text and '(' (or otherwise set the node's
raw/spacing metadata so output becomes "keyword ("), so the same spacing rule
applied by the earlier guard is enforced in the fallback path.
Member
Author
|
Closing - replacing with a cleaner implementation. The current approach has side effects on space handling (3 test failures) and uses a wasteful save/restore/save pattern. |
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
2 participants
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.
Rebased version of #4332 by @puckowski. Resolves conflicts with current master.
Original PR: #4332
Fixes: #4331
Credit: @puckowski
Summary by CodeRabbit
New Features
Bug Fixes
Tests