Skip to content

Conversation

@jonathantneal
Copy link
Collaborator

@jonathantneal jonathantneal commented Apr 25, 2020

This fixes an issue where certain CSS functions can cause the parser to hang indefinitely.

The following CSS causes the parser to hang indefinitely.

conic-gradient()

The 1st commit in this PR addresses the immediate issue with minimal change, where the issue is a lack of support for dashes in function names.

The 2nd commit in this PR addresses the issue with a holistic approach; leveraging spec compliant regular expressions to properly capture Number and Identifier tokens. The Identifier token represents both the unit of a Numeric and the name of a Func.

A side-effect of this approach is that all code related to the allFunctions whitelist becomes obsolete and is removed.

The 3rd commit adds tests for my use-case as well as the example in #112. Therefore, I believe this PR resolves #112.

@jonathantneal jonathantneal marked this pull request as ready for review April 25, 2020 21:03
@jonathantneal jonathantneal changed the title Fix function parsing RegExp Fix Function RegExp Apr 25, 2020
@shellscape
Copy link
Owner

I'm not really a fan of this proposed change. The function names that are removed by this change were added so that we can assert that all spec compliant functions are supported. I dislike shifting that to a generic identifier.

@shellscape
Copy link
Owner

Basically I missed a hyphen on a RegExp. See 317357d#diff-a0b9e206953e658d1ff3cdaa45017a6fR126

@jonathantneal
Copy link
Collaborator Author

jonathantneal commented Apr 26, 2020

The function names that are removed by this change were added so that we can assert that all spec compliant functions are supported.

I’ve added those function names to a 4th commit of tests.

All new spec compliant functions must follow the identifier pattern matched by the regular expression in this PR, because function names are identifiers immediately followed by a left parenthesis (Citation).

It’s unfortunate to require RegExp tokenization for numbers and identifiers, but PostCSS itself is simply not designed to parse this fidelity of CSS.

@shellscape
Copy link
Owner

I'm just loathe to apply this because I'll have to assume the burden of maintaining a regexp I barely understand, especially so since there's a much simpler fix in the commit I linked to. I always greatly appreciate your contributions but I have to pass on this one. I believe it just adds too much complication that I'm not willing to inherit. This decision will no doubt be contestable, but we'll still be solving the issue at hand.

@jonathantneal
Copy link
Collaborator Author

I'll have to assume the burden of maintaining a regexp I barely understand

That makes sense to me. I have been considering that as well. You understand the "cost" of fixes, and I’m happy you’ve shared that in a way that I think others can understand. As someone who considers themselves a fellow maintainer, if that’s not too presumptuous, I support your decision.

This decision will no doubt be contestable

Yea, and that stinks. I imagine referees can nod their heads in solidarity, too. I hope folks remember that these things we make will come and go and come again, but that it’s your ability to create and share and also to garden these works that ultimately helps more folks in the longer run. Seriously, 10 years ago this might have been a contestable PR to a jQuery plugin.

we'll still be solving the issue at hand.

Thank you for that. Your patch resolves my issue.

@shellscape
Copy link
Owner

Thanks for that comment. I really do appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

postcss-values-parser hangs indefinitely

2 participants