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

Make AST mutations during a walk safer #1204

Closed
romainmenke opened this issue Dec 8, 2023 · 4 comments · Fixed by #1203
Closed

Make AST mutations during a walk safer #1204

romainmenke opened this issue Dec 8, 2023 · 4 comments · Fixed by #1203
Labels
common-tools fixed Fixed but waiting for confirmation of bug reporter

Comments

@romainmenke
Copy link
Member

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.

Originally posted by @nex3 in #1202 (comment)

@romainmenke
Copy link
Member Author

romainmenke commented Dec 8, 2023

Context :

PostCSS and PostCSS-like libs have a very user friendly API but to achieve this they have a bunch of internal complexity.

This is outside my area of expertise and I also don't care that much about the UX of the parser libs. The focus and priority has always been on correctness of tokenizing and parsing and being able to keep up with the specifications.


We don't currently test for heavy mutations during AST walks.
We should at least record the current behavior and compare that to other tools.


PostCSS :

What I see in the code is that they provide methods for insert and remove operations.
In these methods they adjust the indices of all current iterators.

So adding nodes before the current item increments the index.
And removing nodes after the current index decrements the index.

I don't want to do that.
The implementation in PostCSS is brittle and has caused many bugs.


Todo :

  • gather issues as failing tests
  • define possible ways forward

@romainmenke
Copy link
Member Author

define possible ways forward

I think I found a way to always determine a next good index even when a list is mutated, without having to provide convenience methods for all mutations.

I keep a reference to the previous state of the list and use that to look for a candidate of a next element.

This seems to work and it makes mutations safe.

@romainmenke
Copy link
Member Author

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

@romainmenke romainmenke added the fixed Fixed but waiting for confirmation of bug reporter label Dec 16, 2023
@romainmenke
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common-tools fixed Fixed but waiting for confirmation of bug reporter
Projects
None yet
1 participant