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

[WIP] Refactor interpolation #1738

Closed
wants to merge 3 commits into from

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Nov 15, 2015

Preview (WIP) of interpolation refactoring. Got rid of most of the escape vodoo.

Currently still fails in two main corners:

Parsed AST for issue 948:

Binary_Expression 0x26933f0 [interpolant: 0]  [delayed: 0]  (0@[0:11]-[0:13]) [mul]
 left:  Textual  [NUMBER]0x2693240 [10]
 right: String_Schema 0x2b15f30 4 [has_interpolants] <>
 right:  Textual  [NUMBER]0x26932d0 [5] [delayed]
 right:  String_Constant 0x2693360 4 (0@[0:19]-[0:21]) [px] [interpolant] <>

I guess we had a "hotfix" in place for this before. But we may just don't parse it correctly.
Edit: Seems to be related to how we handle binary_expressions, see test code below:

foo {
  bar: 10 * 5#{px};
  bar: 10 / 5#{px};
  bar: 10 + 5#{px};
  bar: 10 - 5#{px};
}

Use an intermediate list representation to create
the actual hash map in the evaluation phase.
@mgreter
Copy link
Contributor Author

mgreter commented Nov 15, 2015

Spec test I'm currently using to test binary expressions with interpolations:

foo {
  bar: 10 * 5;
  bar: 10 / 5;
  bar: 10 + 5;
  bar: 10 - 5;
}

foo {
  bar: 10 * #{5};
  bar: 10  /  #{5};
  bar: 10 + #{5};
  bar: 10 - #{5};
}

foo {
  bar: 10 * 5#{px};
  bar: 10 / 5#{px};
  bar: 10 + 5#{px};
  bar: 10 - 5#{px};
}

baz {
  baz: #{5} * 1;
  baz: #{5} / 1;
  baz: #{5} + 1;
  baz: #{5} - 1;
}

baz {
  baz: #{5px} * 1;
  baz: #{5px} / 1;
  baz: #{5px} + 1;
  baz: #{5px} - 1;
}

bar {
  width: #{1+2}+4;
  width: #{1/2}/4;
  width: #{1+2}+4;
  width: #{1-2}-4;
}

foo {
  bar: 10/5;
  bar: 10/ 5;
  bar: 10 /5;
  bar: 10 / 5;
  bar: 10  /  5;
}

div {
  baz: #{4/5}/1;
  baz: #{4/  5}/  1;
  baz: #{4  /5}  /1;
  baz: #{4  /  5}  /  1;
}

mul {
  baz: #{4*5}*1;
  baz: #{4*  5}*  1;
  baz: #{4  *5}  *1;
  baz: #{4  *  5}  *  1;
}
add {
  baz: #{4+5}+1;
  baz: #{4+  5}+  1;
  baz: #{4  +5}  +1;
  baz: #{4  +  5}  +  1;
}

sub {
  baz: #{4-5}-1;
  baz: #{4-  5}-  1;
  baz: #{4  -5}  -1;
  baz: #{4  -  5}  -  1;
}

Expected result:

foo {
  bar: 50;
  bar: 10 / 5;
  bar: 15;
  bar: 5; }

foo {
  bar: 10 * 5;
  bar: 10 / 5;
  bar: 10 + 5;
  bar: 10 - 5; }

foo {
  bar: 50px;
  bar: 10/5px;
  bar: 15px;
  bar: 5px; }

baz {
  baz: 5 * 1;
  baz: 5 / 1;
  baz: 5 + 1;
  baz: 5 - 1; }

baz {
  baz: 5px * 1;
  baz: 5px / 1;
  baz: 5px + 1;
  baz: 5px - 1; }

bar {
  width: 3+4;
  width: 1/2/4;
  width: 3+4;
  width: -1-4; }

foo {
  bar: 10/5;
  bar: 10/ 5;
  bar: 10 /5;
  bar: 10 / 5;
  bar: 10  /  5; }

div {
  baz: 4/5/1;
  baz: 4/5/ 1;
  baz: 4/5 /1;
  baz: 4/5 / 1; }

mul {
  baz: 20*1;
  baz: 20* 1;
  baz: 20 *1;
  baz: 20 * 1; }

add {
  baz: 9+1;
  baz: 9+ 1;
  baz: 9 +1;
  baz: 9 + 1; }

sub {
  baz: -1-1;
  baz: -1- 1;
  baz: 4 -5 -1;
  baz: -1 - 1; }

What this WIP currently produces:

foo {
  bar: 50;
  bar: 10 / 5;
  bar: 15;
  bar: 5; }

foo {
  bar: 10 * 5;
  bar: 10 / 5;
  bar: 10 + 5;
  bar: 10 - 5; }

foo {
  bar: 50px;
  bar: 10/5px;
  bar: 15px;
  bar: 5px; }

baz {
  baz: 5 * 1;
  baz: 5 / 1;
  baz: 5 + 1;
  baz: 5 - 1; }

baz {
  baz: 5px * 1;
  baz: 5px / 1;
  baz: 5px + 1;
  baz: 5px - 1; }

bar {
  width: 3+4;
  width: 1/2 / 4;
  width: 3+4;
  width: -1-4; }

foo {
  bar: 10/5;
  bar: 10/ 5;
  bar: 10 /5;
  bar: 10 / 5;
  bar: 10  /  5; }

div {
  baz: 4/5 / 1;
  baz: 4/5 / 1;
  baz: 4/5 / 1;
  baz: 4/5 / 1; }

mul {
  baz: 20 * 1;
  baz: 20 * 1;
  baz: 20 * 1;
  baz: 20 * 1; }

add {
  baz: 9+1;
  baz: 9 1;
  baz: 9 + 1;
  baz: 9 + 1; }

sub {
  baz: -1-1;
  baz: -1- 1;
  baz: 4 -5 -1;
  baz: -1 - 1; }

As far as I can see this now would only differ in whitespace, but I had to do some very crude checks to get this behavior. @xzyfer IMO you said you want to takle this too? It seems that binary expressions somehow remember the whitespace around operators. IMO this needs to be addressed first before I could clean up the binary expression part of this refactoring.

@saper
Copy link
Member

saper commented Nov 15, 2015

Libsass is also not accepting asterisk glued to numbers and interpolations, maybe this is related?

@xzyfer
Copy link
Contributor

xzyfer commented Nov 15, 2015

Part of this code will be addressed with my static value refactor which
will introduce a basic sass script parser. Doing so will reduce the scope
of string schema which is at the root of our interpolation issues.
On 15 Nov 2015 5:21 am, "Marcin Cieślak" [email protected] wrote:

Libsass is also not accepting asterisk glued to numbers and
interpolations, maybe this is related?


Reply to this email directly or view it on GitHub
#1738 (comment).

@mgreter
Copy link
Contributor Author

mgreter commented Nov 15, 2015

@xzyfer so will your refactor solve all the issues in the sample I posted above?
Also is there a WIP PR where I can check in what direction you're going with that?

@xzyfer
Copy link
Contributor

xzyfer commented Nov 15, 2015

I'm not planning to directly address them. My work is focusing on improving
how we parse declaration values. This means some cases where we currently
use string schema will go away potentially improving these issues as a
consequence.

As far as my approach I'm simply changing our parser to better match the
Ruby implementation.
On 15 Nov 2015 9:40 am, "Marcel Greter" [email protected] wrote:

@xzyfer https:/xzyfer so will your refactor solve all the
issues in the sample I posted above?
Also is there a WIP PR where I can check in what direction you're going
with that?


Reply to this email directly or view it on GitHub
#1738 (comment).

@mgreter mgreter closed this Jan 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants