Refactoring / back-porting selectors and extend

[mgreter]

Needs sass/sass-spec#1396

Continued from #2904.

I know this is a big commit, and it took quite some effort to get this ready
for review. Positive thing is that it removes more lines than it adds 😄

Feel free to poke at the changes. I know there are still a lot of loose ends.
Some stuff is just there to really make it work. I tried hard to clean this up
to at least have a better state than before. But maybe don't be too picky.
There are over 7000 lines of code. Easy to nitpick, but I would like to get
this merged at some point, so please focus on the big picture. As always
we can adjust and polish spots in the future.

Main features

• Ports extend logic back from dart-sass
• Gets rid of old extend and node classes
• Ports superselector logic back from dart-sass
• Overhauls internal selector storage structure
• Refactors media rules to match dart-sass closer
• Simplifies compare operations for selectors a bit
• Adds initial implementation for min/max specificity
• Improves selector schema parsing and evaluation
• Implements custom insertion ordered hash map
• Cleans header includes for files touched
• Tons of other small fixes and improvements

New selector structure (equal to dart sass)

• A SelectorList is an array of ComplexSelector
• A ComplexSelector is an array of SelectorComponent
  • Between each SelectorComponent is a space
• A SelectorComponent is a base class for either
  • A SelectorCombinator is either >, + or ~
  • A CompoundSelector in an array of Simple_Selector
    • Need to follow some rules (e.g. only one ID_Selector)
    • Are joined without any space in between.

I removed Parent_Selector, but there is still the Parent_Refernce. The reference
is only used in interpolations. Actual selector now get rid of all the fake parent. The
logic is that a parent selector can only occur at the begining of a CompoundSelector.
Therefore this class has a flag hasRealParent that indicates that we parsed a real parent!
reference on the input. Implicit connections (previously fake parents) are now indicated
by the chroots flag on the ComplexSelector. If a selector is chrooted, it should not
connect implicitly to its parent (hasRealParent might still play a role here).

Known spots for improvement

• The min/max specificity for selectors has only be partially implements yet.
• Had to add an originalSelectorStack in order to support parent resolving
  like dart sass does. Dart sass stores both items in an object, which we lack.
• Parser for interpolated media queries is now overly complex. We have two
  classes only to store this interpolation. Find or create/adjust a parser that
  parses exactly the schema we need for this interpolation. For now it works.
• It should be questioned in general if schemas/interpolation should be stored
  by the target node (e.g. SelectorList vs Ruleset). It IMO makes more sense
  to store the Selector_Schema for a Ruleset on hat Ruleset. Once it is
  evaled the selector object can be set. This should solve the disambiguate from
  storing a Selector_Schema as a base Selector.

Known spots to develop further

• Added new TO_CSS output target. Currently it takes care to throw an
  error if something css incompatible is being printed (e.g. invalid units).
  ToDo: we might should move more error into this code path (TBD)
• Added new CssMediaRule class. On extend we directly work with
  media-queries and need them to be evaluated and parsed beforehand.
  Dart-sass seems to have certain ast-nodes only for static css syntax. This
  is not only useful when parsing pure css, once nodes are evaluated, their
  result should match that of any static css. If we ever want to support
  a pure css parser, we probably need to adopt this route anyway!

[mgreter]

Had to throw out some facy template functions for dart sass "compatibility".
Restored the more generic ones, they also work for our use cases.

[xzyfer]

By porting over dart sass' extend behaviour does this mean loose extending of compound selectors? I believe this feature was deprecated and maybe even removes from dart sass' entirely. This is important to know given its a breaking change and we haven't had a deprecation period yet.

[xzyfer]

See #2418 and #2419

[mgreter]

Thanks, good point. It seems that spec tests for this have gone missing?

• https://github.com/sass/sass-spec/tree/3.4/spec/extend-tests/092_test_long_extendee
• https://github.com/sass/sass-spec/tree/3.4/spec/libsass-closed-issues/issue_1654/nested

Should be simple enough to get the old behavior back though. Will try to come up with a fix.

[glebm]

Reviewing this piecemeal. Not the big picture, which I don't fully understand yet, but C++ stuff.

[glebm]

I'm not a big fan of ObjPtrEquality* and PtrObjEquality*. They are very similar and confusing.

As I mentioned before, I don't think (Obj)?Ptr(Equality|Hash)(Fn)? functions and structs should exist: they are the default comparison / hash functions, and having them is confusing because the reader thinks they are custom / non-default ones. It is also confusing because we now have very similarly named PtrObj functions / structs.

[glebm]

Easy to nitpick, but I would like to get this merged at some point, so please focus on the big picture. As always we can adjust and polish spots in the future.

Please, let's properly clean up before submitting: it is easier to clean up at review time than later.

[mgreter]

I'm not a big fan of ObjPtrEquality* and PtrObjEquality*. They are very similar and confusing.
As I mentioned before, I don't think (Obj)?Ptr(Equality|Hash)(Fn)? functions and structs should exist: they are the default comparison / hash functions, and having them is confusing because the reader thinks they are custom / non-default ones. It is also confusing because we now have very similarly named PtrObj functions / structs.

The problem is we don't always want the same behavior on every hash. Sometimes I want the hash to have unique pointers and sometimes I want the hash to have unique objects. To do this I need to pass the functor structs to the hash declaration. Also we mostly use SharedImpl as keys, so for those the default implementation is even less obvious. Therefore I needed these explicit functions and functors.

On the naming I could agree a bit more. But I believe it is not too hard to get it right. ObjPtr means that you have an object (SharedImpl) and want to hash by the shared pointer (so two SharedImpl pointing to the same object are considered equal). PtrObj means you have a raw pointer, but want to compare the actual object behind it (so two SharedImpl with different objects can sill be equal if the two objects are themselves equal). Obj and Ptr should be obvious them, they either compare the object or the pointer.

[glebm]

Also we mostly use SharedImpl as keys, so for those the default implementation is even less obvious.

It is idiomatic for smart pointers (like SharedImpl) to behave just like regular pointers. This means == compares the pointers (as we currently do), not the pointed values. This is what std::shared_ptr and std::unique_ptr do as well. This is why I don't consider the default equality/hash confusing at all.

On the naming I could agree a bit more.

I have an idea about this:

• Pointer(Equality|Hash)
• PointedValue(Equality|Hash)

There is no need for Obj-specific versions, as these functions are templated and work on anything that compares with nullptr and implements -> (both regular pointers and our smart pointer type).

[mgreter]

I need to compare objects in hashes here: https://github.com/sass/libsass/pull/2908/files#diff-61f3048af5edbbaf974fb08271245c6eR32 ... also see the first one, that compares the raw pointers.

[mgreter]

Polished up the extender code once more ...

[mgreter]

Something tripped up MSVC debug build, need to go back to last success and go from there ...

[glebm]

Latest error:

Failing test:

%default-color {color: red}
%alt-color {color: green}

%default-style {
@extend %default-color;
&:hover {@extend %alt-color}
&:active {@extend %default-color}
}

.test-case {@extend %default-style}

(from sass-spec\spec\extend-tests\215_test_multiple_source_redundancy_elimination.hrx)

[mgreter]

Maybe if you have time feel free to debug. I'm going to bed now. I think the latest commit is ok, but changing the other function prototypes keep triggering MSVC debug build. Will continue tomorrow.

[glebm]

Figured out where extension.target starts pointing to invalid memory:

extensionsByExtender[simple].push_back(withExtender);

Here once enough elements are pushed, extensionsByExtender[simple] reallocates.

Since this invalidates the pointers (extension.extender and extension.target), these objects appear to be pointers to elements of the extensionsByExtender[simple] vector.

This is unsafe because pointer addresses may change when the vector resizes its buffer.

Patch to expose the issue (not a fix): https://gist.github.com/glebm/75a6ddf05cf4533e79601adddb6047fc

[mgreter]

I feel that it might have something to do with for (auto x : loops, since IMO those use iterators that could get invalid during the whole loop, as opposite to regular for(size_t i loops.

[glebm]

I feel that it might have something to do with for (auto x : loops, since IMO those use iterators that could get invalid during the whole loop (as opposite to regular for(size_t i loops.

Definitely not iterators. This is caused by pointers (SharedImpl) pointing to elements in a vector storage. When the backing vector's buffer is resized, these SharedImpls' become dangling pointers. Evidenced by stepping through with a debugger to the crash (more obvious with my patch above).

[mgreter]

IMO the for range loops do iterate from begin to end, so if in the meantime, the underlying container is relocated, it makes kinda sense that the iteration in progress is about to fail. I could be wrong but my tests (I did now install MSVC 2013) seem to indicate that this is the case. Not saying the other thing you mention is also an issue.

[mgreter]

OK, got it to pass both tests. Will squash now to see what I got to clean up ...

[mgreter]

So I finally seem to have found the culprit, took way longer than it should have 😓

It basically boils down to:

- for (Extension extension : oldExtensions) {
- for (size_t i = 0; i < oldExtensions.size(); i += 1) {
+ for (size_t i = 0, iL = oldExtensions.size(); i < iL; i += 1) {

As expected we seem to append to the underlying vector somewhere. As I wrote earlier, this does invalidate the for range based loop, but also made clang on mac read the newly added items, which is not how the original dart sass behavior is (probably because it does in fact work on a copy). That's why once MSVC timed out and on another occasion clang on mac os did. Will finish up now ...

[mgreter]

Just to gain back my sanity, I made a test:

    for (size_t i = 0, iL = oldExtensions.size(); i < iL; i += 1) {
      if (iL != oldExtensions.size()) {
        std::cerr << "Gotcha\n";
      }

And who would have guessed:

#   Failed test 'Warnings: t\sass-spec\spec\extend-tests\215_test_multiple_source_redundancy_elimination\input.scss'
#   at t/99_sass_specs.t line 658.
# +---+----------+----------+
# | Ln|Got       |Expected  |
# +---+----------+----------+
# *  1|'Gotcha'  |''        *
# +---+----------+----------+

The actual change is in the same loop:

          sources.insert(complex, mergeExtension(
            sources.get(complex), withExtender));

[mgreter]

That should be the 🚀 final version. Just waiting for 💚 !
The last 2% were the hardest, as always ...

[mgreter]

While CI is doing its job, I just wanted to mention the magnitude of this commit. It closes and cuts-off one of the most "alien" code bases we dragged around for so many years, namely node.cpp and extend.cpp. To bad the bounty is no longer up for grab. Nearly 8000 lines of code mean about 20% of the whole libsass code base. And thanks to the base work by dart-sass and @nex3 this was possible. The time investment was something around 200h as I initially estimated. 🔥 🎆 🍻

[mgreter]

Here we go 🛫

[glebm]

Great work! 🎉

[xzyfer]

Very impressive effort

…

On Mon., 17 Jun. 2019, 9:00 pm Gleb Mazovetskiy, ***@***.***> wrote: Great work! 🎉 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2908?email_source=notifications&email_token=AAENSWAJHRFKCQH447Y7OQTP27NK7A5CNFSM4HYKAJQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX4EHKQ#issuecomment-502809514>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAENSWDNU4GOID354IEERTDP27NK7ANCNFSM4HYKAJQQ> .