Skip to content

Commit

Permalink
Fix: Generated mappings end at EOL
Browse files Browse the repository at this point in the history
This is to follow the spec.

As discussed in the [mailing list][1], when getting original positions,
mappings end at the end of the line. However, when getting generated
positions, mappings do not end at the end of the line.

[1]: https://groups.google.com/forum/#!topic/mozilla.dev.js-sourcemap/Uo26hB5v5oY

Getting original positions:

For a generated position (L,C), find a mapping (L,X) such that X<=C and
X is as great as possible. If no such mapping can be found, then there
is no mapping for (L,C). This is because according to the spec, mappings
end at the end of the line. Previously, we used to look for mappings on
lines <L too.

Getting generated positions:

For a generated position (L,C), find a mapping (A,B) such that A<=L, A
is as great as possible, B<=C and B is as great as possible. If there
are multiple such mappings, I don't know which is or should be chosen.
The spec doesn't say anything about it. The idea is at least that
multiline source statements usually (ideally) only have a mapping at the
beginning of the multiline statement. Then it is convienient to be able
to get the generated position even if you're not at the first line.

Note that this change is backwards incompatible. There were even tests
for the faulty behavior in test/source-map/test-dogfooding.js! I had to
change them, which of course breaks backwards compatibility.
  • Loading branch information
lydell committed Feb 14, 2014
1 parent 0196397 commit 699dff1
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 6 deletions.
2 changes: 1 addition & 1 deletion lib/source-map/source-map-consumer.js
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ define(function (require, exports, module) {
"generatedColumn",
util.compareByGeneratedPositions);

if (mapping) {
if (mapping && mapping.generatedLine === needle.generatedLine) {
var source = util.getArg(mapping, 'source', null);
if (source && this.sourceRoot) {
source = util.join(this.sourceRoot, source);
Expand Down
22 changes: 17 additions & 5 deletions test/source-map/test-dog-fooding.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,31 +42,43 @@ define(function (require, exports, module) {
generated: { line: 5, column: 2 }
});

smg.addMapping({
source: 'gza.coffee',
original: { line: 5, column: 10 },
generated: { line: 6, column: 12 }
});

var smc = new SourceMapConsumer(smg.toString());

// Exact
util.assertMapping(2, 2, '/wu/tang/gza.coffee', 1, 0, null, smc, assert);
util.assertMapping(3, 2, '/wu/tang/gza.coffee', 2, 0, null, smc, assert);
util.assertMapping(4, 2, '/wu/tang/gza.coffee', 3, 0, null, smc, assert);
util.assertMapping(5, 2, '/wu/tang/gza.coffee', 4, 0, null, smc, assert);
util.assertMapping(6, 12, '/wu/tang/gza.coffee', 5, 10, null, smc, assert);

// Fuzzy

// Original to generated
// Generated to original
util.assertMapping(2, 0, null, null, null, null, smc, assert, true);
util.assertMapping(2, 9, '/wu/tang/gza.coffee', 1, 0, null, smc, assert, true);
util.assertMapping(3, 0, '/wu/tang/gza.coffee', 1, 0, null, smc, assert, true);
util.assertMapping(3, 0, null, null, null, null, smc, assert, true);
util.assertMapping(3, 9, '/wu/tang/gza.coffee', 2, 0, null, smc, assert, true);
util.assertMapping(4, 0, '/wu/tang/gza.coffee', 2, 0, null, smc, assert, true);
util.assertMapping(4, 0, null, null, null, null, smc, assert, true);
util.assertMapping(4, 9, '/wu/tang/gza.coffee', 3, 0, null, smc, assert, true);
util.assertMapping(5, 0, '/wu/tang/gza.coffee', 3, 0, null, smc, assert, true);
util.assertMapping(5, 0, null, null, null, null, smc, assert, true);
util.assertMapping(5, 9, '/wu/tang/gza.coffee', 4, 0, null, smc, assert, true);
util.assertMapping(6, 0, null, null, null, null, smc, assert, true);
util.assertMapping(6, 9, null, null, null, null, smc, assert, true);
util.assertMapping(6, 13, '/wu/tang/gza.coffee', 5, 10, null, smc, assert, true);

// Generated to original
// Original to generated
util.assertMapping(2, 2, '/wu/tang/gza.coffee', 1, 1, null, smc, assert, null, true);
util.assertMapping(3, 2, '/wu/tang/gza.coffee', 2, 3, null, smc, assert, null, true);
util.assertMapping(4, 2, '/wu/tang/gza.coffee', 3, 6, null, smc, assert, null, true);
util.assertMapping(5, 2, '/wu/tang/gza.coffee', 4, 9, null, smc, assert, null, true);
util.assertMapping(5, 2, '/wu/tang/gza.coffee', 5, 9, null, smc, assert, null, true);
util.assertMapping(6, 12, '/wu/tang/gza.coffee', 6, 19, null, smc, assert, null, true);
};

});
24 changes: 24 additions & 0 deletions test/source-map/test-source-map-consumer.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,30 @@ define(function (require, exports, module) {
util.assertMapping(2, 9, '/the/root/two.js', 1, 16, null, map, assert, null, true);
};

exports['test mappings and end of lines'] = function (assert, util) {
var smg = new SourceMapGenerator({
file: 'foo.js'
});
smg.addMapping({
original: { line: 1, column: 1 },
generated: { line: 1, column: 1 },
source: 'bar.js'
});
smg.addMapping({
original: { line: 2, column: 2 },
generated: { line: 2, column: 2 },
source: 'bar.js'
});

var map = SourceMapConsumer.fromSourceMap(smg);

// When finding original positions, mappings end at the end of the line.
util.assertMapping(2, 1, null, null, null, null, map, assert, true)

// When finding generated positions, mappings do not end at the end of the line.
util.assertMapping(1, 1, 'bar.js', 2, 1, null, map, assert, null, true);
};

exports['test creating source map consumers with )]}\' prefix'] = function (assert, util) {
assert.doesNotThrow(function () {
var map = new SourceMapConsumer(")]}'" + JSON.stringify(util.testMap));
Expand Down

6 comments on commit 699dff1

@lieut-data
Copy link

Choose a reason for hiding this comment

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

I have a case where mapping.generatedLine is 29 and needle.generatedLine is "029", which fails to pass the strict type check on line 315. Source map is being generated by a version of UglifyJS2.

Relaxing this comparison resolves my issue, but is there any underlying problem to fix instead?

@lydell
Copy link
Contributor Author

@lydell lydell commented on 699dff1 Apr 7, 2014

Choose a reason for hiding this comment

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

is there any underlying problem to fix instead?

Yes, work with numbers, not strings, of course.

@lydell
Copy link
Contributor Author

@lydell lydell commented on 699dff1 Apr 7, 2014

Choose a reason for hiding this comment

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

BTW, it is possible to comment on certain lines (instead of writing "on line 315").

@lieut-data
Copy link

Choose a reason for hiding this comment

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

Aha! I didn't realize I could comment on a particular line -- thanks for the tip!

I've already monkey patched my version to perform the less strict comparison, but I wasn't sure if there was an even deeper problem that led to the disparate data types. Perhaps it's just an oddity of the various minifiers.

Thanks!

@lydell
Copy link
Contributor Author

@lydell lydell commented on 699dff1 Apr 8, 2014

Choose a reason for hiding this comment

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

Where does "029" come from?

@lieut-data
Copy link

Choose a reason for hiding this comment

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

Chalk this one up to PEBKAC -- upon digging deeper, I realized the source of the incorrect needle line/column values originated in my own code, and that it's perfectly reasonable for the code above to expect a numeric value. Sorry for the distraction!

Please sign in to comment.