google / traceur-compiler

Traceur is a JavaScript.next-to-JavaScript-of-today compiler
Apache License 2.0
8.18k stars 578 forks source link

InlineES6ModuleTransformer fails when two modules import to the same name. #2005

Open johnjbarton opened 8 years ago

johnjbarton commented 8 years ago

Given two modules:

import {x} from './reexport-x.js';
this.result = x;  // To verify execution, test this global value.

and

// Combine with import-x.js to test InlineES6ModuleTransformer
import {x} from './reexport-x.js';
export var y = x;

The compilation with options --modules=inline --outputLanguage=es6 will fail with

Duplicate declaration, x

The generated code is

const $__test_47_unit_47_node_47_resources_47_x_46_js__ = (function() {
  "use strict";
  var x = 'x';
  return {get x() {
      return x;
    }};
})();
const $__test_47_unit_47_node_47_resources_47_reexport_45_x_46_js__ = (function() {
  "use strict";
  return {get x() {
      return $__test_47_unit_47_node_47_resources_47_x_46_js__.x;
    }};
})();
"use strict";
const {x} = $__test_47_unit_47_node_47_resources_47_reexport_45_x_46_js__;
this.result = x;
"use strict";
const {x} = $__test_47_unit_47_node_47_resources_47_reexport_45_x_46_js__;
export var y = x;

The problem is that the names in the ModuleSpecifierSet are not renamed: https://github.com/google/traceur-compiler/blob/master/src/codegeneration/InlineES6ModuleTransformer.js#L132

arv commented 8 years ago

What is the command line you are using? How do we we happen to stil have that export line?

johnjbarton commented 8 years ago
node /work/traceur-compiler/src/node/command.js --out /work/traceur-compiler/0327dd1d-8843-450f-9b1f-24fe6e10d7b7.js --modules=inline --outputLanguage=es6 -- /work/traceur-compiler/test/unit/node/resources/import-x.js /work/traceur-compiler/test/unit/node/resources/reimport-x.js

I don't know why the export is still there, I suppose it's another small bug in the InlineES6ModuleTransformer.

arv commented 8 years ago

I think we should make it an error to provide more than one module here.

If you want to provide multiple modules then a module that imports those modules would work better:

import {x as x1} from './import-x.js';
import {x as x2} from './reimport-x.js';
...
johnjbarton commented 8 years ago

I don't see how that would help: the result of the --modules=inline will be one file including the two modules and the import-as code. The problem comes from the two modules: their code has to be rewritten to replace {x} with {x as x1} to ensure that the generated code does not redeclare x. I can try that in our code.

arv commented 8 years ago

This works:

node src/node/command.js --out out.js --modules=inline --outputLanguage=es6 -- test/unit/node/resources/import-x.js

What I'm saying is that we should not allow more than one input file here. That way there cannot be any naming conflicts.

However, I do see a use case to do what rollupjs is doing and put all the imported modules into the same scope so that imported bindings are truly the same binding as the exported one.

johnjbarton commented 8 years ago

I'm still confused:

node src/node/command.js --out out.js --modules=inline --outputLanguage=es6 -- test/unit/node/resources/import-x.js

gives

const $__test_47_unit_47_node_47_resources_47_x_46_js__ = (function() {
  "use strict";
  var x = 'x';
  return {get x() {
      return x;
    }};
})();
const $__test_47_unit_47_node_47_resources_47_reexport_45_x_46_js__ = (function() {
  "use strict";
  return {get x() {
      return $__test_47_unit_47_node_47_resources_47_x_46_js__.x;
    }};
})();
"use strict";
const {$__test_47_unit_47_node_47_resources_47_import_45_x_46_js_95_x__} = $__test_47_unit_47_node_47_resources_47_reexport_45_x_46_js__;
this.result = x;

and

tval out.js

gives

tval fails: ModuleEvaluationError: x is not defined
    at /work/traceur-compiler/out.js:16:15

The x on the RHS is not re-written with the munged name.

arv commented 8 years ago

I see what you are saying now.

arv commented 8 years ago

Hmmm... I'm not getting the same output as you:

const $__test_47_unit_47_node_47_resources_47_x_46_js__ = (function() {
  "use strict";
  var x = 'x';
  return {get x() {
      return x;
    }};
})();
const $__test_47_unit_47_node_47_resources_47_reexport_45_x_46_js__ = (function() {
  "use strict";
  return {get x() {
      return $__test_47_unit_47_node_47_resources_47_x_46_js__.x;
    }};
})();
"use strict";
const {x} = $__test_47_unit_47_node_47_resources_47_reexport_45_x_46_js__;
this.result = x;
johnjbarton commented 8 years ago

Oh, sorry I ran the command on a branch where I was looking into re-writing.

So finally I understand your "don't allow more than one fille": the non-root modules are wrapped in IIFE, but the files listed on the command line are not wrapped. That is why the emitted code has collisions.

I'll have to re-investigate why we can't apply this to Traceur.