[First-Class Calc] Add an initial draft #2983

nex3 · 2020-12-31T01:18:13Z

Note that this is not expected to be the finalized proposal, and it
has a number of outstanding TODOs. However, since it's a highly
impactful proposal, I want to get a draft committed so we can start
sending it around to stakeholders, getting feedback, and iterating.

See #818

proposal/first-class-calc.md

stof · 2021-01-04T22:12:25Z

proposal/first-class-calc.md

+### Operations
+
+A calc follows the default behavior of all SassScript operations, except that it
+throws an error if used as an operand of a unary or binary `+` or `-` operation.


what about the * operator ?

The default behavior for *, / and % is to throw an error. + and - are special in that they default to converting their operands to strings.

stof · 2021-01-04T22:16:36Z

proposal/first-class-calc.md

+* Let `arguments` be the result of [simplifying](#simplifying-a-calcvalue) each
+  of `calc`'s arguments.
+
+* If `calc`'s name is `"calc"`, `arguments` must contain a single argument. If


This must here looks weird to me. It could be interpreted 2 ways:

it triggers an error if not satisfied (the usual meaning of must in a spec)

it must be valid to satisfy the if here.

From the explanation in the PR, I understand that you mean the second case. But that would be better written as a condition in the if instead.

It actually means "the parser only allows a calc with name "calc" that has one argument". I'll make that more explicit.

stof · 2021-01-04T22:17:16Z

proposal/first-class-calc.md

+
+* If `calc`'s name is `min`, `max`, or `clamp` and `arguments` are all numbers
+  whose units are mutually [compatible], return the result of calling
+  [`math.min()`], [`math.max()`], or `math.clamp()` (respectively) with those


no link for math.clamp here ? Why ?

It hasn't been added to the math spec yet.

stof · 2021-01-04T22:23:41Z

proposal/first-class-calc.md

+
+* Emit `" "`, then the operator, then `" "`.
+
+* If the operator is `"*"` or `"/"` and the right value is a `CalcOperation`


I think this is not sound when the left or right values are interpolated calcs. To be safe, parenthesis should be added in such cases (similar to the TODO you have in the simplification algorithm)

Added a TODO.

jathak

LGTM overall, though I wonder if it makes sense to refer to the data type by a different name than just calc to avoid confusion between ones named "calc" and ones with other names (even just calling the type by the full word "calculation" could make things a bit clearer).

proposal/first-class-calc.md

stof · 2021-01-06T08:43:55Z

LGTM overall, though I wonder if it makes sense to refer to the data type by a different name than just calc to avoid confusion between ones named "calc" and ones with other names (even just calling the type by the full word "calculation" could make things a bit clearer).

I tend to agree here

proposal/first-class-calc.md

Awjin · 2021-01-06T19:24:33Z

proposal/first-class-calc.md

+    > numbers of any units, and numbers across different known types (length +
+    > time) are also invalid.
+
+    > TODO: Should we try to simplify `calc(1px + 1% + 1px)`?


For here and other similar TODOs below: what are the cons of simplifying this as much as possible? It seems to me that simplifying to calc(2px + 1%) would be the most logically consistent.

This would require a more complex structure for the AST, because the 1px + 1% + 1px would be represented as CalcOperation(1px, '+', CalcOperation(1%, '+', 1px)) currently while simplifying such case would require recombining the sum to try another variant.

Alternately, it would require complexity in the simplification logic to whether CalcOperations can be "flattened" usefully. The CSS spec actually does cover this sort of simplification logic, so there's prior art, but it's definitely more complex.

Awjin · 2021-01-06T19:26:47Z

proposal/first-class-calc.md

+   return-on-investment would also be inherently limited, since we're planning
+   on gradually transitioning users away from interpolation in `calc()` anyway.
+
+2. Treating interpolation as a `CalcValue` that participates in the normal


These are formally defined below, but it could be helpful to introduce them earlier on and use the terms throughout.

I couldn't think of a good way to summarize them here, so I added a link to their formal definitions. Let me know if you have a better idea of how to describe them.

stof · 2021-01-06T20:26:48Z

proposal/first-class-calc.md

+**CalcExpression** ::= `calc(`¹ CalcArgument ')'
+**ClampExpression** ::= `clamp(`¹ CalcArgument ',' CalcArgument ',' CalcArgument ')'
+**CalcArgument**²  ::= InterpolatedDeclarationValue | CalcSum
+**CalcSum**     ::= CalcProduct (('+' | '-')³ CalcProduct)?


shouldn't the right hand side actually be CalcSum to support 1px + 1% + 1px ? the current syntax would require braces to achieve that as 1px + (1% + 1px). (making the CalcProduct a single CalcValue containing a CalcArgument enclosed in braces).

You're right that this is wrong, but I actually intended to make the second clause repeated rather than using recursion to repeat them (that's the structure that's assumed in the Semantics section below).

then the simplification algorithm needs to be updated, as it was describing only operations with 2 terms.

Actually, CalculationOperation only has left and right so the necessary change is explaining how this repeated clause gets turned into the struct.

I think it works as written, the CalcSum and CalcProduct semantics sections assemble the lists into a sequence of CalcOperations with two operands each.

proposal/first-class-calc.md

stof · 2021-01-06T20:29:17Z

proposal/first-class-calc.md

+    > numbers of any units, and numbers across different known types (length +
+    > time) are also invalid.
+
+    > TODO: Should we try to simplify `calc(1px + 1% + 1px)`?


This would require a more complex structure for the AST, because the 1px + 1% + 1px would be represented as CalcOperation(1px, '+', CalcOperation(1%, '+', 1px)) currently while simplifying such case would require recombining the sum to try another variant.

Co-authored-by: Jennifer Thakar <jathak@google.com>

Awjin

LGTM

See #818 Co-authored-by: Jennifer Thakar <jathak@google.com>

[First-Class Calc] Add an initial draft

a5ada91

See #818

nex3 force-pushed the first-class-calc branch from 765bac1 to a5ada91 Compare December 31, 2020 01:18

nex3 requested review from Awjin and jathak December 31, 2020 01:18

nex3 added 2 commits December 30, 2020 17:23

Update TOC

77e440c

Fix links

16dd8f2

stof reviewed Jan 4, 2021

View reviewed changes

Review changes

fcad934

jathak approved these changes Jan 6, 2021

View reviewed changes

proposal/first-class-calc.md Outdated Show resolved Hide resolved

proposal/first-class-calc.md Outdated Show resolved Hide resolved

proposal/first-class-calc.md Outdated Show resolved Hide resolved

Awjin reviewed Jan 6, 2021

View reviewed changes

stof reviewed Jan 6, 2021

View reviewed changes

nex3 and others added 3 commits January 8, 2021 16:26

Apply suggestions from code review

3d43f08

Co-authored-by: Jennifer Thakar <jathak@google.com>

Rename "calc" to "calculation"

c115fde

Code review changes

6dae00e

nex3 requested a review from Awjin January 9, 2021 01:02

Awjin approved these changes Jan 9, 2021

View reviewed changes

nex3 merged commit 5e3feee into master Jan 9, 2021

nex3 deleted the first-class-calc branch January 9, 2021 01:35

mirisuzanne pushed a commit that referenced this pull request Feb 10, 2022

[First-Class Calc] Add an initial draft (#2983)

053dd2d

See #818 Co-authored-by: Jennifer Thakar <jathak@google.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[First-Class Calc] Add an initial draft #2983

[First-Class Calc] Add an initial draft #2983

nex3 commented Dec 31, 2020

stof Jan 4, 2021

nex3 Jan 6, 2021

stof Jan 4, 2021

nex3 Jan 6, 2021

stof Jan 4, 2021

nex3 Jan 6, 2021

stof Jan 4, 2021

nex3 Jan 6, 2021

jathak left a comment

stof commented Jan 6, 2021

Awjin Jan 6, 2021

stof Jan 6, 2021

nex3 Jan 9, 2021

Awjin Jan 6, 2021

nex3 Jan 9, 2021

stof Jan 6, 2021 •

edited by nex3

nex3 Jan 9, 2021

stof Jan 11, 2021

stof Jan 11, 2021

nex3 Jan 11, 2021

stof Jan 6, 2021

Awjin left a comment


		* Emit `" "`, then the operator, then `" "`.

		* If the operator is `"*"` or `"/"` and the right value is a `CalcOperation`

[First-Class Calc] Add an initial draft #2983

[First-Class Calc] Add an initial draft #2983

Conversation

nex3 commented Dec 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jathak left a comment

Choose a reason for hiding this comment

stof commented Jan 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof Jan 6, 2021 • edited by nex3

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Awjin left a comment

Choose a reason for hiding this comment

stof Jan 6, 2021 •

edited by nex3