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

Binary operators adjacent to interpolations produce incorrect output and/or errors #1728

Closed
andrew-skybound opened this issue Nov 13, 2015 · 12 comments

Comments

@andrew-skybound
Copy link

This issue is somewhat complicated so I'm first going to explain how I have observed Ruby SASS to work with respect to operators that appear adjacent to interpolation expressions, followed by the tests and results.

Background

In SASS, the behavior of binary operators changes when they are adjacent to an interoplation expression. They produce a kind of delimited string-concatenation operation, where the delimiter is the operator itself, and they cause the entire surrounding sequence of terms to become part of a single, unquoted string.

This behavior is specific to interpolation expressions; unquoted strings produced by unquote() do not operate in this way:

unquote("a")+b -> ab
#{a}+b         -> a+b
a*#{b}         -> a*b

This behavior is consistent for all numeric, comparison, and logical binary operators. But here is the interesting part: when another binary expression is concatenated to an interpolation expression in this way, the entire expression is evaluated according to operator precedence even though concatenation does not produce a numeric result. Consider these examples:

#{2}+3px*4 -> 2+12px    // right (numeric multiplication) expression is evaluated first, then concatenated
#{2}*3px+4 -> 2*3px4    // left (concatenation) expression is evaluated first, then concatenated
#{2}+3px+4 -> 2+7px     // when both operators have the same precedence, concatenation happens last
#{2}*3px*4 -> 2*12px    // same -- concatenation happens last
3px*4*#{2} -> 12px*2    // same -- concatenation happens last

Take special notice of example 3, 4, and 5: the order in which these terms are being evaluated is different than it would be if the same operators were used for a numeric expression. Sequential numeric expressions with operators of the same precedence are usually evaluated left-to-right, but here, the concatenation always happens last even when it is on the left.

Finally, whitespace around the operator is preserved in the output as either zero or one space:

#{2} +3px    -> 2 +3px
#{2}+  3px   -> 2+ 3px
#{2}   + 3px -> 2 + 3px

Tests

Here is a complete test suite that should cover everything. I have categorized the tests:

(a) tests the spacing around numeric operators when the interpolation expression is on the left
(b) tests the spacing around numeric operators when the interpolation expression is on the right
(c) tests numeric operator precedence when the interpolation expression is on the left
(d) tests numeric operator precedence when the interpolation expression is on the right
(e) tests comparison operator precedence

Logical operators (and/or) have been omitted for brevity but exhibit the same behavior.

.test {
  a01: (#{a}+5.0% + 2);
  a02: (#{a}+ 5.0% + 2);
  a03: (#{a}  +5.0% + 2);
  a04: (#{a} +  5.0% + 2);
  b01: (5 + 2.0%+#{a});
  b02: (5 + 2.0%+ #{a});
  b03: (5 + 2.0%  +#{a});
  b04: (5 + 2.0% +  #{a});
  c01: (#{a} +5.0% + 2);
  c02: (#{a} -5.0% + 2);
  c03: (#{a} /5.0% + 2);
  c04: (#{a} *5.0% + 2);
  c05: (#{a} +5.0% - 2);
  c06: (#{a} -5.0% - 2);
  c07: (#{a} /5.0% - 2);
  c08: (#{a} *5.0% - 2);
  c09: (#{a} +5.0% / 2);
  c10: (#{a} -5.0% / 2);
  c11: (#{a} /5.0% / 2);
  c12: (#{a} *5.0% / 2);
  c13: (#{a} +5.0% * 2);
  c14: (#{a} -5.0% * 2);
  c15: (#{a} /5.0% * 2);
  c16: (#{a} *5.0% * 2);
  d01: (5 + 2.0% +#{a});
  d02: (5 + 2.0% -#{a});
  d03: (5 + 2.0% /#{a});
  d04: (5 + 2.0% *#{a});
  d05: (5 - 2.0% +#{a});
  d06: (5 - 2.0% -#{a});
  d07: (5 - 2.0% /#{a});
  d08: (5 - 2.0% *#{a});
  d09: (5% / 2.0 +#{a});
  d10: (5% / 2.0 -#{a});
  d11: (5% / 2.0 /#{a});
  d12: (5% / 2.0 *#{a});
  d13: (5 * 2.0% +#{a});
  d14: (5 * 2.0% -#{a});
  d15: (5 * 2.0% /#{a});
  d16: (5 * 2.0% *#{a});
  e01: (#{a} ==5.0% == 2);
  e02: (#{a} >5.0% > 2);
  e03: (#{a} <5.0% < 2);
  e04: (#{a} >=5.0% >= 2);
  e05: (#{a} <=5.0% <= 2);
  e06: (#{a} !=5.0% != 2);
}

Ruby SASS Output: (3.4.19)

.test {
  a01: a+7%;
  a02: a+ 7%;
  a03: a +7%;
  a04: a + 7%;
  b01: 7%+a;
  b02: 7%+ a;
  b03: 7% +a;
  b04: 7% + a;
  c01: a +7%;
  c02: a -3%;
  c03: a /5%2;
  c04: a *5%2;
  c05: a +3%;
  c06: a -7%;
  c07: a /5%-2;
  c08: a *5%-2;
  c09: a +2.5%;
  c10: a -2.5%;
  c11: a /2.5%;
  c12: a *2.5%;
  c13: a +10%;
  c14: a -10%;
  c15: a /10%;
  c16: a *10%;
  d01: 7% +a;
  d02: 7% -a;
  d03: 52% /a;
  d04: 52% *a;
  d05: 3% +a;
  d06: 3% -a;
  d07: 5-2% /a;
  d08: 5-2% *a;
  d09: 2.5% +a;
  d10: 2.5% -a;
  d11: 2.5% /a;
  d12: 2.5% *a;
  d13: 10% +a;
  d14: 10% -a;
  d15: 10% /a;
  d16: 10% *a;
  e01: a ==false;
  e02: a >true;
  e03: a <false;
  e04: a >=true;
  e05: a <=false;
  e06: a !=true;
}

Now, because of the way Libsass incorrectly interprets these operators, many of these tests cause errors to occur. Here is the same test as above with the declarations that Libsass cannot yet handle commented out:

.test {
  a01: (#{a}+5.0% + 2);
  a02: (#{a}+ 5.0% + 2);
  a03: (#{a}  +5.0% + 2);
  a04: (#{a} +  5.0% + 2);
  b01: (5 + 2.0%+#{a});
  b02: (5 + 2.0%+ #{a});
  b03: (5 + 2.0%  +#{a});
  b04: (5 + 2.0% +  #{a});
  c01: (#{a} +5.0% + 2);
  c02: (#{a} -5.0% + 2);
  c03: (#{a} /5.0% + 2);
  //c04: (#{a} *5.0% + 2);
  c05: (#{a} +5.0% - 2);
  c06: (#{a} -5.0% - 2);
  c07: (#{a} /5.0% - 2);
  //c08: (#{a} *5.0% - 2);
  c09: (#{a} +5.0% / 2);
  c10: (#{a} -5.0% / 2);
  c11: (#{a} /5.0% / 2);
  //c12: (#{a} *5.0% / 2);
  c13: (#{a} +5.0% * 2);
  c14: (#{a} -5.0% * 2);
  //c15: (#{a} /5.0% * 2);
  //c16: (#{a} *5.0% * 2);
  d01: (5 + 2.0% +#{a});
  d02: (5 + 2.0% -#{a});
  d03: (5 + 2.0% /#{a});
  //d04: (5 + 2.0% *#{a});
  d05: (5 - 2.0% +#{a});
  d06: (5 - 2.0% -#{a});
  d07: (5 - 2.0% /#{a});
  //d08: (5 - 2.0% *#{a});
  d09: (5% / 2.0 +#{a});
  d10: (5% / 2.0 -#{a});
  d11: (5% / 2.0 /#{a});
  //d12: (5% / 2.0 *#{a});
  d13: (5 * 2.0% +#{a});
  d14: (5 * 2.0% -#{a});
  d15: (5 * 2.0% /#{a});
  //d16: (5 * 2.0% *#{a});
  //e01: (#{a} ==5.0% == 2);
  //e02: (#{a} >5.0% > 2);
  //e03: (#{a} <5.0% < 2);
  //e04: (#{a} >=5.0% >= 2);
  //e05: (#{a} <=5.0% <= 2);
  //e06: (#{a} !=5.0% != 2);
}

It's worth noting that the "e" tests fail beacuse of #1727.

Here is the output Libsass produces. Only c02, c10, and c14 are correct:

Libsass Output (3.2.5):

.test {
  a01: a+5.0%2;
  a02: a5%2;
  a03: a5%2;
  a04: a5%2;
  b01: 7%a;
  b02: 7%a;
  b03: 7%a;
  b04: 7%a;
  c01: a5%2;
  c02: a -3%;
  c03: a/5%2;
  c05: a5%-2;
  c06: a -7%;
  c07: a/5%-2;
  c09: a2.5%;
  c10: a -2.5%;
  c11: a/5%/2;
  c13: a10%;
  c14: a -10%;
  d01: 7%a;
  d02: 7%-a;
  d03: 52%/a;
  d05: 3%a;
  d06: 3%-a;
  d07: 5-2%/a;
  d09: 2.5%a;
  d10: 2.5%-a;
  d11: 2.5%/a;
  d13: 10%a;
  d14: 10%-a;
  d15: 10%/a;
}
@saper
Copy link
Member

saper commented Nov 13, 2015

I wonder if this is intended behaviour or just some byproduct of Ruby Sass implementation...

@xzyfer
Copy link
Contributor

xzyfer commented Nov 13, 2015

I wonder if this is intended behaviour or just some byproduct of Ruby Sass implementation...

Does this matter? We need to do what Ruby Sass does.

@xzyfer
Copy link
Contributor

xzyfer commented Nov 13, 2015

Spec added sass/sass-spec#606

@xzyfer xzyfer added this to the 3.4 milestone Nov 13, 2015
@andrew-skybound
Copy link
Author

I think @saper is correct in that it was not an intentional decision and rather a byproduct of Ruby SASS not having a clear specification.

From a language design perspective, this behavior is terrible. The output is unexpected, use cases are practically non-existant, and it means accurate syntax coloring is impossible without analyzing the AST. Perhaps an issue should be opened upstream to have it reviewed, and possibly improved as a breaking change in a future version.

Ideally the only operator that would act this way is dash, and only when there is no whitespace separating the interpolation and the identifier.

#{"margin"}-top  // no problem here
#{a}+b           // this should concatenate like two ordinary strings
#{a}*2           // this should be an error
#{a}=="a"        // this should be "true"
#{a} -2          // this should result in a list, not a single unquoted string with a space in it

Wishful thinking, perhaps.

@xzyfer
Copy link
Contributor

xzyfer commented Nov 13, 2015

It would be good to @nex3's thoughts.

@nex3
Copy link
Contributor

nex3 commented Nov 13, 2015

This was, more or less, intentional. It's an artifact of the long-ago move from explicitly invoking SassScript (e.g. color= $color) to having it always implicitly active (e.g. color: $color). Before that move, many people used interpolation as the primary way to insert variables into properties. That was purely textual interpolation, though, like for selectors, which meant that putting it near any arbitrary text was valid. In order to preserve backwards-compatibility with this as much as possible when switching to always-on SassScript, we did everything we could to make free interpolation in weird situations just work like string interpolation.

I agree that this is a gross status quo, though. This is why I filed sass/sass#1778 to track the removal of this bizarre behavior. It'll be gone in 4.0.

@andrew-skybound
Copy link
Author

@nex3 Thank you for the explanation. sass/sass#1778 looks like a move in the right direction, for sure.

@saper
Copy link
Member

saper commented Nov 15, 2015

Thank you for the explanation @nex3 ! Regarding 4.0 I've had a brief look at https://github.com/sass/sass/milestones/4.0 and I was wondering is there any chance we could have a more formal (BNF?) specification of Sass 4.0? Would simplify greatly testing and building new interpreters.

@nex3
Copy link
Contributor

nex3 commented Nov 15, 2015

Writing a full specification is a huge amount of effort. It's something I'd like to do at some point in the future, but currently design and implementation take up all my available time.

@gedkott
Copy link

gedkott commented Dec 6, 2015

Based on the comment by @andrew-skybound "#{a}+b // this should concatenate like two ordinary strings", I believe I am seeing the same problem. To start, here are the details of my environment, system, etc. to as great as detail as I can provide (let me know if there is something more I should mention):

OS X Yosemite 10.10.5
node engine v0.10.40
npm 3.5.1
gulp-sass version 2.1.0
node-sass version 3.4.2

I was seeing some odd behavior in the output of my scss files processes by node-sass.
Here is the example that is causing the problem:

// example.scss
$SOME_STRING_TO_INTERPOLATE_WITH: "JUNK";

div {
  color: #{$SOME_STRING_TO_INTERPOLATE_WITH} + '-appended-value';
}

p {
  color: $SOME_STRING_TO_INTERPOLATE_WITH + '-appended-value';
}

p.special {
  color: "#{$SOME_STRING_TO_INTERPOLATE_WITH -appended-value}";
}

$ANOTHER_VAR: #{$SOME_STRING_TO_INTERPOLATE_WITH} + '-appended-value';

div.wrong {
  color: $ANOTHER_VAR;
}

$STILL_ANOTHER_VAR: #{$SOME_STRING_TO_INTERPOLATE_WITH}+'-appended-value';

div.still-wrong {
  color: $STILL_ANOTHER_VAR;
}
//output.css
div {
  color: JUNK-appended-value; 
}

p {
  color: "JUNK-appended-value"; 
}

p.special {
  color: "JUNK -appended-value"; 
}

div.wrong {
  color: JUNK-appended-value; 
}

div.still-wrong {
  color: JUNK; 
}

The problem is that div.still-wrong was expected to have a color property of JUNK-appended-value, but it just leaves off the 'appended-value' part. I know how to fix it and that is by just placing a space between the variables, strings, and operators like in the ANOTHER_VAR example. This works, but I don't understand what is actually wrong. I have been chasing this bug for some time and just stumbled across this issue conversation. I'm still lost as to why this is an issue though. Am I seeing the same problem? If so, can anyone do me a huge favor and explain what is going on here?

@andrew-skybound
Copy link
Author

@gedkott The example you posted works just fine for me (the value of the last declaration comes out JUNK-appended-value). I tried it with Libsass 3.3.2-32-gb2a3 and Ruby SASS as well. Perhaps the version of node-sass you're using is linking an old version of Libsass.

@mgreter
Copy link
Contributor

mgreter commented Jan 17, 2016

This is fixed on latest master (specs are already activated). @saper @xzyfer IMO we should have a test in libsass-todo-tests for each issue. I don't mind adding more tests to the suite in other folders, but these tend to get overlooked when there is no test in the issues folder (as with this one).

@mgreter mgreter closed this as completed Jan 17, 2016
@mgreter mgreter modified the milestones: 3.3.3, 3.4 Jan 17, 2016
@mgreter mgreter self-assigned this Jan 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants