Fix for sourcemap offsets

[squidfunk]

This PR fixes many issues with incorrect source mappings as reported in #2204. Unfortunately with the current design of libsass it's not possible to fix all issues (read on).

Issues fixed

Rules with parent selectors result in incorrect source mappings

Sequence_Selector::resolve_parent_refs replaces & with the actual name of the parent selector. The problem is that parent selectors are cloned with their original positions so any rule that directly extends a parent selector gets the original position of the parent selector, resulting in duplicate mappings. This is also why Google Chrome shows the same line numbers for the following rules:

.foo { // maps to line 1
  background: red;
  &-bar { // also maps to line 1
    background: green;
  }
}

Result:

• master
• fix/sourcemaps-parent-selectors

Emitted source mappings for compound classes differ from ruby-sass

Nested definitions that result in compound class definitions when compiled result in source mappings for each of the classes:

.foo {
  background: red;
  .bar {
    background: green;
  }
}

This results in separate source mappings for .foo and .bar. ruby-sass emits one source mapping for .foo .bar which is also more correct and more compatible with tools that rely on those mappings. This was fixed by fleshing out emitter.cpp and scheduling the open mapping in Inspect::operator()(Sequence_Selector* c), and the close mapping in Inspect::operator()(CommaSequence_Selector* g).

Result:

• master
• fix/sourcemaps-parent-selectors

Furthermore, most of the calls to append_token which add source mappings on their own without respecting the context have been replaced by append_string and specific source mappings in the callers. Source mappings were also generated for keywords or rules that don't make it into the final CSS, e.g. @return, @warn, @debug. Those have also been removed, because there is no direct representation in the compiled source.

Declarations with variables result in incorrect line numbers

When a variable is referenced in a declaration, the name of the variable and trailing semicolon map to the variable definition, but not to the position where it is referenced. This doesn't break Chrome source mappings (they are shown correctly when the classes are mapped correctly), but Gemini CSS Coverage or other tools that rely on source mappings for declarations.

$color: green;
.foo {
  background: $color; // starts in line 3, ends in line 1
}

This was fixed by omitting source mappings for all parts of the declaration, eg. background, :, green, ; and consolidating it into one mapping background: green;. The offset of the mapping in the original source is not absolutely correct, because the length of the value is not included in the parser state, so the mapping ends at the end of background.

Result:

• master
• fix/sourcemaps-parent-selectors

Issues that won't fix

While the noise in sourcemaps could be largely reduced and they appear to be actually usable, there are some cases that are, at least in my opinion, not fixable by the current design of libsass.

Inline blocks for @media and @supports

Media queries that do not scope the contained declarations into selectors, but are contained within a selector (inline) do not map correctly. The selector is inserted by the parser (it's an implied &), but since there is no direct representation of the implied parent in the original source, the parent refers to the original parent, which results in a double mapping that breaks offsets again.

.foo {
  background: red;
  @media only screen {
    background: green;
  }
}

Result:

• master
• fix/sourcemaps-parent-selectors - double mapping for .foo

However, in the proposed PR, this can be omitted by explicitly using the parent selector:

.foo {
  background: red;
  @media only screen {
    & {
      background: green;
    }
  }
}

Result:

• master - still broken
• fix/sourcemaps-parent-selectors - correct mappings

Sourcemap offsets are largely incorrect

The fact that libsass implements a multi-step approach (parse, check nesting, expand, check nesting, render) and that during transformation/expansion selectors and expressions are irreversibly overwritten, original offsets are irreversibly lost. Digging into the code and architecture of libsass (which in large parts is very well written!), I come to the conclusion that the emission of correct source mappings – correct lines and columns, the latter of which are currently largely incorrect – involves a huge rewrite. Some concrete examples:

• Variables are replaced with their values during expansion, which are registered in an environment variable (a map), so the offsets from the variable declaration kill the offsets of the variable usage.
• Selectors that have no representation in the original code do not get correct source mappings. This is true for the media query and the implied parent selector, but there are also other cases where selectors get generated. This is a general problem with source maps for CSS, which needs to be addressed.
• Parent selectors are kind of hot-fixed in the PR since the column offsets are still incorrect. However, since the line offsets are now correct, everything works reasonably well.

This PR fixes all issues that we currently have with Gemini CSS coverage (using the @media workaround). I'm very excited to hear your ideas on this PR and hope it finds its way into master, since it should be of good use for all of us.

[xzyfer]

Thank you for this. I wont have time to look it over until next week. In the mean times it's worth noting we purposely differ from Ruby Sass in some cases to provide a better experience in non-spec compliant browsers.

#1747

[squidfunk]

Thank you for the update. Feedback is appreciated.

[mgreter]

First thanks for your contributions. For some background info on sourcemaps. They were first implement in #207. I have then further improved it begining with #591. Since then no real cleanup has been done, as the initial applied architecture proved to have some unexpected issues, therefore the state of sourcemaps have always been kind of unknown.

I guess you mostly relate your expectations regarding sourcemap according to how gemini-coverage behaves? IMO sourcemaps V3 is still just a draft/proposal and I'll try to explain why libsass creates the sourcemaps as it currently does. We had lengthy discussions (//cc @am11) if we want to create just a very basic sourcemap (as ruby sass does) or a more elaborated one (as less does). We decided to try the second.

I also already want to point out that sourcemap has no concept of opening and closing mappings. AFAIR there is no concept of "enclosing" ranges for sourcemaps. Libsass does not output evenly distributed pairs of start-end mappings (explanations why this is follows).

.foo { // maps to line 1
  background: red;
  &-bar { // also maps to line 1
    background: green;
  }
}

SourceMap Inspector (click [inspect] and use keyboard arrows to navigate)

Going to try to explain the mappings libsass produces (see it in the inspector linked above): As @xzyfer already pointed out, we added one crutch to libsass (you can read up that in the linked issue) which causes a duplicate starting mapping (AAAA). Afterwards we have a start-end mapping for the .foo selector (AAAA,IAAI). Last comes a single mapping for the scope opening bracket (CAAC). Next line is a simple declaration (EACH,UAAU,EAAE,GAAI,GAIjB;). Third line has the parent selector. This is actually also just a duplicate start mapping (the first one is the duplicate: EALD, the second one the start mapping: AAAA). Then comes the closing map back to .foo selector (parent reference: QAAI). Last mapping (CAEI) again is a single scope opening bracket. Last line is just another simple declaration (IACJ,UAAU,EAAE,KAAM,GACnB).

I agree that we could argue if the mapping for the parent selector should point to where the & was or from where the actual selector in the output is comming from (libsass does the second).

IMHO this is a perfectly valid mapping according to the specs. We tried to do our best to get the best support for browsers. And different tools may take different assumptions. So why does libsass not output even sizes start-end position mappings? One reason is ie. how we could map variables. In theory we could add mappings for every variable, number etc. that was involved in creating the final output
result. Same analogy goes for parent selectors. I don't see a reason why the SourceMaps V3 proposal would not allow that, but I clearly see the problem that arises with it for tools that try to interoperate with each other. And since the SourceMap V3 draft is really just a proposal for now (and still evolving), we have the tendency to keep it as is for now, until the specs are final.

I just re-scanned the proposal and it seem that at least the following was added since a last read it (not sure if it addresses the issues we face here, namely that a lot is still not specified):

Index map: supporting post processing:
To support concatenating generated code and other common post processing, an alternate representation of a map is supported ... The sections must be sorted by starting position and the represented sections may not overlap.

Another example why getting it right is difficult: Take the selector &-bar (becomes .foo-bar). After evaluation we could have a mapping for & to .foo and one for -bar. If we would do that, the first mapping would point to the outermost scope, which would lead to #1747 (unuseful mapping in browsers).

Unfortunately your other examples can also be explained and do IMO not violate against the current SourceMap draft. IMO you can only call the behavior of libsass buggy under certain assumptions which are IMO not given by the current spec. Not sure if one interpretation of the current proposal by one tool is enough to change the current behavior by libsass (since the mappings are IMO technically correct, beside the mentioned crutch we had to add for browsers, and beside the few obvious missmaps, but they are mostly just in col and not in line).

$color: green;
.foo {
  background: $color; // starts in line 3, ends in line 1
}

SourceMap Inspector (click [inspect] and use keyboard arrows to navigate)

This one might actually contain one wrong mapping

.foo {
  background: red;
  .bar {
    background: green;
  }
}

SourceMap Inspector (click [inspect] and use keyboard arrows to navigate)

This one has a wrong col offset mapping for "screen".

.foo {
  background: red;
  @media only screen {
    background: green;
  }
}

SourceMap Inspector (click [inspect] and use keyboard arrows to navigate)

In regard to @return, @warn, @debug: they should never make their way to the output phase. This is just in place if an AST is rendered before the final cssize stage. It's just consistent with all the other items that can be inspected.

Sorry to not have any better news. I really really appreciate your effort. I know that going through libsass source code is not an easy thing and we are in need of good c++ coders here, so I would have hoped for a better outcome. Of course feel free to rebuttal, but I hope we can agree that the main ground should be the SourceMap V3 Proposal. We do tend to follow what browsers do if necessary, as they are really the 1st customers of sourcemaps. So unfortunately I have to decline this PR. Maybe @xzyfer or @am11 see it different. IMO to consider this, the tool in question would need to have a very broad user base, which gemini-coverage doesn't seem to have yet.

This pretty much sums up all the information I have about the state of libsass sourcemaps.
I thought you deserved a thorough answer for your effort!

With my very best regards
Marcel

[squidfunk]

Thank you for the very detailed explanation. I'm quite aware of the fact that my PR is kind of a hotfix to make it work within Chrome and Gemini. I wished I could fix it in a better way, but as you also noticed, the current design of libsass doesn't allow for that.

I guess you mostly relate your expectations regarding sourcemap according to how gemini-coverage behaves? IMO sourcemaps V3 is still just a draft/proposal and I'll try to explain why libsass creates the sourcemaps as it currently does.

Can you link to the section which states something about the way sourcemaps should look like? To me, this proposal only defines the format of the sourcemap file, so how the final JSON should look, not where sourcemap offsets should look like.

I also already want to point out that sourcemap has no concept of opening and closing mappings. AFAIR there is no concept of "enclosing" ranges for sourcemaps. Libsass does not output evenly distributed pairs of start-end mappings (explanations why this is follows).

This is perfectly fine and I understand the concept. Maybe I did not make it clear enough in my explanation. However, the name of the functions add_open_mapping and add_close_mapping might be irritating to the unaware reader.

... we added one crutch to libsass (you can read up that in the linked issue) which causes a duplicate starting mapping (AAAA).

I saw that crutch in the source, read the PR, and in my opinion it was at the wrong position. I put it into the rendering of the Sequence_Selector, in order to put it at the beginning of a compound selector.

I agree that we could argue if the mapping for the parent selector should point to where the & was or from where the actual selector in the output is comming from (libsass does the second).

Well, the main problem why I started with all this was the fact that every selector that is blended with the parent selector & points to the parent selector. We write CSS in BEM-style, and thus everything links to the parent.

IMHO this is a perfectly valid mapping according to the specs.

In my opinion, the specs state nothing about how mappings should look like, so every mapping is perfectly valid, as long as the final JSON can be parsed ;-)

We tried to do our best to get the best support for browsers.

Sourcemaps are broken in Chrome (see the linked issue), so the crutch that was added doesn't solve all the problems.

Unfortunately your other examples can also be explained and do IMO not violate against the current SourceMap draft. IMO you can only call the behavior of libsass buggy under certain assumptions which are IMO not given by the current spec.

Again, in my opinion, the spec doesn't say anything on mappings, so every mapping is valid. However, I do understand the point that you don't want to change things the way I proposed because it may break stuff downstream, so I'm fine with that.

Another example why getting it right is difficult: Take the selector &-bar (becomes .foo-bar). After evaluation we could have a mapping for & to .foo and one for -bar. If we would do that, the first mapping would point to the outermost scope, which would lead to #1747 (unuseful mapping in browsers).

This is wrong, because .foo-bar does have nothing to do with .foo - those are two entirely different selectors. For this reason the way libsass does it currently is wrong. This is also why using SASS with BEM produces wrong sourcemaps. However, this was the first thing I fixed in the following commit: 9da4841

Would you be okay with merging the changes only made in this commit (I can revert everything else in my PR)? This would already fix a lot for us.

[mgreter]

I have create #2216 which should address your issues.

This is perfectly fine and I understand the concept. Maybe I did not make it clear enough in my explanation. However, the name of the functions add_open_mapping and add_close_mapping might be irritating to the unaware reader.

These make sense in terms of how we parse stuff, as there we always have a start and end position, that's why these methods were named that way. Since then the code has evolved and doesn't fit well together in all ends. This works well for tokens that are parsed with one lex call. But doesn't work so well for containers. That's why I said that the whole thing would need a refactoring so everything makes sense together again. Nonetheless it is quite workable and stable, if one knows where to look exactly (debug_ast ist a big help as it shows you the mappings for every node).

This is wrong, because .foo-bar does have nothing to do with .foo - those are two entirely different selectors. For this reason the way libsass does it currently is wrong.

I disagree, since .foo-bar was generated from &-bar, so technically two tokens (.foo and &-bar) are the sources for the final output. Therefore it seems valid to map the .foo part to .foo and the -bar part to &-bar. But this is not possible for libsass atm, since the generated node .foo-bar can only hold a start and end mapping in libsass. But I think the general idea is valid and I could imagine browsers to support this one day (see the following screen that illustrates the same idea for back#{$ground} - ctrl click on properties in chrome inspector):

I think with #2216 all the reported issues should be solved. There are probably quite a few other places that would need similar fixing. I guess #2216 should show a few ways how to fix such sourcemap issues. Most should have similar causes as either not having the right mapping after parsing or that we don't update the mappings after operations.

[mgreter]

Merged #2216