Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Less root functions, lookups, anonymous mixins #135

Merged
merged 7 commits into from Aug 26, 2019

Conversation

@matthew-dean
Copy link
Contributor

commented Dec 2, 2018

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

This comment has been minimized.

Copy link

commented Dec 2, 2018

CLA assistant check
All CLA requirements met.

@matthew-dean

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

...Ping?

@octref

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

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

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

@matthew-dean

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2018

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

@octref
Copy link
Member

left a comment

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.

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));

This comment has been minimized.

Copy link
@octref

octref Jan 2, 2019

Member

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 comment has been minimized.

Copy link
@matthew-dean

matthew-dean Jan 3, 2019

Author Contributor

This is a combination of two things.

  1. 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.
  2. 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 comment has been minimized.

Copy link
@octref

octref Jan 7, 2019

Member

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.

This comment has been minimized.

Copy link
@matthew-dean

matthew-dean Jan 7, 2019

Author Contributor

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


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));

This comment has been minimized.

Copy link
@octref

octref Jan 2, 2019

Member

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() {}?

This comment has been minimized.

Copy link
@matthew-dean

matthew-dean Jan 3, 2019

Author Contributor

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().

});

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

This comment has been minimized.

Copy link
@octref

octref Jan 2, 2019

Member

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?

This comment has been minimized.

Copy link
@matthew-dean

matthew-dean Jan 3, 2019

Author Contributor

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.

}

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

This comment has been minimized.

Copy link
@octref

octref Jan 2, 2019

Member

Maybe move this to cssparser.

This comment has been minimized.

Copy link
@matthew-dean

matthew-dean Jan 3, 2019

Author Contributor

I could probably do that.

return false;
}

public _parseRegexp(regEx: RegExp): nodes.Node {

This comment has been minimized.

Copy link
@octref

octref Jan 2, 2019

Member

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


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

This comment has been minimized.

Copy link
@octref

octref Jan 2, 2019

Member

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

This comment has been minimized.

Copy link
@matthew-dean

matthew-dean Jan 3, 2019

Author Contributor

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

As in:

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

?

This comment has been minimized.

Copy link
@octref

octref Jan 7, 2019

Member

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

@@ -361,9 +488,10 @@ export class LESSParser extends cssParser.Parser {
}

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

This comment has been minimized.

Copy link
@octref

octref Jan 2, 2019

Member

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

This comment has been minimized.

Copy link
@matthew-dean

matthew-dean Jan 3, 2019

Author Contributor

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
octref approved these changes Jan 7, 2019
Copy link
Member

left a comment

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

@aeschli

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

@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 added a commit that referenced this pull request Jan 10, 2019
@aeschli

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

@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

This comment has been minimized.

Copy link
Member

commented Feb 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

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

@radium-v

This comment has been minimized.

Copy link

commented Apr 1, 2019

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.

image

@matthew-dean

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@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

This comment has been minimized.

Copy link

commented Apr 1, 2019

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

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

@kevinramharak

This comment has been minimized.

Copy link

commented Jun 1, 2019

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.

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

@matthew-dean

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@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

This comment has been minimized.

Copy link

commented Jun 5, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@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 octref modified the milestones: Backlog, July 2019, August 2019 Jul 25, 2019

@octref

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

@octref No problem!

@octref
octref approved these changes Aug 26, 2019
src/parser/lessParser.ts Outdated Show resolved Hide resolved
src/test/less/parser.test.ts Outdated Show resolved Hide resolved
@octref

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@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 octref merged commit 3df5c70 into microsoft:master Aug 26, 2019

6 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
microsoft.vscode-css-languageservice Build #20190826.5 succeeded
Details
microsoft.vscode-css-languageservice (Job linux) Job linux succeeded
Details
microsoft.vscode-css-languageservice (Job mac) Job mac succeeded
Details
microsoft.vscode-css-languageservice (Job windows) Job windows succeeded
Details
@octref

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.