Bad variable rename on large project

[lukeapage]

I'm trying rollup out on less.js and found a bad rename. I cannot reproduce simply for now, so at the moment my test case is the less.js repo

1. fetch https://github.com/less/less.js/tree/rollup
2. npm install
3. copy the latest master dist build into node_modules (the fix for the last bug I reported is needed)
4. grunt rollup

this creates a file tmp/less.js

You could also run rollup directly on it, I'm using umd and the entry file is lib/less-browser/bootstrap.js

on line 6242

    function _environment(externalEnvironment, fileManagers) {

on line 6259

    }environment.prototype.getFileManager = function (filename, currentDirectory, options, environment, isSync) {

from the original sources

export default function environment(externalEnvironment, fileManagers) {
...
...
environment.prototype.getFileManager = function (filename, currentDirectory, options, environment, isSync) {

so it has renamed the export default, but not the prototype function.

I've gone from commonjs to es6 modules, so there could be a mistake somewhere in the modules, but even if there is, it shouldn't cause this side effect..

[lukeapage]

actually.. looking later in the file
on line 6281 from the same source file, its fine

    _environment.prototype.addFileManager = function (fileManager) {

it also seems a bit suspicious that its the only line thats screwed up whitespace. Is it possible that the newline character and _ character have been removed somewhere as part of the transformation?

[lukeapage]

after manually adjusting and running the tests I've found a couple more programming errors which I fixed and pushed but they haven't had any effect on this (line numbers unchanged)

[Victorystick]

I've reproduced the issue.

it also seems a bit suspicious that its the only line thats screwed up whitespace. Is it possible that the newline character and _ character have been removed somewhere as part of the transformation?

It's odd that only that line appears to be affected...

[lukeapage]

adding

            if (id.name === "environment") {
                console.log("renaming", id.name, "to", name);
                for(var i = 0; i < id.modifierStatements.length; i++) {
                    var modifierStatement = id.modifierStatements[i];
                    console.log("at...")
                    console.log(modifierStatement.module.source.substring(modifierStatement.node.start, modifierStatement.node.end));
                }
            }

in deconflict gives

renaming environment to _environment
at...
environment.prototype.getFileManager = function (filename, currentDirectory, options, environment, isSync) {

    if (!filename) {
        logger.warn("getFileManager called with no filename.. Please report this issue. continuing.");
    }
    if (currentDirectory == null) {
        logger.warn("getFileManager called with null directory.. Please report this issue. continuing.");
    }

    var fileManagers = this.fileManagers;
    if (options.pluginManager) {
        fileManagers = [].concat(fileManagers).concat(options.pluginManager.getFileManagers());
    }
    for (var i = fileManagers.length - 1; i >= 0 ; i--) {
        var fileManager = fileManagers[i];
        if (fileManager[isSync ? "supportsSync" : "supports"](filename, currentDirectory, options, environment)) {
            return fileManager;
        }
    }
    return null;
};
at...
environment.prototype.addFileManager = function (fileManager) {
    this.fileManagers.push(fileManager);
};
at...
environment.prototype.clearFileManagers = function () {
    this.fileManagers = [];
};

which is correct.. so something is going wrong with the actual rename.

investigating further...

[lukeapage]

Can you give me any hints as to where the rename actually occurs? I see that just because it found it in modifierStatements, doesn't mean it will be used in the rename

[Victorystick]

Scope.deconflict is where the renaming occurs. To make the changes take effect in the generated source, we invoke Statement.replaceIdentifiers on every statement here.

The magicString.overwrite call that replaces most identifiers is this one.

[Rich-Harris]

This now builds successfully with the contingency-plan branch – there's a test here

[lukeapage]

@Rich-Harris I've worked out a test case now, in the middle of tracking the problem and I'm not sure that test case is exactly the same issue.

[Rich-Harris]

@lukeapage That makes sense – I think both branches had an issue preventing less.js from building, but they were completely unrelated.

[lukeapage]

PR here - #161

[Rich-Harris]

Thanks @lukeapage – FWIW the test passes on contingency-plan, I've just released 0.17.1 from that branch.

[lukeapage]

there is now a fix in #161

[lukeapage]

I think both branches had an issue preventing less.js from building

I found another issue with my changes in less.js and after that the main browser tests pass with 0.17.3 or 0.16.3 with my fix

[Rich-Harris]

I'm going to close this issue since less.js appears to build successfully as of 0.18.2 – @lukeapage correct me if that's not the case!

[lukeapage]

yep this is fixed