Fix escapes in dictionary keys. Place source token in sourceMap

[startswithaj]

This will fix dict keys like the ones in lodash:

var deburredLetters = {
'\xc0': 'A', '\xc1': 'A', '\xc2': 'A', '\xc3': 'A', '\xc4': 'A', '\xc5': 'A',
'\xe0': 'a', '\xe1': 'a', '\xe2': 'a', '\xe3': 'a', '\xe4': 'a', '\xe5': 'a',
'\xc7': 'C', '\xe7': 'c',
'\xd0': 'D', '\xf0': 'd',
'\xc8': 'E', '\xc9': 'E', '\xca': 'E', '\xcb': 'E',
'\xe8': 'e', '\xe9': 'e', '\xea': 'e', '\xeb': 'e',
'\xcc': 'I', '\xcd': 'I', '\xce': 'I', '\xcf': 'I',
'\xec': 'i', '\xed': 'i', '\xee': 'i', '\xef': 'i',
'\xd1': 'N', '\xf1': 'n',
'\xd2': 'O', '\xd3': 'O', '\xd4': 'O', '\xd5': 'O', '\xd6': 'O', '\xd8': 'O',
'\xf2': 'o', '\xf3': 'o', '\xf4': 'o', '\xf5': 'o', '\xf6': 'o', '\xf8': 'o',
'\xd9': 'U', '\xda': 'U', '\xdb': 'U', '\xdc': 'U',
'\xf9': 'u', '\xfa': 'u', '\xfb': 'u', '\xfc': 'u',
'\xdd': 'Y', '\xfd': 'y', '\xff': 'y',
'\xc6': 'Ae', '\xe6': 'ae',
'\xde': 'Th', '\xfe': 'th',
'\xdf': 'ss'
};

The keys are read in and converted to their actual character representation by the AST but not escaped back when they are placed in the sourcemap. This means that À gets inserted into the sourcemap when in reality the key is \xc0.

So its looking to map À at whatever offset it can only find '\xc0' in the source. And usually its trying to match 1 character so it attempts to to match À == '.

À should never get into the sourcemap because its not in the source.

@jmalloc might be able to add to this as he came up with the solution.

Solves #341 as well

[startswithaj]

#268 mattrobenolt/python-sourcemap#6

[kzc]

If I understand this pull request correctly, every type of token would have a new __HACK_CODE__ property pointing to the original JS source. That could prove useful for other transforms. I'd just suggest a better name for the property like source.

I'd personally prefer that such a source property be made optional though since the source may not necessarily be available from other parser front ends. If the information is available, great, use it. If not, it should work as before.

[startswithaj]

@kzc yeah your right about the property name. We purposely called it an obtuse name because we were just proposing a solution to the problem and we were hoping that someone more familiar with the library would be able to tell us why it doesn't do this in the first place. As AST is not source code and shouldn't be used to key the soucemaps.

In regards to your second point surely any parser creating sourcemap needs to have access to the source?

[kzc]

any parser creating sourcemap needs to have access to the source?

Source maps are optional.

[startswithaj]

@kzc I still don't follow? Could you please explain a little more about how the source wouldnt be available to the parser?

[kzc]

The back end has to work with a few parsers. I don't think this pull request would work with --acorn or --spidermonkey. The code should still work in the absence of this new source property.

[startswithaj]

It would still work in the absense of thsi new source property because the source property is only accessed in the add_mapping function.

[kzc]

With this pull request does --source-map still work when --acorn or --spidermonkey is in effect?

[startswithaj]

@kzc if you send me a file you want me to test --spidermonkey against I will do it tomorrow.

[kzc]

I've never used --acorn or --spidermonkey. It's up to the contributers to test that their code doesn't break existing functionality.

[startswithaj]

I'm sorry @kzc but as a contributor it is not at all my roll to understand every use case of a product. Particularly that of this product which is vast and missing tests.

I submitted this PR, predicated on the fact that someone 'more familiar with the library' should take a look at it. Hence the variable names.

I'm not going to continue this conversation if you pull the branch and have an actually problem please do let me know. I'm not going to waste more of my time tracking down problems that don't exist.

@mishoo there is are quite a few outstanding issues and PR's that this solves.

On 19 Oct 2015, at 1:09 AM, kzc [email protected] wrote:

I've never used --acorn or --spidermonkey. It's up to the contributers to test that their code doesn't break existing functionality.

—
Reply to this email directly or view it on GitHub.

[kzc]

If you run this command against your pull request:

bin/uglifyjs lib/output.js --acorn --source-map output.map

you'll see the problem I suspected in my comments above:

WARN: Couldn't figure out mapping for lib/output.js:44,0 → 1,0 []
WARN: Couldn't figure out mapping for lib/output.js:46,0 → 1,12 []
WARN: Couldn't figure out mapping for lib/output.js:46,9 → 1,21 []
WARN: Couldn't figure out mapping for lib/output.js:46,22 → 1,35 []
...

This line:

        var originalSource = token.__HACK_CODE__.substring(token.pos, token.endpos)

threw:

TypeError: Cannot read property 'substring' of undefined

I presume the spidermonkey front end has the same issue.

The output.map should be similar to one run against UglifyJS2 v2.5.0.

[rvanvelzen]

This is probably outdated, so I'm closing this PR. Feel free to re-submit if you think it should be merged.