Bug: replaceComponentValues() continues iterating replaced values

[nex3]

const {tokenize} = require("@csstools/css-tokenizer"); // @csstools/css-tokenizer is a peer dependency. 
var parser = require("@csstools/css-parser-algorithms")

const value = parser.parseComponentValue(tokenize({css: "foo(bar(baz))"}));
parser.replaceComponentValues([[value]], node => {
  console.log(node);
  if (parser.FunctionNode.isFunctionNode(node) && node.getName() === 'bar') {
    return new parser.TokenNode(['ident-token', 'x', node.value[2], node.value[3], {value: 'x'}]);
  }
});

(Runkit)

In line with other AST-walking and -replacement APIs, I would expect that if I replace a parent node in the AST, the replacement algorithm will stop traversing any of that node's children. (Note that this is how PostCSS's walk() and replaceWith() interact.) Instead, replaceComponentValues() continues traversing the removed nodes. This is extra unnecessary work and can even cause data corruption if, for example, information is being stored about nodes encountered during the traversal.

[romainmenke]

Hi @nex3,

Thank you for reaching out about this.

I think the root cause is how we walk AST's.
This is used in parseComponentValue

The current walk function implementations roughly work like this :

1. iterate child nodes
2. call the walker callback for each child node
3. check if a child node is walkable
4. walk each child node

The issue you are encountering is between 2. and 4..
We don't check that the child node is still a child node of the current AST tree after 2.

I think we can trivially check in 3. that a child node is not only walkable, but also still part of the same parent node on which walk was last called.

In your case it would no longer be part of the same parent and baz would not be visited.

---

But it also introduces another question.

For this tree :

A
  - 1
  - 2
    - I
  - 9

If we replace A.2 with this tree :

3
  - III

We form :

A
  - 1
  - 3
    - II
  - 9

We can prevent visiting A.2.I with a new check, but is it expected that A.3 and/or A.3.II are visited?

There is no plugin system here, as there is in PostCSS.
So I assume that there is always only a single walker callback and that the AST being inserted is always final and doesn't need further manipulation in the same AST walk.

So I think it's fine to assume that we don't want to visit A.3 and A.3.II and that the next callback would be on A.9?

I hope this notation and naming is a bit clear and that I am not making things worse.

[nex3]

I think not visiting A.3 and A.3.II is the best behavior. I think that's what PostCSS does in a similar situation.

One thing that's worth noting: PostCSS provides strong guarantees that modifying an AST during a walk is safe by storing the index of each active iteration and updating them as necessary when replacements are made. It would be really nice if this tool did so as well, although somewhat out-of-scope for this issue.

[romainmenke]

I took a closer look at what PostCSS does and found that it does visit child nodes, even after they are replaced :

const creator = () => {
	return {
		postcssPlugin: "creator",
		Root: (root, helpers) => {
			root.walk((x) => {
				console.log(x.type);
				if (x.type === 'rule') {
					x.replaceWith(helpers.atRule({ name: 'media', params: '(min-width: 100px)' }));
				}
			});
		}
	};
};

creator.postcss = true;

.baz {
	/* A comment */
	color: green;
}

This prints

rule
comment
decl

---

And postcss-value-parser has the same behavior :

const ast = valueParser('foo(bar(baz))');
ast.walk((node) => {
	console.log(node.type, node.value);
	if (node.type === 'function' && node.value === 'bar') {
		// postcss-value-parser doesn't expose the parent during walking
		ast.nodes[0].nodes.splice(0, 1, {
			type: 'word',
			value: 'x',
		})
	}
})

function foo
function bar
word baz

---

Is it possible that this goes unnoticed because it is more common to mutate nodes with PostCSS and PostCSS-like libs?

---

That said, I think it's obvious that this is sub optimal behavior and that we should fix this.

[romainmenke]

the patch hasn't been released yet

[romainmenke]

@nex3 Patches for this issue have been released as part of 2.4.0 : https://github.com/csstools/postcss-plugins/blob/main/packages/css-parser-algorithms/CHANGELOG.md#240

Can you let us know if this resolves the bugs you encountered?

[nex3]

It'll be a bit before I can get back to trying to integrate this, but I'll keep you posted when we do.

[romainmenke]

@nex3 going to close this for now.
Please let me know if you are still facing issues with this.