Add support for multi-argument functions to function-calc-no-unspaced-operator

[romainmenke]

Which issue, if any, is this issue related to?

Closes #7618

---

@ybiquitous This rule and the implementation was a lot more complex that I originally thought it would be. Doing pure code review was tricky for me and I needed to take a deep dive.

The code in this PR is a mess, but it also isn't intended to be used as is.
It can be used, we can continue work from this. But I haven't spend much time on self-review to get it into a really good state.

As I understand it know there are multiple issues and layers.

Steps:

1. Fix units : 10px-20px -> 10px -20px (px-20px is the entire unit here)
2. Fix missing operators : 10px -20px -> 10px -|20px (| to indicate that these are separate)
3. Insert whitespace : 10px -|20px -> 10px - 20px
4. Normalize whitespace : 10px -\t20px -> 10px - 20px

The csstools tokenizer and parsing packages have several aspects that help with some of these steps.

The tokenizer is just arrays, strings and numbers. So Step 1 can be handled purely on the token stream. It is a messy function, but it gives us more control and nice separation of concerns.

The parsing algorithm walker can be stateful. This is useful to keep track of being in a math context or not e.g. calc((((10px+2px))))

In general I try to do as much work and analysis on the parsed values (e.g. token[4].value, token[4].unit, ...) and as little as possible on the string representation (e.g. token[1], node.toString())

My hope is that doing the same work from different angles is a good way to transfer some knowledge :)

Ref #7655

[changeset-bot]

⚠️ No Changeset found

Latest commit: ae8b36e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Mouvedia]

If you can't make a benchmark, do you feel confident that your new approach is faster or at least as fast as the current one?

[romainmenke]

If you can't make a benchmark, do you feel confident that your new approach is faster or at least as fast as the current one?

This change:

;node benchmark-rule.mjs function-calc-no-unspaced-operator 'true'
Warnings: 0
Mean: 92.64952612499998 ms
Deviation: 11.291622367245036 ms

Main :

;node benchmark-rule.mjs function-calc-no-unspaced-operator 'true'
Warnings: 0
Mean: 78.32598956249998 ms
Deviation: 8.242243635683067 ms

function-calc-no-unspaced-operator-bugfix :

;node benchmark-rule.mjs function-calc-no-unspaced-operator 'true'
Warnings: 0
Mean: 96.32743500000002 ms
Deviation: 9.753154793016495 ms

These results match my expectations.
This rule is doing more work than before, so it will be slightly slower.

[Mouvedia]

so it will be slightly slower

Great.
I am sure we will find some ways to gain some ms during the review.

[romainmenke]

Thank you @Mouvedia for the feedback 🙇

[Mouvedia]

I didn't check the code yet :)
Personally I don't care about the approach as long as it passes the current tests and if we are gaining something while we are losing on the perf side. The tests are looking good.
The only thing I would like is for both PRs to be part of the same version.

Feel free to switch from draft to ready if you want a proper review.

[ybiquitous]

@romainmenke Thanks a lot! I feel this PR token-based approach is much better than the current character-based approach. 👍🏼

So, as @Mouvedia commented, can you fix the conflict and make this PR ready for review? Since this PR will be merged into PR #7655, I will hand over refactoring in #7655 after merging this PR.

[ybiquitous]

Or, if you don't mind, I can take on this issue.

[romainmenke]

@ybiquitous I've made a few more edits:

• bring back support for scss interpolation
• improve coverage
• improve support for comments (e.g. calc(1px/* a comment */+ 2px))

Feel free to take this!

[Mouvedia]

I did a first quick pass; nothing major.
This is well documented except for the token positions but that can't be helped.
e.g. token[4]
If a regression slipped by I couldn't tell but I think our test suite would catch it.

[Mouvedia]

nit

Maybe add some const declarations?
e.g.

const bar = operation.secondOperand?.value[0];
const foo = bar === cssTokenizer.TokenType.Dimension || bar === cssTokenizer.TokenType.Percentage || bar === cssTokenizer.TokenType.Number;

[Mouvedia]

Can you remove the export from

stylelint/lib/reference/functions.mjs

Line 29 in f87f784

export const singleArgumentMathFunctions = new Set([

?

[romainmenke]

I've made a few edits, but will defer to @ybiquitous now for the remainder of the work on this rule :)

[ybiquitous]

@romainmenke Thank you for your work! I'll take over this. 👍🏼

[Mouvedia]

Remember to change the author in the 16.X.0 changelog.