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

[Calc Functions] Update based on implementation feedback #3675

Merged
merged 11 commits into from
Sep 2, 2023

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Aug 30, 2023

See #3504

@nex3 nex3 requested a review from Goodwine August 30, 2023 00:59
@stof
Copy link
Contributor

stof commented Aug 30, 2023

With this new merged syntax, how is it expected to interact with the /-as-division deprecation ? In particular with the expected future state where / should be parsed as division in calculation but as a list separator outside them ?

`round($number: 1px)` should _always_ be evaluated as a function call.
@nex3
Copy link
Contributor Author

nex3 commented Aug 30, 2023

Good question. The same syntax would still parse, so the main issue is one of precedence—a SlashListExpression has lower precedence than all math functions, so a * b / c * d would parse as (a * b) / (c * d) rather than ((a * b) / c) * d. We'll need to have an algorithm for redoing the precedence when evaluating an expression as a calculation. I'll add that to the spec.

@nex3 nex3 marked this pull request as draft August 30, 2023 19:39
@nex3 nex3 marked this pull request as ready for review August 30, 2023 20:23
accepted/calc-functions.d.ts.md Outdated Show resolved Hide resolved
`"round"`, or `"abs"`; and all arguments in `call`'s `ArgumentInvocation` are
[calculation-safe], return the result of evaluating `call` [as a calculation].
`"round"`, or `"abs"`; `call`'s `ArgumentInvocation` doesn't have any
`KeywordArgument`s or `RestArgument`s; and all arguments in `call`'s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this keyword/rest condition apply to the other calc functions too? or why is it that only these four get this special behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These four get special behavior because they'll fall back to calling global Sass functions. For the others, we want keywords and rest arguments to be errors instead—if we exempted calls that included them, rest arguments would just be passed to plain CSS functions, so something like clamp(1 2 3...) would return the unquoted string "clamp(1, 2, 3)" instead of producing an error. I'll add a note to explain this.


* If `result` is not an unquoted string, throw an error.
* If `result` begins case-insensitively with `"var("`, or if `expression` is an
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently calc(1 / #{2 / 5}) results in calc(1/2/5) which evaluates to 0.1.
IIUC this change means that it'll now produce calc(1/(2/5)) which evaluates to 2.5.

Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. These are the semantics for evaluating a ParenthesizedExpression, and calc(1 / #{2 / 5}) doesn't contain a parenthesized expression. It would only produce calc(1 / (2 / 5)) if you wrote calc(1 / (#{2 / 5})).

Comment on lines 711 to 712
> For example, if we insert virtual parentheses to explicitly indicate
> precedence groupings, `(1 + 2) / (3 + 4)` becomes `1 + (2 / 3) + 4`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC the user input here is 1 + 2 / 3 + 4, but the parser reads it like slash_list(1+2, 3+4), and this process changes it to 1 + slash_list(2,3) + 4. right?

If so, I think that even though you mention "virtual parentheses" (TIL -is is singular and -es is plural!) it's confusing to see "(1 + 2) / (3 + 4) becomes 1 + (2 / 3) + 4". Like I can understand it, but it takes me a couple reading passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use slash-list notation.

@nex3 nex3 requested a review from Goodwine September 1, 2023 21:02
@nex3 nex3 changed the title [Calc Functions] Properly handle slash-separated numbers [Calc Functions] Update based on implementation feedback Sep 1, 2023
Comment on lines +580 to +583
> For calculation functions that overlap with global Sass function names, we
> want anything Sass-specific like this to end up calling the Sass function.
> For all other calculation functions, we want those constructs to throw an
> error (which they do when evaluating `call` [as a calculation]).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very helpful, thanks!

@nex3 nex3 merged commit 584e4c2 into main Sep 2, 2023
9 checks passed
@nex3 nex3 deleted the calc-functions.slash branch September 2, 2023 00:35
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.

3 participants