Support for Less root functions, lookups, anonymous mixins by matthew-dean · Pull Request #135 · microsoft/vscode-css-languageservice

matthew-dean · 2018-12-02T02:55:32Z

This fixes a number of errors in the language service parser for CSS/Less (that are not covered in microsoft/vscode#43087).

In Less, functions can be called anywhere to create a node, including any stylesheet or ruleset root. Fixed.
Custom functions can accept mixin-like arguments (such as accepting rulesets), and arguments can be separated with a semi-colon. Fixed.
Adds support for map lookups @ruleset[@lookup] or #ns.mixin(@arg)[prop]
Adds support for property references ($prop and interpolated ${prop})
Less is more forgiving of property and variable names than the CSS parser. This is particularly important after Less 3.5 which allowed the use of rulesets as maps, so declarations may be values like 100: 100 or even 10px: true if someone wanted to get crazy*. The CSS parser parses them as any number of token types (number or dimension), so there's a regex parsing function added in the Less parser to be more forgiving of property names.
Adds support for anonymous mixins, introduced with the each() function
Added if(), boolean, range(), and each() functions to Less completions
Fixes nested at-rule children of nested at-rules not being marked as nested... (Note: in Less, there are more at-rules than @supports and @media that bubble, but I notice the core CSS parser only supports those two types as being nested. Not sure if that's worth addressing or not.)
Fixes spelling for nestedProperties
Fixes detached ruleset declarations (assignment to a variable) no longer needing to terminate in a semi-colon (can end in } like any other block)
Fixes the disparity between mixin body and detached ruleset body. They support the same node types. Not sure why that was originally parsed differently here.
Fixes @supports ( as being parsed as a VariableCall. Checks for whitespace.

* The declaration of 100 as a property name is not crazy; it's actually a real-world use case of a conversion of Bootstrap 4 maps to Less maps. See: https://github.com/seanCodes/bootstrap-less-port/blob/35390325fad4e23e037396dd19081a57d4189579/less/_variables.less#L24

msftclas · 2018-12-02T02:55:43Z

All CLA requirements met.

matthew-dean · 2018-12-02T08:06:44Z

Just to note: I tested this through just the regular (and updated) tests on this repo. I haven't tried this in a local development build of VSCode.

matthew-dean · 2018-12-03T03:10:16Z

One thing that's still kind of outstanding is that because rulesets can be maps in Less, the warning and squiggly lines about "unknown property"s is somewhat useless. Is there a way for those unknown property flags to not be set in a Less document? I think just coloring the keyword differently would be enough, but the squiggly line is quite obnoxious. (That would be true for CSS in general IMO. It's okay to visually distinguish known and unknown, but I wouldn't hit the user over the head.)

matthew-dean · 2018-12-07T05:49:17Z

...Ping?

octref · 2018-12-16T16:29:48Z

Sorry, @aeschli was on vacation and so was I. I'll review it in December.

matthew-dean · 2018-12-16T23:55:43Z

@octref Ok no worries, just wanted to make sure it didn't get lost!

octref

Note: in Less, there are more at-rules than @supports and @media that bubble, but I notice the core CSS parser only supports those two types as being nested. Not sure if that's worth addressing or not.

That's fine for now.

Overall looks good, just a few questions to understand the less features being added.

octref · 2019-01-02T18:43:44Z

src/test/less/parser.test.ts

+		assertNode('$color', parser, parser._parseVariable.bind(parser));
+		assertNode('$$color', parser, parser._parseVariable.bind(parser));
+		assertNode('@$color', parser, parser._parseVariable.bind(parser));
+		assertNode('$@color', parser, parser._parseVariable.bind(parser));


What are these? I don't know much about less, but from http://lesscss.org/features I understand these usages:

@color: #fff; .foo { color: @color; background-color: $color; }

Can you give me an example that uses @$, $@ and $$?

This is a combination of two things.

Less supports variables and (like PHP) variable variables. Meaning, the name (identifier) of a variable being referenced can itself be variable. So: @@var means "return the value of a variable with the id that @var resolves to. If @var is foo then @foo is looked up/returned.

In a 2.x release, support for property referencing ($prop) was added. Basically it just means that properties are (more or less) variables.

Well it's not explicitly documented, you can technically then combine these features of variable variables, where the property identifier being looked up is variable, or the variable id being looked up is variable based on a property name.

Why someone would ever do this, I don't know, it's just a normal side-effect of combining these features.

This doesn't compile with less 3.9:

@color: #fff; @lookup: 'color'; .foo { color: @@lookup; background-color: $@lookup; }

I guess we can add more less-specific language features in the future. Now as long as VS Code doesn't fail at parsing these less files, users can get by.

@octref You're right that that usage may not compile, but this does:

@dr: { prop: value; } .box { @lookup: prop; foo: @dr[$@lookup]; }

For lookup values, I think I did the VSCode parsing to use any of the variable reference form. So, yeah, it may not compile, but VSCode should still not throw an unrecoverable parsing error.

It's also possible the code you wrote will work at some point, just for consistency.

octref · 2019-01-02T19:10:53Z

src/test/less/parser.test.ts

+
+		assertNode('func(a, b; bar)', parser, parser._parseRuleSetDeclaration.bind(parser));
+		assertNode('func({a: b();}, bar)', parser, parser._parseRuleSetDeclaration.bind(parser));
+		assertNode('func(.(@val) {})', parser, parser._parseRuleSetDeclaration.bind(parser));


Is this allowed?

This in lessc 3.9 gives me errors:

func(@x) { @x(); } .foo { func({a: b();}); }

less-test > lessc test.less ParseError: Unrecognised input in /Users/octref/Code/work/less-test/test.less on line 12, column 10: 11 12 func(@x) { 13 @x();

I thought functions only start with dot, like .func() {}?

You're thinking of mixins. Mixins start with . or #, and those are defined in your stylesheet. Functions are defined in JavaScript. They accept a node (or nodes), and return a node. lighten() and darken() are examples of Less functions. After Less made it easier for users to define functions using @plugin, the restriction on functions being only in a property's value was dropped. This way, authors can call a function that returns a ruleset, for example. Less also later added functions that are meant to be called around rules, such as each().

octref · 2019-01-02T19:21:06Z

src/test/less/parser.test.ts

 	test('Interpolation', function () {
 		let parser = new LESSParser();
 		assertNode('.@{name} { }', parser, parser._parseRuleset.bind(parser));
+		assertNode('.${name} { }', parser, parser._parseRuleset.bind(parser));


I believe this is "Adds support for property references ($prop and interpolated ${prop})" you mentioned, but I couldn't find any such example on less doc. Can you give an example?

Similar to what's mentioned above, this is just a normal side-effect of treating properties as variables. That is, you can do interpolation in other values. Would this particular syntax in the above assertion ever make sense? Not really. But it's valid Less, which should be what the parser is checking for.

octref · 2019-01-02T19:45:03Z

src/parser/lessParser.ts

 	}

-	public _acceptInterpolatedIdent(node: nodes.Node): boolean {
+	public acceptRegexp(regEx: RegExp): boolean {


Maybe move this to cssparser.

I could probably do that.

octref · 2019-01-02T19:45:39Z

src/parser/lessParser.ts

+		return false;
+	}
+
+	public _parseRegexp(regEx: RegExp): nodes.Node {


Same as above, seems useful enough that we can have it in cssparser.

octref · 2019-01-02T19:47:28Z

src/parser/lessParser.ts

+
+		while (accept() || 
+			node.addChild(this._parseInterpolation() || 
+			this.try(delimWithInterpolation))) {


You can the conditions in the same line. It's hard to read what's inside the while because of no indentation.

Fair enough. Or maybe for really long conditions, move them all to an indented form?

As in:

while( condition() || condition2() ) { }

?

Yeah that's better. this.try and hasContent on the same indent level was confusing to me.

octref · 2019-01-02T19:50:39Z

src/parser/lessParser.ts

 	public _parseInterpolation(): nodes.Node {
-		//  @{name}
+		// @{name} Variable or 
+		// ${name} Property


Same as the other ${prop} confusion. Can you point me to the docs that describe this feature?

The documentation is very brief and doesn't cover the interpolated form. It's here: http://lesscss.org/features/#variables-feature-properties-as-variables-new-

It should probably be added to Less docs, but it's valid. In a lot of cases for these, I pulled code directly from Less unit tests and added them to the language service tests. In fact, I opened Less unit tests in VSCode to see what was failing first, before adding language service tests.

octref

Just move parse/accept regex to base parser and this should be good to go. Thanks!

aeschli · 2019-01-09T16:46:04Z

@matthew-dean Thanks a lot for your help! We're happy to have your expertise!

Most of the changes are good. To (hopefully) make it simpler to work on this, I already pushed some of the uncontroversial changes, see 5e7a590

What I don't like so much are changes related to the map lookups. The lookup is modelled as a child to variables and mixins. I think it is cleaner if we do it in the term, like an an operator (e.g. the % operator).

matthew-dean · 2019-01-09T19:37:49Z

@aeschli

What I don't like so much are changes related to the map lookups. The lookup is modelled as a child to variables and mixins. I think it is cleaner if we do it in the term, like an an operator (e.g. the % operator).

I'm unclear what you're saying...?

aeschli · 2019-01-10T10:08:38Z

@matthew-dean My suggestion would be to add the support to parseTerm. Something like:

	public _parseTerm(): nodes.Term {
		let term = this._parseSimpleTerm();
		if (term && this.accept(TokenType.BracketL)) {
			// TODO: check for name, index of variable maybe even just `term`?
			if (!this.accept(TokenType.BracketR)) {
				//return error ...
			}
		}
		return term;
	}


	public _parseSimpleTerm(): nodes.Term {
		let term = super._parseTerm();
		if (term) { return term; }

		term = <nodes.Term>this.create(nodes.Term);
		if (term.setExpression(this._parseVariable()) ||
			term.setExpression(this._parseEscaped()) ||
			term.setExpression(this._tryParseMixinReference())) {

			return <nodes.Term>this.finish(term);
		}

		return null;
	}

That allows indexed access of every term which is probably more than necessary, but would keep the variables and mixins as is.

octref · 2019-02-02T03:55:23Z

Although some of the changes are already in, we'll put the changes on release notes for next month when we merge the whole PR.

matthew-dean · 2019-02-05T01:35:28Z

Sorry, will try to get back to this soon and the requested changes when I can find some time.

radium-v · 2019-04-01T18:51:31Z

I would love to see this moved further through the pipeline. Broken map lookups cause every proceeding colon, bracket, and curly brace to be presented as an error. It's really frustrating.

matthew-dean · 2019-04-01T19:37:25Z

@radium-v Would you have time to fix conflicts and address the few changes requested? I've just got a full schedule at work + parenting.

radium-v · 2019-04-01T19:54:08Z

@matthew-dean sure, I'll dive in this week.

kevinramharak · 2019-06-01T17:43:28Z

Sooo, is anyone still on this? As it has missed its milestones and I would love to see this implemented but do not have the skills to finish this.

matthew-dean · 2019-06-03T17:45:09Z

@kevinramharak I apologize. As someone who coordinates open source work, I know it's frustrating when a PR seems so close but languishes in changes. I just haven't had the extra cycles for any development on the side. Hopefully @radium-v can get to it? On a long enough timeline, I should be able to return to a pile of Less maintenance stuff, which includes this, but I don't yet have a slot in my schedule.

radium-v · 2019-06-05T14:35:21Z

@kevinramharak @matthew-dean I also apologize, I overpromised when I volunteered to pick this up. I do still intend to pick it up and run it to the finish line, This is a blocker for radium-v/Better-Less#14 and I'm finally able to get back to it.

matthew-dean · 2019-07-25T02:04:41Z

@octref @aeschli

I've merged master and moved methods according to feedback. However, I found that with even the base-level merge I was getting unrelated errors on SCSS (which was logging console errors but not reporting as errors to the test runner?) and an error with a property in the CSS actions test.

Can you take a look?

octref · 2019-07-26T23:57:07Z

It's too late in our development week for July, but I'll try to review and make sure this goes in for August. Thanks again for your work!

matthew-dean · 2019-07-27T00:42:19Z

@octref No problem!

src/parser/lessParser.ts

src/test/less/parser.test.ts

octref · 2019-08-26T21:50:42Z

@matthew-dean Thanks a lot for your work! Can I ask you what's the less version this supports up-to? If I remember correctly it's 3.9.0 and there's no syntax change in 3.10.0 as far as I can tell.

I'll start to link issues fixed by your PR. Thanks again 👍 🎉

octref · 2019-08-26T22:40:52Z

There's probably a few bugs that was fixed in 5e7a590 #135 (comment) that I failed to link to. Thanks again for bringing the less support up-to-date 👍

matthew-dean · 2019-08-26T23:54:37Z

@octref Less 3.10 was a minor version bump just because I converted the Less.js project to ES2015 modules, and as a result, was more than just a bugfix? (Not really sure how that fits into Semver lol.) But to answer your question, no, there have been no syntax changes and there are none currently planned. The last major ones were the "rulesets-as-maps" feature, and the ability to call functions on any node, which this should cover.

Support for Less root functions, lookups, anonymous mixins

ffddc4b

vscodebot bot assigned aeschli Dec 2, 2018

matthew-dean added 2 commits December 1, 2018 23:57

Fixes for lookups and interpolation

73e6009

Updated Less functions

2aa11e6

This was referenced Dec 2, 2018

Refactor using native each(), bump Less version seanCodes/bootstrap-less-port#12

Closed

[Up For Grabs] Improve Less parser for VSCode and other IDEs less/less.js#3277

Closed

Fixes for detached rulesets and nested at-rules

ed0c2d3

octref added this to the December 2018 milestone Dec 16, 2018

octref suggested changes Jan 2, 2019

View reviewed changes

kevinramharak mentioned this pull request Jan 4, 2019

[less] css-varvalueexpected when using more complicated variable assignment statements #141

Closed

octref approved these changes Jan 7, 2019

View reviewed changes

aeschli pushed a commit that referenced this pull request Jan 10, 2019

support for functions & properties. part of PR #135, pushed by aeschli

389f082

octref modified the milestones: December/January 2019, February 2019 Feb 12, 2019

octref modified the milestones: February 2019, March 2019 Feb 24, 2019

octref modified the milestones: March 2019, April 2019 Apr 3, 2019

aeschli modified the milestones: April 2019, Backlog Jun 3, 2019

Merge master and update from feedback

1b5f8e7

octref modified the milestones: Backlog, July 2019, August 2019 Jul 25, 2019

Merge remote-tracking branch 'origin/master' into pr/matthew-dean/135

5c6c548

octref approved these changes Aug 26, 2019

View reviewed changes

src/parser/lessParser.ts Outdated Show resolved Hide resolved

src/test/less/parser.test.ts Outdated Show resolved Hide resolved

💄

6e66ab9

octref merged commit 3df5c70 into microsoft:master Aug 26, 2019

This was referenced Aug 26, 2019

[less] linter issue with digit-starting properties microsoft/vscode#67399

Closed

Syntex: Less v3.5.0 Property / value accessors issue microsoft/vscode#66763

Closed

LessCss 3.5 feature "using mixins as functions" not fully supported microsoft/vscode#65832

Closed

Conversation

matthew-dean commented Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthew-dean commented Dec 2, 2018

Uh oh!

matthew-dean commented Dec 3, 2018

Uh oh!

matthew-dean commented Dec 7, 2018

Uh oh!

octref commented Dec 16, 2018

Uh oh!

matthew-dean commented Dec 16, 2018

Uh oh!

octref left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octref left a comment

Choose a reason for hiding this comment

Uh oh!

aeschli commented Jan 9, 2019

Uh oh!

matthew-dean commented Jan 9, 2019

Uh oh!

aeschli commented Jan 10, 2019

Uh oh!

octref commented Feb 2, 2019

Uh oh!

matthew-dean commented Feb 5, 2019

Uh oh!

radium-v commented Apr 1, 2019

Uh oh!

matthew-dean commented Apr 1, 2019

Uh oh!

radium-v commented Apr 1, 2019

Uh oh!

kevinramharak commented Jun 1, 2019

Uh oh!

matthew-dean commented Jun 3, 2019

Uh oh!

radium-v commented Jun 5, 2019

Uh oh!

matthew-dean commented Jul 25, 2019

Uh oh!

octref commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

matthew-dean commented Dec 2, 2018 •

edited

Loading

msftclas commented Dec 2, 2018 •

edited

Loading

octref commented Jul 26, 2019 •

edited

Loading

matthew-dean commented Aug 26, 2019 •

edited

Loading