Heavily improve wrapped pseudo selector handling

[mgreter]

Quite a hacky PR, but the logic once more escapes my imagination.
Lets see what CI thinks ... fixes #2464 and fixes #2383

Correctly compiles the following:

:host(:not(.baz1)) {
  left: 0;
}

:not(:not(.baz2)) {
  left: 0;
}

a {
  :not(:not(.baz3)) {
    left: 0;
  }
}

q {

    .thing {
      color: black;
    }
  
    .a,
    .b,
    .c,
    .d,
    .e {
      &:not(.thing) { @extend .thing; }
    }

  }
  
:host(.abc1) { x: y; }
.a {@extend .abc1}

:not(.abc2) { x: y; }
.a {@extend .abc2}

:not(.thing) {
  color: red;
}
:not(.bar) {
  @extend .thing;
  background: blue;
}

:not(.foo1):not(:not(.ext1)) {
  color: red; }

a {
  :not(.foo2):not(:not(.ext2)) {
  color: red; }
}

to

:host(:not(.baz1)) {
  left: 0; }

:not(:not(.baz2)) {
  left: 0; }

a  {
  left: 0; }

q .thing, q .a:not(.thing),
q .b:not(.thing),
q .c:not(.thing),
q .d:not(.thing),
q .e:not(.thing), q :not(.bar) {
  color: black; }

:host(.abc1, .a) {
  x: y; }

:not(.abc2):not(.a) {
  x: y; }

:not(.thing) {
  color: red; }

:not(.bar) {
  background: blue; }

:not(.foo1):not(:not(.ext1)) {
  color: red; }

a :not(.foo2) {
  color: red; }

[mgreter]

Found another interesting issue with ruby sass 3.4.23:

:not(.thing) {
  color: red;
}

:not(.bar) {
  @extend .thing;
}

foobar {
  :host(:not(.other)) {
    left: 0;
  }
}

results in

NoMethodError: undefined method `do_extend' for :not(.other):Sass::Selector::Pseudo
Did you mean?  extend
  Use --trace for backtrace.

[xzyfer]

Dart sass is now the reference implementation. It's somewhat diverged from ruby sass. We should compare against that instead from now on.

…

On 12 Nov. 2017 12:43 pm, "Marcel Greter" ***@***.***> wrote: Found another interesting issue with ruby sass 3.4.23: :not(.thing) { color: red; } :not(.bar) { @extend .thing; } foobar { :host(:not(.other)) { left: 0; } } results in NoMethodError: undefined method `do_extend' for :not(.other):Sass::Selector::Pseudo Did you mean? extend Use --trace for backtrace. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2498 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWAvmb5vBRpxGOmOf6n7DH80_xGvqks5s1k02gaJpZM4QatPp> .

[mgreter]

Dart sass is now the reference implementation. It's somewhat diverged from ruby sass. We should compare against that instead from now on.

Is there an official announcement that it is already the new reference implementation? //CC @nex3

[xzyfer]

Good question. Maybe @nex3 can shed some light on the specifics. The way I understand it, - if ruby and dart diverge in behaviour we should follow dart sass (unless it looks a bug, then file an issue with dart sass). - 3.4.x is deprecated (it's since been removed from sass-spec) so 3.5 behaviour is authoritive. - dart sass has been 3.5 from the beginning

…

On 12 Nov. 2017 12:49 pm, "Marcel Greter" ***@***.***> wrote: Dart sass is now the reference implementation. It's somewhat diverged from ruby sass. We should compare against that instead from now on. Is there an official announcement that it is already the new reference implementation? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2498 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWHgI1qDwghIuUoDb_r29reRyOXIyks5s1k63gaJpZM4QatPp> .

[mgreter]

Fair enough ... the mentioned example doesn't give any result on any ruby sass version at least on sassmeister ... no time to test locally with all ruby versions ...

[mgreter]

@xzyfer btw 3.4.6 is still missing release notes :-/

[xzyfer]

I'll talk to Stu about getting sassmiester updated. npm install dart-sass should get you the latest version. I think it's safe to replicate dart sass behaviour unless is seems broken. I'm about to jump on a plan. I can verify in a couple of hours if needed.

…

On 12 Nov. 2017 1:02 pm, "Marcel Greter" ***@***.***> wrote: Fair enough ... the mentioned example doesn't give any result on any ruby sass version at least on sassmeister ... no time to test locally with all ruby versions ... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2498 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWDeiRoES5dfkSXTJi2cKtHeGnqWPks5s1lHSgaJpZM4QatPp> .

[xzyfer]

Ok I can fix that later today. Aiming to cut a new 3.5-beta release also for node-sass.

…

On 12 Nov. 2017 1:07 pm, "Michael Mifsud" ***@***.***> wrote: I'll talk to Stu about getting sassmiester updated. npm install dart-sass should get you the latest version. I think it's safe to replicate dart sass behaviour unless is seems broken. I'm about to jump on a plan. I can verify in a couple of hours if needed. On 12 Nov. 2017 1:02 pm, "Marcel Greter" ***@***.***> wrote: > Fair enough ... the mentioned example doesn't give any result on any ruby > sass version at least on sassmeister ... no time to test locally with all > ruby versions ... > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#2498 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAjZWDeiRoES5dfkSXTJi2cKtHeGnqWPks5s1lHSgaJpZM4QatPp> > . >

[mgreter]

FYI I already verified with dart-sass and libsass output now matches, I only wanted to mention the strange behavior of ruby sass, nothing more ... looking forward to the pending release notes ...

[xzyfer]

Ok cool :-)

…

On 12 Nov. 2017 1:11 pm, "Marcel Greter" ***@***.***> wrote: FYI I already verified with dart-sass and behavior matches, I only wanted to mention the strange behavior of ruby sass, nothing more ... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2498 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWBWCZQV9p6KbPY_TrXJzftYxtcKlks5s1lO1gaJpZM4QatPp> .

[mgreter]

@nex3 here is one for a starter (ruby sass 3.4):

:not(.foo2):not(:not(.ext2)) {
  color: red;
}

foo {
  :not(.foo2):not(:not(.ext2)) {
    color: red;
  }
}

# sass test.scss
:not(.foo2):not(:not(.ext2)) {
  color: red; }
foo :not(.foo2) {
  color: red; }

# sassc test.scss
:not(.foo2):not(:not(.ext2)) {
  color: red; }
foo :not(.foo2) {
  color: red; }

# dart-sass.bat test.scss
:not(.foo2):not(:not(.ext2)) {
  color: red;
}
foo :not(.foo2):not(:not(.ext2)) {
  color: red;
}

Another one:

:not(:not(.baz1)) {
  left: 0;
}
:not(.foo) {
  @extend .baz1;
}

# sass test.scss
:not(:not(.baz1)) {
  left: 0; }

# sassc test.scss
:not(:not(.baz1)) {
  left: 0; }

# dart-sass.bat test.scss
Unexpected exception:
Invalid argument(s): components may not be empty.

[mgreter]

This PR is as good as it gets. IMO extending pseudo selectors is pretty brittle as it is. This PR implements current 3.4 behavior as closely as I can get it to work without regressing anything that we had in earlier versions. It should support the most common use cases, but I think extending wrapped selectors should be discouraged, since it is hard to predict what behavior you want/get. It is nearly impossible to debug the code-path in libsass due to the multiple conversion between Node and Selector instances ...

[xzyfer]

👍

Maybe not directly related but I thought it was worth mentioning changes to extend recently in sass sass/sass#2319.

[nex3]

Is there an official announcement that it is already the new reference implementation?

No, since the notion of a "reference implementation" isn't super relevant to most users. I will say that Dart Sass is generally more in line with our goals for the eventual behavior of Sass, though, since it makes some backwards-incompatible changes relative to Ruby Sass (see the README for details).

:not(.foo2):not(:not(.ext2)) {
  color: red;
}

foo {
  :not(.foo2):not(:not(.ext2)) {
    color: red;
  }
}

# sass test.scss
:not(.foo2):not(:not(.ext2)) {
  color: red; }
foo :not(.foo2) {
  color: red; }

# sassc test.scss
:not(.foo2):not(:not(.ext2)) {
  color: red; }
foo :not(.foo2) {
  color: red; }

# dart-sass.bat test.scss
:not(.foo2):not(:not(.ext2)) {
  color: red;
}
foo :not(.foo2):not(:not(.ext2)) {
  color: red;
}

Dart Sass is correct here. We shouldn't be simplifying user-authored styles, especially outside of the presence of @extend.

Another one:

:not(:not(.baz1)) {
  left: 0;
}
:not(.foo) {
  @extend .baz1;
}

# sass test.scss
:not(:not(.baz1)) {
  left: 0; }

# sassc test.scss
:not(:not(.baz1)) {
  left: 0; }

# dart-sass.bat test.scss
Unexpected exception:
Invalid argument(s): components may not be empty.

I've filed sass/dart-sass#191 to track this.

It's worth noting in general that our approach here should probably change soon, since :not() now allows full selector lists and Safari already implements that.