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] Prepare selector refactoring for extend refactoring #2904

Closed
wants to merge 3 commits into from

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented May 31, 2019

Needs sass/sass-spec#1396

This PR and Text is in flux and (hopefully) updated regularly.

This is probably the biggest refactoring after bringing reference counted
memory objects to libsass -> https://github.com/sass/libsass/pull/2222/files.

So far I invested around 60h for this refactoring, and I estimate that it needs
around 120h more to finish from here. The main goals here are:

  • Refactor complex selectors to get rid of "tail" linked lists
  • Much closer (on par) to how selectors are implemented in dart-sass
  • Get rid of or "normalize" parent selector use

ToDo:

  • Refactor unify selector paths
  • Refactor is superselector paths
  • Refactor @extend and drop "Node" class
  • Drop and replace old Complex_Selector class
  • Optimize hot code paths to reduce allocations

Already Done:

  • Convert between old and new object representations

Main pain points:

  • extend does alter/modify an existing ruleset and its selector_list. So we can't convert here
    between old and new representation, since extend is meant to update the existing
    reference (if we do it on a copy, the result will never be in the resulting css).
    But I can make a start at selector-extend function. Once I have that one right I can
    try to change the real extend logic to use the new implementation (current ToDo).
    But we have to exchange the whole implementation at once, so for now I try
    to reliably convert small functions from dart-sass to libsass.

What went well

  • The reference counted implementation works extremely well (better than I expected).

Get rid of or "normalize" parent selector use

When I first fixed/improved/implemented parent resolving I wasn't ware of all the
subtle details how sass handles those, so I made a few logic errors here and there.
The linked-list nature of complex selectors also added to that confusion. With the new
implementation we should get a clear understanding of implicit and explicit parent selectors.

The goal for this new implementation is to get rid of explicit parent selector objects (we had
them as fake and real ones). New we have ComplexSelector->chroots (TBD) and CompoundSelector->hasRealParent (TBD). These boolean options replace all the
"FAKE" (implicit) Parent_Selector instances. It might also be worth mentioning that
a CompoundSelector can only have an explicit parent at the beginning and must
errror if seen at any other position, so the boolean flag matches exactly that. On the
other hand any parent context will define if a SelectorList should be connected to
its parent on the stack or not (e.g. @at-root), so the chroots option fits here well.

  • CompoundSelector->hasRealParent() - The compound selector (list of simple selectors)
    starts with an explicit parent selector (e.g. &.foo).
  • ComplexSelector->chroots() - If true, don't connect to parent on stack (formerly fake parent).
  • In the end a Parent_Selector should only be seen in interpolations. But I still have to see
    if we need to treat interpolations different from regular parent selectors (we probably do).

New selector structure (equal to dart sass)

  • A SelectorList is an array of ComplexSelector
  • A ComplexSelector is an array of CompoundOrCombinator
    • Between each CompoundOrCombinator is a space
    • 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.

And btw. if somebody wants to sponsor such efforts, I do have a paypal account :)

@mgreter
Copy link
Contributor Author

mgreter commented Jun 2, 2019

@nex3 from an implementer standpoint I would really like to have more low-level spec-tests.
I already added a "selector-trim" function to libsass and dart-sass, so I can compare results
at a lower level, which helps me to find differences in the implementations.
Would you mind exposing more of the lower level functions to sass land? E.g.

  • selector-trim
  • selector-paths
  • selector-weave
  • selector-subweave
  • selector-weave-parents

I know those functions are probably useless for end-users, but AFAICS their APIs seem
well defined, so we could at least profit from having them properly spec-tested? Even
if I only could get tests for the first three functions, it would IMO help implementers (me) !

Just wanted to ask if that would be something you might consider or not? Thanks!

Btw. here my local code for trim I added to dart-sass:

functions.dart

BuiltInCallable("selector-trim", r"$selectors", (arguments) {
  var selectors = arguments[0].assertSelector(name: "selectors");
  var result = Extender.trim(selectors.components);
  return SelectorList(result).asSassList;
})

extender.dart

static List<ComplexSelector> trim(List<ComplexSelector> selectors) {
  var extender = Extender._mode(ExtendMode.normal);
  return extender._trim(selectors, extender._originals.contains);
}

@mgreter mgreter added this to the 3.7 milestone Jun 2, 2019
@nex3
Copy link
Contributor

nex3 commented Jun 2, 2019

I'm not a big fan of adding this to the language, for a couple reasons. First, as you say, it's not very useful for end-users and I want to keep the set of functions we expose pretty tightly scoped to real user needs. Second, I don't want to lock future (or existing) implementations into a given internal model of extensions. We even did this for Dart Sass—the ways its extensions work isn't identical to Ruby Sass, and we got performance gains as a result.

On the other hand, it's totally valid to add code like you have in your examples to a fork of Dart Sass, and use that to generate local test cases or to assist with debugging. I think that's a great strategy.

On a tangential note—since you're spending time on LibSass dev, have you considered working on #2887? It's something we'd like to land in Dart Sass very soon, but we don't want to deprecate something there if it's still the only solution in LibSass.

src/ast.hpp Outdated Show resolved Hide resolved
src/tsl/ordered_set.h Outdated Show resolved Hide resolved
@mgreter mgreter force-pushed the refactor-selectors branch from 1fb6a0f to c2bb647 Compare June 11, 2019 23:36
Copy link
Contributor

@glebm glebm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few review comments on the first few files. The diff does make it difficult to do a full review.

I realize this is WIP but there are bugs that I've spotted, hoping this helps.

src/ast.hpp Outdated Show resolved Hide resolved
src/ast.hpp Show resolved Hide resolved
src/ast.hpp Outdated Show resolved Hide resolved
src/ast.hpp Outdated Show resolved Hide resolved
src/ast.hpp Show resolved Hide resolved
src/ast_fwd_decl.hpp Outdated Show resolved Hide resolved
src/ast_fwd_decl.hpp Outdated Show resolved Hide resolved
@mgreter mgreter force-pushed the refactor-selectors branch 4 times, most recently from c1c95c8 to de42ad1 Compare June 12, 2019 19:36
@mgreter mgreter force-pushed the refactor-selectors branch 10 times, most recently from fc89dc7 to 0393104 Compare June 14, 2019 14:25
@mgreter mgreter force-pushed the refactor-selectors branch from 0393104 to 7aaabd2 Compare June 14, 2019 15:17
This operator is only needed on values,
since sass also allows to compare them.
@mgreter
Copy link
Contributor Author

mgreter commented Jun 14, 2019

Continued in #2908

@mgreter mgreter closed this Jun 14, 2019
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