escope icon indicating copy to clipboard operation
escope copied to clipboard

ES6 module import tests relies on a version of esprima that doesn't follow estree spec

Open fczuardi opened this issue 10 years ago • 25 comments

If you look at https://github.com/estools/escope/blob/master/test/es6-import.coffee#L25 you see that the tests for es6 uses an esprima version that does not generate https://github.com/estree/estree spec compliant ast

For example:

ast.body[0].specifiers for code 'import v from "mod"' is expected to output:

 [ { type: 'ImportDefaultSpecifier', local: { type: 'Identifier', name: 'v' } } ]

and not

 [ { type: 'ImportDefaultSpecifier', id: { type: 'Identifier', name: 'v' } } ]

fczuardi avatar Mar 09 '15 19:03 fczuardi

by the way, there is no version of esprima at the moment that follows estree, the harmony branch is old and master is expected to merge https://github.com/eslint/espree changes soon according to comments on Freenode's IRC channel #esprima

fczuardi avatar Mar 09 '15 19:03 fczuardi

related: https://github.com/estools/escope/issues/33

fczuardi avatar Mar 09 '15 19:03 fczuardi

and the proper esprima issue might be this one: https://github.com/jquery/esprima/issues/1000

fczuardi avatar Mar 09 '15 19:03 fczuardi

Yeah, Espree is the first parser to implement the ESTree representation of ES6 modules.

nzakas avatar Mar 09 '15 19:03 nzakas

estraverse, esrecurse are now upgraded to suppert those types :) So next is escope :D

Constellation avatar Mar 09 '15 19:03 Constellation

Yay!

nzakas avatar Mar 09 '15 19:03 nzakas

if replaced with espree, lines such as this one https://github.com/estools/escope/blob/master/src/referencer.js#L117 must be updated to use node.local instead of node.id

fczuardi avatar Mar 09 '15 20:03 fczuardi

one possible fix to this bug:

diff --git a/package.json b/package.json
index f9ea500..9b45783 100644
--- a/package.json
+++ b/package.json
@@ -28,6 +28,7 @@
     "browserify": "^9.0.3",
     "chai": "^2.1.1",
     "coffee-script": "^1.9.1",
+    "espree": "^1.11.0",
     "esprima": "~1.2.2",
     "gulp": "~3.8.10",
     "gulp-babel": "^4.0.0",
diff --git a/src/referencer.js b/src/referencer.js
index 7b592ce..a237e43 100644
--- a/src/referencer.js
+++ b/src/referencer.js
@@ -108,20 +108,20 @@ class Importer extends esrecurse.Visitor {
     }

     ImportNamespaceSpecifier(node) {
-        if (node.id) {
-            this.visitImport(node.id, node);
+        if (node.local) {
+            this.visitImport(node.local, node);
         }
     }

     ImportDefaultSpecifier(node) {
-        this.visitImport(node.id, node);
+        this.visitImport(node.local, node);
     }

     ImportSpecifier(node) {
         if (node.name) {
             this.visitImport(node.name, node);
         } else {
-            this.visitImport(node.id, node);
+            this.visitImport(node.local, node);
         }
     }
 }
diff --git a/test/es6-import.coffee b/test/es6-import.coffee
index 11d8a6b..6e524b3 100644
--- a/test/es6-import.coffee
+++ b/test/es6-import.coffee
@@ -22,7 +22,7 @@
 #  THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

 expect = require('chai').expect
-harmony = require '../third_party/esprima'
+harmony = require 'espree'
 escope = require '..'

 describe 'import declaration', ->

fczuardi avatar Mar 09 '15 20:03 fczuardi

It would probably be easier to replace node.id with node.local || node.id

nzakas avatar Mar 09 '15 21:03 nzakas

true, here is a pull request: https://github.com/estools/escope/pull/52

fczuardi avatar Mar 09 '15 21:03 fczuardi

Other nodes, (exportXXX should be handled).

Constellation avatar Mar 10 '15 16:03 Constellation

@Constellation do you have a plan to release this fix?

ai avatar Mar 10 '15 17:03 ai

@nzakas, @ai

What do you think of updating the major number with these ES6 commits?

Constellation avatar Mar 10 '15 18:03 Constellation

@Constellation it will be more difficult for me personally, because I will be wait for eslint update (this issue is critical for me).

But we must do it, if we change the API. But maybe it is only a fix, because new behaviour was expected from current API?

ai avatar Mar 10 '15 18:03 ai

It looks like these changes are backwards compatible, so I don't think a major version bump is necessary.

nzakas avatar Mar 10 '15 18:03 nzakas

@nzakas, @ai

Ah, my concern is that, to align to estree that is actively developed, we will need to change/add/drop many nodes. So I'm worrying about that backporting becomes difficult. Maybe, this change can be easily ported to 2.0.x. So I'll pick it to 2.0.x now :)

Constellation avatar Mar 10 '15 18:03 Constellation

Oh yeah, I was just thinking about the changes for this issue, specifically. With all the node changes, it's probably best to do a major version bump.

nzakas avatar Mar 10 '15 18:03 nzakas

And I'll pick super support and I'll publish it as 2.0.7 soon! :D

Constellation avatar Mar 10 '15 18:03 Constellation

Awesome!

nzakas avatar Mar 10 '15 18:03 nzakas

Released as 2.0.7 :)

Constellation avatar Mar 10 '15 19:03 Constellation

Thanks!

nzakas avatar Mar 10 '15 20:03 nzakas

@Constellation I'm not sure what happened, but 2.0.7 seems to have a regression related to AssignmentPattern. See: https://github.com/eslint/eslint/issues/2001

nzakas avatar Mar 10 '15 23:03 nzakas

@nzakas

Ah, current escope is locked with estraverse 1.9.x because there's no exportxxx node support in estree. We need to implement all ExportXXX node before updating estraverse to 2.0.x.

Constellation avatar Mar 10 '15 23:03 Constellation

@nzakas

Maybe, to support ExportXXX, only modifying ExportSpecifier is sufficient I think. I'll create a patch and update the estraverse contained in escope.

Constellation avatar Mar 10 '15 23:03 Constellation

AssignmentPattern is done in b713c53fc5b23c8df3a947af8f91f58e93b78751. This is initial support of AssignmentPattern and TDZ scope issue is not fixed yet.

Constellation avatar Mar 11 '15 01:03 Constellation