fix faulty source map generation with variables in selectors

[pgoldberg]

Note to reviewer: There are two potential solutions here, one that fixes the specific issue linked, and one that might be a bit more robust. Filter down to the first commit for the bandaid fix, second commit for the more robust one, third commit for added test. I recommend hiding whitespace changes in the GitHub UI – my editor removed a bunch of unnecessary trailing spaces

PR Template

What:

Fixes #3567

Source maps are pointing to the wrong files, and it seems that it points at different files on different runs, causing non-deterministic source map output.

Checklist:

• Documentation - N/A
• Added/updated unit tests
• Code complete

Why and How are included in the below description, which details the problem in the code and the proposed solutions.

Investigation and the problems

Bear with me a bit, this was my first venture into the less source code 🙂

We were seeing that we got weird source map output, mapping to files that shouldn't even appear in the source maps (e.g. reference imports from which nothing was used). After digging, there are a couple of problems that are causing this whole issue.

Background information

I was able to narrow down my search a bit by finding when this bug was introduced. It was released in 3.5.0-beta.2, and the bug was introduced in #3227.

We were seeing this happen when a selector included a variable, which directed me to this bit: https://github.com/less/less.js/pull/3227/files#diff-d8c0204835f49ae90096efe1e2d0d80868e0e6214bfd4c960a097eb20cc14ec9R67-R83

It looks like what this is doing is recreating the selector CSS with the "variableCurly" node replaced with the variable's value. Then it parses that newly created CSS, and replaces the old Selector node(s) with the new one(s). It looks like the code is trying to preserve source information by passing in the fileInfo and index of the original selectors to parseNode:

less.js/packages/less/src/less/tree/ruleset.js

Lines 85 to 86 in eefe33a

	selectors[0].getIndex(),
	selectors[0].fileInfo(),

For a little more context, my understanding is that parseNode is used for creating new nodes after the original input files are parsed.

First problem

In parseNode, the provided currentIndex and fileInfo are added to the newly created node(s) here:

less.js/packages/less/src/less/parser/parser.js

Lines 111 to 112 in eefe33a

	result._index = i + currentIndex;
	result._fileInfo = fileInfo;

These lines assume that result is a tree node. However, in some cases, result is actually an array of nodes. So when it tries to add the source info from the old selectors to the new ones, it's actually just setting _index and _fileInfo properties on the array itself, so it doesn't actually ever make it to the source maps. In this case, the parseList is ["selectors"], so result is an array of Selector tree nodes.

Second problem

Selector nodes have an elements array, containing the elements that make up the selector. When a Selector is added to the output, it actually does so by generating the CSS for each of its elements:

less.js/packages/less/src/less/tree/selector.js

Lines 135 to 138 in eefe33a

	for (i = 0; i < this.elements.length; i++) {
	element = this.elements[i];
	element.genCSS(context, output);
	}

This means that if the _fileInfo or _index for the Selector's elements is incorrect, then the output source map will be incorrect. That also means that even if the _fileInfo and _index were correctly set on the Selector(s) rather than the array containing the Selector(s), the source maps would still be incorrect on the Elements that make up the selector. The source info for the elements gets set here:

less.js/packages/less/src/less/parser/parser.js

Line 1299 in eefe33a

if (e) { return new(tree.Element)(c, e, e instanceof tree.Variable, index, fileInfo); }

There are two problems here:

1. The currentIndex passed into parseNode doesn't get added to the created Elements index
2. That fileInfo is not the fileInfo that gets passed into parseNode. It's the fileInfo that's given to the Parser when it's instantiated:

   less.js/packages/less/src/less/parser/parser.js

   Line 41 in eefe33a

   const Parser = function Parser(context, imports, fileInfo) {

The proposed solution(s)

I have two proposals for how we can solve this. One is more of a bandaid fix, and the other is (I believe) a bit more robust. Both solutions include the fix for where _fileInfo and _index get set on an array.

1. Bandaid fix by going through the elements for each selector returned by parseNode, and update the _fileInfo and add the currentIndex to the _index for each one.
2. Instead of calling this.parse.parseNode and relying on parseNode to update the _index and _fileInfo of the created nodes, create a new Parser object each time we want to call parseNode. This removes the currentIndex and fileInfo params for parseNode, and adds a currentIndex param to the Parser itself. All nodes created by the parser will add currentIndex (which defaults to 0) to the index from parserInput. This way, we can ensure that any nodes created with parseNode will have the correct source information.

The first proposed solution can be seen by filtering to the first commit, and the second proposed solution can be seen by filtering to both the first and second commits. The third commit adds a test to ensure that selectors with variables in them have properly generated source maps. I recommend hiding whitespace changes in the GitHub UI – my editor removed a bunch of unnecessary trailing spaces.

[pgoldberg]

Hey @matthew-dean and @iChenLei! If one of you gets a chance to review this sometime soon I'd really appreciate it! This will fix a pretty bothersome bug we're seeing (and it looks like others have run into it, too: #3567)

[pgoldberg]

I copied this from another source map test:

less.js/packages/less/test/less-test.js

Lines 115 to 119 in eefe33a

	fail('ERROR: ' + (err && err.message));
	if (isVerbose) {
	process.stdout.write('\n');
	process.stdout.write(err.stack + '\n');
	}

Although I think this actually refers to the wrong error object? I think this should refer to the variable e that comes from the fs.readFile callback, because err (passed as an arg to the function) is handled at the beginning of the function:

less.js/packages/less/test/less-test.js

Lines 87 to 90 in eefe33a

	if (err) {
	fail('ERROR: ' + (err && err.message));
	return;
	}

This happens in several of the tests, so I just followed the convention. If it's correct that this should, in fact, be e (and e.message/e.stack), I'm happy to follow up with a PR that fixes these references throughout this file 🙂

[matthew-dean]

Although I think this actually refers to the wrong error object?

@pgoldberg That's entirely possible. I'd like to see all of the source entirely type-checked with JSDoc annotations, because then anyone contributing would not have to guess what is what.

[pgoldberg]

Hey @matthew-dean and @lumburr, I just wanted to ping on this PR again since I think it is a somewhat major bug in less and we'd really like to be able to pick up this fix. Thank you!

[pgoldberg]

@matthew-dean @lumburr I totally understand you are probably pretty busy and that less is only lightly maintained at this point, but I would really appreciate one of you taking a look at this bugfix. Is there anything I could do to help speed up the process here/make things easier for you to review? Thank you both for maintaining!

[iChenLei]

@pgoldberg Only Matthew-Dean has the authority to release a new version for Less currently. I can review and merge, but no npm permission.

However, you can use the patch-package tool to apply your changes to Less before Matthew-Dean reviews and releases a new version.

[matthew-dean]

@iChenLei @pgoldberg Sorry y'all. Yeah, I work full time and am a single parent, so Less doesn't get a lot of love. I would love to hand over Less to a new group and can help steward that.

I don't know the implications of adding other NPM publishing rights, but I can do that.

As for this, @iChenLei do you have approval permissions for PRs? It would be nice to know that code like this has some review and sign-off.

In the meantime, my intuition @pgoldberg is that your research is correct, because Less doesn't have robust coverage around source map validation, so I'll just trust that this is more correct than what existed.

In addition, even though I'm responsible for the parseNode section that does re-parsing of some nodes post evaluation, I now feel that's mostly unnecessary and a mistake. I was able to create a minimal Less-like language that evaluates variables within selectors, and successfully re-combines it into selectors without re-parsing. (Re-parsing also doesn't cleanly separate parsing from evaluation, which makes improving / replacing the parser trickier in the future.)

In the meantime, I will approve / merge this code.

[matthew-dean]

Btw, @pgoldberg if you have any interest in being added to the collaborator list for Less, I'd be happy to add you.

[pgoldberg]

@matthew-dean Thank you!! Would it be possible to get a new release with this change?

Sorry y'all. Yeah, I work full time and am a single parent, so Less doesn't get a lot of love. I would love to hand over Less to a new group and can help steward that.

No worries – totally understand. Thank you for taking a look and merging!!

Btw, @pgoldberg if you have any interest in being added to the collaborator list for Less, I'd be happy to add you.

Thank you for the offer! I think I will probably pass just since I don't feel extremely familiar with much of the less code (although this was definitely a good introduction!) 🙂