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

SourceMap: Produces incorrect "mappings" #324

Closed
am11 opened this issue Mar 29, 2014 · 19 comments
Closed

SourceMap: Produces incorrect "mappings" #324

am11 opened this issue Mar 29, 2014 · 19 comments

Comments

@am11
Copy link
Contributor

am11 commented Mar 29, 2014

Consider the following code:

blockquote {
    .selector1 {
        .selector1a {
            display: none;
        }
        & > .selector1b {
            color: white;
        }
    }

    .selector2 {
        border: none;
    }
}

When compiled with source map switch, it produces the following B64VLQ mappings:

AACA,WAAW,WAAW;EAEV,SAAS;AACrB,WAAW,aAAa;EAEZ,OAAO;AAEnB,WAAW;EAGH,QAAQ

The decompiled form becomes:

Format: ([source row, source column] 
          (source file index in import chain -- index belongs to source array in map file))
          => [generated column] 


x----x-----x-----x-----x

([1,0](#0)=>[0,0]) | ([1,11](#0)=>[0,11]) | ([1,22](#0)=>[0,22])
([3,12](#0)=>[1,2]) | ([3,21](#0)=>[1,11])
([4,0](#0)=>[2,0]) | ([4,11](#0)=>[2,11]) | ([4,24](#0)=>[2,24])
([6,12](#0)=>[3,2]) | ([6,19](#0)=>[3,9])
([8,0](#0)=>[4,0]) | ([8,11](#0)=>[4,11])
([11,8](#0)=>[5,2]) | ([11,16](#0)=>[5,10])

Note in first row we have three pairs.

First one says source rule start at 1,0 (line, column) and generated rule is also present at 0,0 in CSS file.

Second suggests; when source is at 1,11 generated one starts at 0,11.. similarly the third one.

1: Inconsistent line numbers and column number

For source code, the line number start from 1 and column number starts from 0.
For generated code, both the line and column number start from 0.

Note that the second generated-column (0,11) is suddenly 1-based! The expected pair was (0,10).

Please make source file line number start from 0 (LESS and CoffeeScript also do the same).

But wait; what if in .scss file, we just have:

blockquote {
    content: "/*";
}

Then it shows 0,0 !! So the issue is more than the wrong line number. Perhaps the following portion has some gotchas...

2: It feels wrong in certain context!

Compile the same code with LESS, the mappings it generates are:

AAAA,UACI,WACI;EACI,aAAA;;AAEJ,UAJJ,WAIM;EACE,YAAA;;AANZ,UAUI;EACI,YAAA

Decompiles to:

([0,0](#0)=>[0,0]) | ([1,4](#0)=>[0,10]) | ([2,8](#0)=>[0,21])
([3,12](#0)=>[1,2]) | ([3,12](#0)=>[1,15])
([5,8](#0)=>[3,0]) | ([1,4](#0)=>[3,10]) | ([5,10](#0)=>[3,21])
([6,12](#0)=>[4,2]) | ([6,12](#0)=>[4,14])
([0,0](#0)=>[6,0]) | ([10,4](#0)=>[6,10])
([11,8](#0)=>[7,2]) | ([11,8](#0)=>[7,14])

Compare the last decoding of source line and column pair in first row:

LESS: ([2,8](#0)=>[0,21])
SCSS: ([1,22](#0)=>[0,22])

There is no 22nd column on line 1 in source code. Both are referring to .selector1a, which is present on line#3 of source.

It turned out; SCSS is not targeting the token itself but referring to the consecutive ranges. The pervious range of 1,22 is 1,11. So basically it reads like; "start from 11 on first line and read the following 22 characters".

But still it sees all three of them:

blockquote {
    .selector1 {
        .selector1a {

all on same line number 1; which is incorrect.

Please change its behavior (or provide an optional behavior); so the individual pair points to the exact beginning of token.

Thanks. 8-)

@am11 am11 changed the title SourceMap: Alternate "mappings" behavior (B64VLQ) SourceMap: Alternate "mappings" behavior Mar 29, 2014
am11 added a commit to am11/WebEssentials2013 that referenced this issue Mar 29, 2014
With the way we were extracting the selectors
before, some nested scenarios were failing.
With these changes, each subset of selector
now corresponds to its selector.

Secondly, SCSS' source maps is buggy.
Either that or they are doing it differently
than LESS and CoffeeScript. Nonetheless,
an issue is filed at sass/libsass#324.
Meanwhile, a post-process(ugly hack) is deployed
to bring harmony/uniformity in decoded maps from
various compilers.
am11 added a commit to am11/WebEssentials2013 that referenced this issue Mar 29, 2014
With the way we were extracting the selectors
before, some nested scenarios were failing.
With these changes, each subset of selector
now corresponds to its selector.

Secondly, SCSS' source maps is buggy.
Either that or they are doing it differently
than LESS and CoffeeScript. Nonetheless,
an issue is filed at sass/libsass#324.
Meanwhile, a post-process(ugly hack) is deployed
to bring harmony/uniformity in decoded maps from
various compilers.
@am11
Copy link
Contributor Author

am11 commented Mar 31, 2014

Here is the source-map generated by sass gem:

{
"version": 3,
"mappings": "AAEQ,iCAAY;EACR,OAAO,EAAE,IAAI;AAEjB,mCAAgB;EACZ,KAAK,EAAE,KAAK;AAIpB,qBAAW;EACP,MAAM,EAAE,IAAI",
"sources": ["test.scss"],
"names": [],
"file": "test.css"
}

Decompiles to:

([2,8](#0)=>[0,0]) | ([2,20](#0)=>[0,33])
([3,12](#0)=>[1,2]) | ([3,19](#0)=>[1,9]) | ([3,21](#0)=>[1,11]) | ([3,25](#0)=>[1,15])
([5,8](#0)=>[2,0]) | ([5,24](#0)=>[2,35])
([6,12](#0)=>[3,2]) | ([6,17](#0)=>[3,7]) | ([6,19](#0)=>[3,9]) | ([6,24](#0)=>[3,14])
([10,4](#0)=>[4,0]) | ([10,15](#0)=>[4,21])
([11,8](#0)=>[5,2]) | ([11,14](#0)=>[5,8]) | ([11,16](#0)=>[5,10]) | ([11,20](#0)=>[5,14])

Not as comprehensive as LESS (which captures the interim unlinked selectors -- with no declaration -- too) but at least the line numbers are not misleading like libsass.

@am11 am11 changed the title SourceMap: Alternate "mappings" behavior SourceMap: Incorrect "mappings" produced Mar 31, 2014
@am11 am11 changed the title SourceMap: Incorrect "mappings" produced SourceMap: Produces incorrect "mappings" Mar 31, 2014
@am11
Copy link
Contributor Author

am11 commented Mar 31, 2014

@nschonni, @andrew, is this issue related to libsass or node-sass?

@andrew
Copy link

andrew commented Mar 31, 2014

@am11 it's a libsass issue I believe.

SLaks pushed a commit to SLaks/WebEssentials2013 that referenced this issue May 13, 2014
With the way we were extracting the selectors
before, some nested scenarios were failing.
With these changes, each subset of selector
now corresponds to its selector.

Secondly, SCSS' source maps is buggy.
Either that or they are doing it differently
than LESS and CoffeeScript. Nonetheless,
an issue is filed at sass/libsass#324.
Meanwhile, a post-process(ugly hack) is deployed
to bring harmony/uniformity in decoded maps from
various compilers.
@simonexmachina
Copy link
Contributor

Is there any update on this issue? What can people do to help move this along?

@am11
Copy link
Contributor Author

am11 commented Sep 2, 2014

After #443 (comment), I tried to dig into it. The essence is; the libsass internals are only reporting the start of line (not even the correct column number), while it should be reporting a map for each token. My knowledge about that code is still wooly..

The operation is taking place in output_compressed.cpp and output_nested.cpp. We should make it so it conforms with LESS' mappings standards (npm install -g less) ; to capture mapping for maximum details: every token in the generated output.

I used http://murzwin.com/base64vlq.html (looks like it's written in Ruby) for decoding the VLQ-B64s while implementing the VLQ class and the hack for node-sass in Web Essentials.

Perhaps other folks like @akhleung, @mgreter and @nschonni can provide some insightful pointers here about that code.

@simonexmachina
Copy link
Contributor

Thanks for the detailed response. I really hope that you can get some help
from the maintainers because the current solution is really flawed. Keep up
the good work @am11!

am11 referenced this issue in am11/WebEssentials2013 Sep 23, 2014
The problem is, when both rtlcss and autoprefixer
are enabled, it works fine with SCSS, but throws
with LESS.

When the code is too complicated,
the `result` object seems to have problem with
returning `result.css` member. When the code is
comparatively straight-forward, it throws a
slightly lenient error.

/cc @MohammadYounes
@HamptonMakes
Copy link
Member

Neither @akhleung nor myself are familiar with or use sourcemaps ourselves. @svnieuw would you mind helping out again? We have a lot of open sourcemap issues and I'm feeling kind of helpless on them!

@simonexmachina
Copy link
Contributor

Yeah @svnieuw, we'd all really appreciate your help getting source maps right!

@am11
Copy link
Contributor Author

am11 commented Oct 5, 2014

@svnieuw 😄 👍

@ghost
Copy link

ghost commented Oct 6, 2014

I will try to have a look at this issue this week, but can't promise
anything.

On Sun, Oct 5, 2014 at 11:57 PM, Adeel Mujahid notifications@github.com
wrote:

@svnieuw https://github.com/svnieuw [image: 😄][image: 👍]


Reply to this email directly or view it on GitHub
#324 (comment).

@am11
Copy link
Contributor Author

am11 commented Oct 6, 2014

Thanking you in anticipation! 😄

@am11
Copy link
Contributor Author

am11 commented Oct 28, 2014

@xzyfer, can you shed some light on @svnieuw question? That was the last known show stopper here.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2014

@am11 I'm vaguely aware of what's happening there. My hunch would be that the r->selector()->path()/position() is potentially being set incorrectly by the parser. Try using r->path()/position() instead to see if the mappings still drift.

@svnieuw it'd be great if you could do a write up on how you're debugging and verifying the correctness of sourcemaps so one the maintainers can try dealing with these issues in the future.

@booleanbetrayal
Copy link

👍

1 similar comment
@adam-beck
Copy link

👍

@mgreter
Copy link
Contributor

mgreter commented Jan 6, 2015

This issue should (hopefully) be addressed/fixed by #792.
Please try with latest master and open a new issue if the problem still exists.
Thank you!

@mgreter mgreter closed this as completed Jan 6, 2015
@mgreter mgreter removed this from the 3.3 milestone Mar 9, 2015
am11 added a commit to am11/WebEssentials2013 that referenced this issue Jun 21, 2015
* Removes fallback VLQ cure for sass since sass/libsass#324 is fixed.
* `.ToList()` to eagerly load VLQ maps. The subsequent `.Count()` was
  causing redundant processing of all mappings (which is pretty
  expensive).
* Autoprefix takes stringly maps and returns object for map. So we are
  strigifying returned map in Less and Sass.
am11 added a commit to am11/WebEssentials2013 that referenced this issue Jun 21, 2015
* Removes fallback VLQ cure for sass since sass/libsass#324 is fixed.
* `.ToList()` to eagerly load VLQ maps. The subsequent `.Count()` was
  causing redundant processing of all mappings (which is pretty
  expensive).
* Autoprefix takes stringly maps and returns object for map. So we are
  strigifying returned map in Less and Sass.
GProulx pushed a commit to GProulx/WebEssentials2013 that referenced this issue Jun 23, 2015
* Removes fallback VLQ cure for sass since sass/libsass#324 is fixed.
* `.ToList()` to eagerly load VLQ maps. The subsequent `.Count()` was
  causing redundant processing of all mappings (which is pretty
  expensive).
* Autoprefix takes stringly maps and returns object for map. So we are
  strigifying returned map in Less and Sass.
GProulx pushed a commit to GProulx/WebEssentials2013 that referenced this issue Jun 23, 2015
* Removes fallback VLQ cure for sass since sass/libsass#324 is fixed.
* `.ToList()` to eagerly load VLQ maps. The subsequent `.Count()` was
  causing redundant processing of all mappings (which is pretty
  expensive).
* Autoprefix takes stringly maps and returns object for map. So we are
  strigifying returned map in Less and Sass.
GProulx pushed a commit to GProulx/WebEssentials2013 that referenced this issue Jun 23, 2015
* Removes fallback VLQ cure for sass since sass/libsass#324 is fixed.
* `.ToList()` to eagerly load VLQ maps. The subsequent `.Count()` was
  causing redundant processing of all mappings (which is pretty
  expensive).
* Autoprefix takes stringly maps and returns object for map. So we are
  strigifying returned map in Less and Sass.
GProulx pushed a commit to GProulx/WebEssentials2013 that referenced this issue Jun 23, 2015
* Removes fallback VLQ cure for sass since sass/libsass#324 is fixed.
* `.ToList()` to eagerly load VLQ maps. The subsequent `.Count()` was
  causing redundant processing of all mappings (which is pretty
  expensive).
* Autoprefix takes stringly maps and returns object for map. So we are
  strigifying returned map in Less and Sass.
GProulx pushed a commit to GProulx/WebEssentials2013 that referenced this issue Jun 23, 2015
* Removes fallback VLQ cure for sass since sass/libsass#324 is fixed.
* `.ToList()` to eagerly load VLQ maps. The subsequent `.Count()` was
  causing redundant processing of all mappings (which is pretty
  expensive).
* Autoprefix takes stringly maps and returns object for map. So we are
  strigifying returned map in Less and Sass.
GProulx pushed a commit to GProulx/WebEssentials2013 that referenced this issue Jun 23, 2015
* Removes fallback VLQ cure for sass since sass/libsass#324 is fixed.
* `.ToList()` to eagerly load VLQ maps. The subsequent `.Count()` was
  causing redundant processing of all mappings (which is pretty
  expensive).
* Autoprefix takes stringly maps and returns object for map. So we are
  strigifying returned map in Less and Sass.
GProulx pushed a commit to GProulx/WebEssentials2013 that referenced this issue Jun 23, 2015
* Removes fallback VLQ cure for sass since sass/libsass#324 is fixed.
* `.ToList()` to eagerly load VLQ maps. The subsequent `.Count()` was
  causing redundant processing of all mappings (which is pretty
  expensive).
* Autoprefix takes stringly maps and returns object for map. So we are
  strigifying returned map in Less and Sass.
GProulx pushed a commit to GProulx/WebEssentials2013 that referenced this issue Jun 23, 2015
* Removes fallback VLQ cure for sass since sass/libsass#324 is fixed.
* `.ToList()` to eagerly load VLQ maps. The subsequent `.Count()` was
  causing redundant processing of all mappings (which is pretty
  expensive).
* Autoprefix takes stringly maps and returns object for map. So we are
  strigifying returned map in Less and Sass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants