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

Operator precedence is not preserved #115

Closed
coreyworrell opened this issue Sep 1, 2020 · 7 comments · Fixed by #159
Closed

Operator precedence is not preserved #115

coreyworrell opened this issue Sep 1, 2020 · 7 comments · Fixed by #159
Labels
bad-output postcss-calc produces incorrect output bug

Comments

@coreyworrell
Copy link

Source:

:root {
    --fluid-min-width: 320;
    --fluid-max-width: 1140;
    --fluid-screen: 100vw;
    --fluid-bp: calc(
        (var(--fluid-screen) - ((var(--fluid-min-width) / 16) * 1rem)) /
        ((var(--fluid-max-width) / 16) - (var(--fluid-min-width) / 16))
    );
}

7.0.4 outputs:

:root {
    --fluid-min-width: 320;
    --fluid-max-width: 1140;
    --fluid-screen: 100vw;
    --fluid-bp: calc(
        (var(--fluid-screen) - var(--fluid-min-width) / 16 * 1rem) / 
        (var(--fluid-max-width) - var(--fluid-min-width)) / 16
    );
}

The output should instead be:

:root {
    --fluid-min-width: 320;
    --fluid-max-width: 1140;
    --fluid-screen: 100vw;
    --fluid-bp: calc(
        (var(--fluid-screen) - (var(--fluid-min-width) / 16) * 1rem)) / 
        ((var(--fluid-max-width) - var(--fluid-min-width)) / 16
    );
}

postcss-calc is removing the necessary parentheses and affecting the operator precedence.

@mischnic
Copy link
Contributor

mischnic commented Sep 1, 2020

After some testing, I came up with this. But I'm not sure if it's even correct and not over engineered:

diff --git src/lib/stringifier.js src/lib/stringifier.js
index 40f92f5..c58f8a9 100644
--- src/lib/stringifier.js
+++ src/lib/stringifier.js
@@ -5,6 +5,30 @@ const order = {
   "-": 1,
 };
 
+var nonAssociative = {
+  '*': false,
+  '/': true,
+  '+': false,
+  '-': true
+}
+
+function needsParenthesis(op, childOp){
+  let opOrder = order[op];
+  let childOpOrder = order[childOp];
+  if (opOrder === childOpOrder){
+    // Chains of the same operation only need parenthesis if non-associative
+    if (op === childOp){
+      return nonAssociative[op];
+    } else {
+      // Same precedence but different operator: always
+      return true;
+    }
+  } else {
+    // Follow operator precendence
+    return order[op] < order[childOp];
+  }
+}
+
 function round(value, prec) {
   if (prec !== false) {
     const precision = Math.pow(10, prec);
@@ -19,7 +43,7 @@ function stringify(node, prec) {
       const {left, right, operator: op} = node;
       let str = "";
 
-      if (left.type === 'MathExpression' && order[op] < order[left.operator]) {
+      if (left.type === 'MathExpression' && needsParenthesis(op, left.operator)) {
         str += `(${stringify(left, prec)})`;
       } else {
         str += stringify(left, prec);
@@ -27,7 +51,7 @@ function stringify(node, prec) {
 
       str += order[op] ? ` ${node.operator} ` : node.operator;
 
-      if (right.type === 'MathExpression' && order[op] < order[right.operator]) {
+      if (right.type === 'MathExpression' && needsParenthesis(op, right.operator)) {
         str += `(${stringify(right, prec)})`;
       } else {
         str += stringify(right, prec);

This does fix your case but some other existing test cases change. For now, I don't have time to figure out why/if they should change

@alexander-akait
Copy link
Collaborator

@mischnic Can you send a PR and we will look what we break, I can help with fixing

@philwolstenholme
Copy link

philwolstenholme commented Sep 17, 2020

I think this relates to #91 too

@mischnic
Copy link
Contributor

Given that CSS variables are really just text replacements and they don't have implicit parenthesis around them (like variables in a mathematical sense), I think minification of expressions with var() need to be a lot less aggressive.

@basher
Copy link

basher commented Sep 21, 2020

I have same issue with 7.0.4 which comes with parcel@2.0.0-nightly.400.

I only spotted this today on production sites with our responsive iframe containers, where we set an inline HTML style attribute with correct iframe aspect ratio, and pass that to Sass to evaluate padding-top as per this CSS Tricks article:

<div class="responsive-media" style="--aspectRatio: 560/315;">
    <iframe width="560" height="315" ...></iframe>
</div>
.responsive-media {
    padding-top: calc(100% / (var(--aspectRatio)));
}

Output in DEV mode with Parcel start:

padding-top: calc(100% / (var(--aspectRatio)));

Output in PROD mode after with Parcel build:

padding-top: calc(100% / var(--aspectRatio));

@dangelion
Copy link

Same problem here. This bug is serious since it completely breaks calculation then the interface.
Please someone can take a look to this?
Thanks!

@basher
Copy link

basher commented Feb 19, 2021

I've given up waiting for a fix for this.

I'm now using the new standard CSS aspect-ratio property in supported browsers, and a hardcoded padding-top hack without calc() for older browsers.

@ludofischer ludofischer added bad-output postcss-calc produces incorrect output bug labels Jan 9, 2022
ludofischer added a commit that referenced this issue Jan 10, 2022
Modify the parser to track parenthesized expressions.
Keep the parentheses if a function is inside.

Fix #113
Fix #115
ludofischer added a commit that referenced this issue Jan 10, 2022
Modify the parser to track parenthesized expressions.
Keep the parentheses if a function is inside.

Fix #113
Fix #115
ludofischer added a commit that referenced this issue Jan 10, 2022
Modify the parser to track parenthesized expressions.
Keep the parentheses if a function is inside.

Fix #113
Fix #115
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bad-output postcss-calc produces incorrect output bug
Projects
None yet
7 participants