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

Compilation and length issues for appended variable-length arglist #1171

Closed
davidkpiano opened this issue May 7, 2015 · 11 comments · Fixed by #1173
Closed

Compilation and length issues for appended variable-length arglist #1171

davidkpiano opened this issue May 7, 2015 · 11 comments · Fixed by #1173

Comments

@davidkpiano
Copy link

Compilation runs forever with the below code:

@function foo($initial, $args...) {
    $args: append($args, 3);

    @return nth($args, 1);
}

@debug foo(1, 2);

// EXPECTED: 2

// ACTUAL (Ruby Sass v.3.4.13): 2
// ACTUAL (Node Sass v.3.0.0 and lower): << never finishes compiling, no output >>

Also, potentially related - an appended variable-length arglist should have an accurate length given by length()

@function foo($initial, $args...) {
    $args: append($args, 3);

    @return bar($initial, $args...);
}

@function bar($args...) {
    @debug $args; // DEBUG: 1, 2, 3 
                  // (in both node-sass and Ruby Sass)

    @return length($args);
}

@debug foo(1, 2);

// EXPECTED: 3

// ACTUAL (Ruby Sass v.3.4.13): 3
// ACTUAL (Node Sass v.3.0.0 and lower): 2
@xzyfer
Copy link
Contributor

xzyfer commented May 7, 2015

Spec added sass/sass-spec#364

@xzyfer
Copy link
Contributor

xzyfer commented May 7, 2015

@davidkpiano is this part of SassDash?

@davidkpiano
Copy link
Author

@xzyfer Yep, _defaults() and _assign() (as well as other functions) rely on this behavior. 😄

@xzyfer
Copy link
Contributor

xzyfer commented May 7, 2015

Ok cool, prioritizing this to 3.2.3.

@xzyfer xzyfer added this to the 3.2.3 milestone May 7, 2015
@mgreter mgreter self-assigned this May 7, 2015
mgreter added a commit to mgreter/libsass that referenced this issue May 7, 2015
@mgreter
Copy link
Contributor

mgreter commented May 8, 2015

First issue is covered by the PR. Maybe we should create a new issue for the second, since it is related to how we handle lists. IMO the second sample can probably be shortened down to:

foo {
  test-01: inspect(( (1, 2), (3, 4) ));
}

ruby sass:

foo {
  test-01: (1, 2), (3, 4); }

libsass:

foo {
  test-01: 1, 2, 3, 4; }

Seems like we join/flatten lists too often ...

@davidkpiano
Copy link
Author

@mgreter Sure, want me to make a new issue with the second example? I feel like it might be an issue with arglists from variable-length arguments, rather than just lists (correct me if I'm wrong), hence the specific example.

@mgreter
Copy link
Contributor

mgreter commented May 8, 2015

If you can confirm the first one is gone, sure! Close this one then and create a separate issue for the list append bug (which is caused by the sample I posted above, I'm very sure about that).

Ruby sass appends 3 to this list (1, 2), which results in (1, 2), 3, which has a length of 2.
Libsass appends 3 to the list 1, 2, which results in 1, 2, 3, which has a length of 3.
If I get your comments right, this has changed in ruby sass and we probably missed that!

@davidkpiano
Copy link
Author

@mgreter Other way around. It seems that appending 3 to the arglist (1, 2) results in 1, 2, 3 for Ruby Sass and (1, 2), 3 for Libsass, or something like that.

@xzyfer
Copy link
Contributor

xzyfer commented May 13, 2015

This fix was a false positive due to sass/sass-spec#380. The spec still fails.

@xzyfer xzyfer reopened this May 13, 2015
@xzyfer xzyfer modified the milestones: 3.2.5, 3.2.3 May 13, 2015
@davidkpiano
Copy link
Author

😭

@xzyfer
Copy link
Contributor

xzyfer commented May 13, 2015

We're bummed as well. I've patched the test runner so there shouldn't be
anymore false positives.
On 14 May 2015 01:48, "David Khourshid" notifications@github.com wrote:

:(


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

@mgreter mgreter removed their assignment May 13, 2015
@xzyfer xzyfer removed this from the 3.2.5 milestone Jun 9, 2015
@xzyfer xzyfer modified the milestones: 3.2.6, 3.2.5 Jun 9, 2015
@mgreter mgreter self-assigned this Jun 12, 2015
@mgreter mgreter modified the milestones: 3.3, 3.2.6 Jun 12, 2015
@mgreter mgreter removed the Dev - WIP label Jun 12, 2015
@mgreter mgreter modified the milestones: 3.3.1, 3.3, 3.4 Jun 13, 2015
@mgreter mgreter removed their assignment Jul 27, 2015
@mgreter mgreter modified the milestones: 3.3.1, 3.4 Aug 30, 2015
@mgreter mgreter self-assigned this Aug 30, 2015
@mgreter mgreter modified the milestones: 3.3, 3.3.1 Sep 4, 2015
mgreter added a commit to mgreter/libsass that referenced this issue Sep 4, 2015
mgreter added a commit to mgreter/libsass that referenced this issue Sep 4, 2015
mgreter added a commit to mgreter/libsass that referenced this issue Sep 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants