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

Fix and refactor delayed evaluation flag #2059

Merged
merged 2 commits into from
Apr 30, 2016

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Apr 30, 2016

Fixes #2042, #2034 and #2057 (Spec tests sass/sass-spec#856).

This is pretty much a complete rewrite of how we handle delayed values:

// Notes about delayed: some ast nodes can have delayed evaluation so
// they can preserve their original semantics if needed. This is most
// prominently exhibited by the division operation, since it is not
// only a valid operation, but also a valid css statement (i.e. for
// fonts, as in `16px/24px`). When parsing lists and expression we
// unwrap single items from lists and other operations. A nested list
// must not be delayed, only the items of the first level sometimes
// are delayed (as with argument lists). To achieve this we need to
// pass status to the list parser, so this can be set correctly.
// Another case with delayed values are colors. In compressed mode
// only processed values get compressed (other are left as written).

Issue #2042 was never fully working, but its behavior was changed with #1986 from undelayed to delayed. From my experience libsass struggled to emulate these behaviors from the beginning. First I figured out why #2042 was never working. It turns out that we have to only set delayed on certain parsed ast nodes. And whoever calls parse_list must tell it if values on the first level must be delayed (//CC @xzyfer). This is the case for declarations and argument lists (and others), but not for nested lists (with exceptions).

Once implemented pretty much all other stuff related to delayed values broke. So I set out to fix them one by one, and most were easy. The trickiest one revealed itself in the following sass:

.foo {
  eval: foobar(true, ((yellow 3) red), 1/2);
}  

This compiles to (in compressed ❗ mode):

.foo{eval:foobar(true, #ff0 3 red, 1/2)}

It shows that undelaying colors vs divisions are really two different things. Because in the sample above the color got undelayed, while the division did not. Another key code in the PR is that binary expression never get delayed when they contain nested binary expressions. With this logic in place I was able to remove most of the complicated code regarding delayed value handling.

@mgreter mgreter force-pushed the refactor/delayed-values branch from e6322a1 to 7b11296 Compare April 30, 2016 05:10
@mgreter mgreter changed the title [WIP] Fix and refactor delayed evaluation flag Fix and refactor delayed evaluation flag Apr 30, 2016
@mgreter mgreter added this to the 3.3.7 milestone Apr 30, 2016
@mgreter mgreter self-assigned this Apr 30, 2016
@mgreter mgreter force-pushed the refactor/delayed-values branch from 7b11296 to 77aa82e Compare April 30, 2016 05:40
@mgreter mgreter force-pushed the refactor/delayed-values branch 2 times, most recently from e9cb9ac to 2b66889 Compare April 30, 2016 09:26
@mgreter mgreter force-pushed the refactor/delayed-values branch from 2b66889 to 2c75e9b Compare April 30, 2016 09:48
@coveralls
Copy link

coveralls commented Apr 30, 2016

Coverage Status

Coverage increased (+0.08%) to 79.078% when pulling 2c75e9b on mgreter:refactor/delayed-values into 2f6e81a on sass:master.

@mgreter mgreter merged commit c95478c into sass:master Apr 30, 2016
@drewwells
Copy link
Contributor

👍

joefiorini added a commit to joefiorini/node-sass that referenced this pull request Sep 28, 2016
joefiorini pushed a commit to joefiorini/libsass that referenced this pull request Sep 28, 2016
Fix and refactor delayed evaluation flag
@xzyfer xzyfer modified the milestones: 3.3.7, 3.4 Oct 20, 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.

Math operations.
4 participants