[DNM] LibSass 4.0 Alpha Big Bang

[mgreter]

Continued from #2918

Please note that this PR is a little bit bigger than your regular PR :)

During the last year development of LibSass seems to have stagnated, which is certainly partially true!
Although corona has hit us all, I still did enjoy the beautiful summer here in my hometown and took
some time off from GitHub and opensource. None the less I already had invested quite some time into
a dart-sass parser back-port, after the back-port for the extend code already landed in master (not without
any headaches). During the year I've back-ported, refactored or rewritten nearly 90% of the code-base.

I always had a version that was working, but the code behind it was never 100% clean. It still isn't in a lot
of parts, but in many others the code is now reviewed and properly done. The new version will also require
at least a very decent C++11 compiler, as I took great care to use move semantics wherever possible!

Don't bother to try to review this PR, you can just review the whole code base instead ;)
There are still edges not fully reworked or that still have some debugging traits attached!
Corresponding sass-spec branch at https://github.com/mgreter/sass-spec/tree/refactor/libsass-4-alpha

Open points for LibSass 4.0 ALPHA release:

• Define, implement and finalize C-API
• Fully finalize the C-API (85%)
• Update documentation pages (90%)
• Cleanup exception handling (40%)
• Replace addFinalStackTrace (100%)
• Change default precision to 10.
• Finalize terminal/logger (90%)
• Check source mappings (80%)
• Check output styles (35%)

Additional points to be done:

• Get approval for sass-spec changes (or use fork repo)
• Get approval to merge into sass/libsass master
• Render error css to output file

Optional stuff:

• Add training target for MSVC
• Move warn return to C-API invocation.
• Extend C-API for SassNumber units.
• Extend C-API for SassFunctions.
• Custom importer for @use etc.
• Compile to wasm32-wasi target.
• Support external callbacks for log/warn.

I will update those list if I find any other points.

For the release notes I might also want to mention the main improvements:

• Native css/sass imports are now supported
• Added HWB color modes (only for already available functions)
  • Functions whiteness/blackness are only exposed via module system
• CRTP changed to more clear visitor pattern (tested no performance impact)
• Detangled expression, statement, value and selector base classes
• Fixed quoted/literal strings, binary operations and interpolations
• Initial module use and forward support

BTW I will keep squashing and force pushing this branch!

[nschonni]

Since most of the code is changing, does it make sense to setup a formatter like in the (broken) #2324

[mgreter]

@nschonni Still want to wait with that, I used default formatter of MSVC up to now.
IMO it's something we can easily do at any point and nothing gained or lost by not doing it.
There are still bigger fish to fry, and so long I'm mostly the only one working with the code.

[asottile]

(not going to subscribe so if you need to reply, please @-mention me -- thanks!)

I took a look at this on stream today and ran into a few things and I think I'm stuck (maybe an api is missing?). here's the notes I have for this:

• sass_value_get_tag should take const struct SassValue* instead
• I think I need sass_make_import_error for custom importers, I couldn't figure out how to port the existing code
• how do I replace sass_make_data_context -- I assume with SassCompiler*
  but I don't see any apis for it. same for sass_make_file_context

I also created two PRs against your branch to fix a few small problems I ran into:

• Add missing headers to install-headers mgreter/libsass#6
• make sass_make_map error on unknown arguments mgreter/libsass#7

excited for the changes here! some of the map / list push / set stuff is really really nice!

[asottile]

neat! I got a lot further today, here's my next round of feedback!

I couldn't get sourcemaps working at all, I'm guessing they're not completed yet or I'm holding it wrong -- probably the latter let's be realistic 😄

I also couldn't get custom function working 🤔

I've got two new PRs for today: mgreter#8 mgreter#9

and I have a bunch of oddities I can't explain :S

• compiling a{} used to lead to a result of an empty string, but now produces no output at all (null?) -- I didn't dig into this much, I'm not quite sure where this comes from, my guess is there's a truthy check where there should be a null check somewhere 🤷

compressed mode seems to be overly aggressive, breaking some selectors:

$ pysassc testpkg/testpkg/static/scss/a.scss -t compressed
pa{color:red}pb{color:blue}

$ cat testpkg/testpkg/static/scss/a.scss
p {
  a {
    color: red;
  }
  b {
    color: blue;
  }
}

the default output mode seems to have changed indentation slightly:

$ pysassc test/a.scss  # NEW
body {
  background-color: green; }

body a {
  color: blue; }

$ /tmp/venv/bin/pysassc test/a.scss  # OLD
body {
  background-color: green; }
  body a {
    color: blue; }

and lastly, it seems the quotes have changed from single to double:

$ pysassc test/d.scss  # NEW
@charset "UTF-8";
body {
  background-color: green; }

body a {
  font: "나눔고딕", sans-serif; }


$ /tmp/venv/bin/pysassc test/d.scss  # OLD
@charset "UTF-8";
body {
  background-color: green; }
  body a {
    font: '나눔고딕', sans-serif; }

[mgreter]

SourceMaps and custom functions/importers are indeed last on my list!
Although SourceMaps should be producible, but I haven't checked the results.
Regarding quotes, there are a lot of subtle changes which are closer to dart-sass.
Maybe dart-sass outputs it the same? Anyway, guess this is a very minor one ...
There will certainly be a lot of these changes in LibSass 4.0!

Was busy the last two weeks implementing use and forward modules.
Had experimentally done it before so I knew it needs a major overhaul of scope handlings.
This should now be done and I only seem to have lost about 5% of performance currently.
It seems to be pretty complete already, but no module extend barriers have been added yet.
Anyway should see a sea of passing spec-tests :)

5814 runs, 0 assertions, 0 failures, 0 errors, 69 skips

And yes, that's against the full sass-spec suite!

Also some current benchmarks against bolt-bench (compiled via MSVC 2019).
Dart-sass (untrained from git clone) clocked in earlier this year at around 9s.

min: 0.8049
max: 0.8286
avg: 0.8122

With training we can get this even further down :)

min: 0.6972
max: 0.7201
avg: 0.7116

I will now try to get back to the C-API. Not sure I will give it a try to (not) support
extends across module upstream boundaries. Probably have to do some cleanup
regarding the loading, which I already did, so the re-adding of custom importers
should not be too hard. But need to see which API adjustments would make sense.

I really hope I can wrap this all up for X-Mas.

[mgreter]

For the other points you mentioned:

pa{color:red}pb{color:blue}

Yep, unfortunately we don't test output modes in sass-spec anymore.
Probably needs a mandatory space instead of an optional one somewhere.
I plan to add at least some bare specs in libsass-specs, but not had time yet.
It may also happen that libsass 4.0 alpha will only support one output mode first.

For the indentation I try to match dart-sass, so need to check against that.
I consider these white-space changes not very important or do you disagree?
The same goes for the quotes, as long as the are valid I think it should be ok.

[kerryj89]

I came from here sass/sass#2737 and saw the huge leaps in performance you've made! Really impressive stuff.

My project has one entrypoint file for sass and uses the latest gulp-sass (dart-sass js) to build it. This takes about about 10s to build for every change which is about the same time as node-sass surprisingly (I would have thought node-sass using libsass would be much faster but I think the dart-sass team has made some great gains in performance in the last few years to make the JS version comparable to the libsass binary).

Anyway, what surprised me was that dart-sass cli only took 1.734s to build my project. Dart-sass team are working on "Embedded Dart Sass" to leverage this efficiency because this level can't be reached by relying on the JS version.

With all that said, if your version is out soon I may change back to node-sass just for the sake of these massive speed gains. Thanks for maintaining and backporting even though this project is listed as deprecated, I always like having options. I would take speed over feature-set until my needs for the project are no longer met, and having survived without most of the new features I think that can last a few more years or until Embedded Dart Sass happens.

[mgreter]

Sorry for the long silence, but as you know, life and stuff 😃 The C-API part of the todo list is closely to be finished, one of the biggest open points for a LibSass 4.0 alpha release. I guess most other points can be prolonged into the alpha phase, as they are often optional and highly OS dependent (e.g. terminal Color or Unicode recognition). Implementers can often simply disable a feature via options if it turns out to be unreliable or bugged. On the other hand there are so many changes that I would like to get it right in the first place as much as possible.

[johnfairh]

Great to see all the progress on this. I've been playing at wrapping a Swift API around this branch; the core of the thing seems really good, here are a few minor problems I found:

• Output path is not inferred from input path -- because it's initialized non-empty
• error_get_string() always comes back NULL -- I think because the what variable is nobbled by this loop before it's copied
• set_logger_style() doesn't always work -- the requested style is ignored unless AUTO
• Units::unit(u) does not parse the '(x*y)^-1' style that can be produced by Units::unit()
  (hmm I can't remember why I cared about this -- so FYI I suppose!)
• @use doesn't seem to consult custom importers (no problems with @import)

Happy to PR at least the easy ones...

Some less specific things:

• The CWD caching may be scoped too widely. I was surprised to find that when I changed my process's current directory between separate Sass compile jobs that LibSass retained the previous directory. Easily addressed with sass_chdir() but the comment on the prototype is a bit discouraging -- what design point do you have in mind here?
• The signal handling is more opinionated than I expect in a library -- perhaps this is just for your bringup though?
• It's probably worth exposing a bunch of the unit APIs under sass_number_ as per Dart -- it is tough to work with the pretty-printed string. I realise this is all new WIP code for LibSass.
• Did you consider unifying the Sass::Function and SassFunction types at all? Dart Sass allows the return value of a custom function to be a new custom function which I don't think is possible here.

Again I'm really glad to see your efforts here.

[mgreter]

Thx @johnfairh for the feedback, very useful and encouraging as it seems you already got quite far with the current state.

• Output path is not inferred from input path -> seems to be a bug then
• error_get_string() always comes back NULL -> will check that, thx
• set_logger_style() doesn't always work -> else case seems missing, thx
• Units::unit(u) does not parse the '(x*y)^-1' -> need to check how far I want to extend this for a initial 4.0 relesae
• @use doesn't seem to consult custom importers -> currently deliberately left out. Not sure if I want to up the API a bit to support these new cases different. IMO this could wait until after an initial 4.0 release until people adapt this new feature more.
• The CWD caching may be scoped too widely. -> IMHO changing the working directory via sysfs calls is dangerous, as the CWD is a process wide entity and is therefore shared between all threads and linked libraries of your app. So if this would be the only way for an implementor to tell LibSass where to look for the initial file load, we could introduce issues if an app uses different libraries or code that depends on CWD. Therefore I guess it's safest to just get it once and then use a local variable for the CWD (which makes it necessary to have the sass_chdir function). Not sure why I added the warning on the API though. Certainly needs more clear documenting if that's the way LibSass will go.
• signal handling -> not sure what you mean exactly as the link seems incorrect!?
• exposing a bunch of the unit APIs -> could be a future improvement, IMO for an initial release this has very low priority. I will add it to the Optional ToDo list in the initial post in this thread.
• unifying the Sass::Function and SassFunction types -> I did consider it but is also something that can be added after an initial release. There are certainly more of these nice to have details, but I think it's better now to focus on bringing what is currently there to a releasable state. But will also add it to the Optional ToDo list.

Thanks again and please feel free to poke around more in the code ;)
Maybe I can also squeeze in some more hours over Easter!

[mgreter]

Hopefully fixed error_get_string()and set_logger_style(). Also checked "Output path is not inferred from input path", but after looking at it again, I'm not sure we actually can infer the output path, although we might have done so in the past, so this might be valid after all, but needs a bit more clarification. E.g. if CLI only specifies input path we assume the output is just printed to the console and this kinda implies that we can't e.g. reliably create sourcemap references (I guess this is were I was aiming at with it).

[johnfairh]

Thanks; fixes look good. Your general approach of prioritising releasing something sounds right to me!

I understand your point about CWD as global shared state. Think I agree current behaviour is fine with docs.

This is the signal code I meant to link: I don't generally expect a library to grab all these and decide what the right behaviour is for the host program.

Output path inference. Old LibSass infers the output path if the user supplies an input file; as you hint the only usage is to fill in the optional 'file' SourceMap field -- so really maybe any behaviour here (1 - infer a path; 2 - assume stdout; 3 - don't fill in the field) is acceptable as long as it's documented; the SourceMap can always be fixed up by the client later. Marginal preference for keeping the old behaviour I suppose.

[mgreter]

Minor update as I'm currently trying to rebase sass-spec to latest master. There have been some major additions in dart-sass in regard to "slash" lists and "math.div" which are a bit more complicated to support, mainly because of the deprecation warnings, as LibSass doesn't have a way (yet) to convert Expressions to strings. So this might add another month or two of development to support correctly. So far around +500 new tests and ~30 are not exactly reporting as expected (mostly warning that mismatch).

[mgreter]

Continues in https://github.com/mgreter/libsass-ng/tree/develop
And https://github.com/mgreter/libsass-ng/actions/runs/9558047752
Still a lot of work to do: https://github.com/sponsors/mgreter 😄

As a little teaser, here some current benchmarks:

bolt-bench-dart: 5.2958406s
bolt-bench-sassc: 0.58601194s
spec-bench-dart: 3.6402846s
spec-bench-sassc: 0.688924067s

https://github.com/sass/dart-sass/blob/main/perf.md

preceding-sparse-dart: 2.0365426s
preceding-sparse-sassc: 0.41399228s
following-sparse-dart: 2.045372s
following-sparse-sassc: 0.4261757917s
preceding-dense-dart: 2.2909458s
preceding-dense-sassc: 0.6903514s
following-dense-dart: 1.96043s
following-dense-sassc: 0.433s

sassc.zip