FlowType "declare" bug: 'isSequenceExpression' of undefined

[klvnptr]

How to reproduce it.
Create an empty node package with the following package.json

{
  "name": "babel-core-bug",
  "version": "1.0.0",
  "description": "Bug report",
  "main": "index.js",
  "devDependencies": {
    "babel": "^5.5.8",
    "babel-eslint": "^3.1.15",
    "eslint": "^0.23.0"
  }
}

Run in the console:

npm install

Create an .eslintrc file with the following:

{
  "parser": "babel-eslint"
}

Create a index.js file with the following:

"use strict";

declare class Native {
  open():Response;
  close():Response;
}

Finally run eslint:

node_modules/eslint/bin/eslint.js index.js

And you will get the following:

main.js
  0:0  error  Cannot read property 'isSequenceExpression' of undefined

✖ 1 problem (1 error, 0 warnings)

Which is kind of odd. Am I missing something?

[hzoo]

No, I don't think you are. I need to look into this; I don't think I did the declare stuff right.

[klvnptr]

Thanks @hzoo

[hzoo]

Oh this is that isSequenceExpression thing again with parent is undefined (because of this.dangerouslyRemove)

If I remove

if (this.isDeclareModule() ||
        this.isDeclareClass() ||
        this.isDeclareFunction() ||
        this.isDeclareVariable()) {
      return this.dangerouslyRemove();
    }

then the error "A" is not defined no-undef will occur until I add all the declare's as scope variables as well (didn't do it last time with the flow PR) which I can do

[klvnptr]

Awesome. Thank you. I'm looking forward to this in the next version released.

[hzoo]

Ok should be fixed in 3.1.16

[klvnptr]

Thanks @hzoo. But there are still some issues. Paste the following in index.js:

declare class Native {
  open():Response;
  close():Response;
}

var test:Native;
test = test;

You gonna get this error:

index.js
  6:9  error  "Native" is not defined  no-undef

✖ 1 problem (1 error, 0 warnings)

Which is not true, since it was already declared.

[hzoo]

Ok I did the wrong thing - will fix soon.

[hzoo]

@mcz Can you test again on master?

I'm only adding a check for the top level identifier (Native in your case) and not doing anything for open():Response;, although thinking about it now I probably should for classes/modules.

[klvnptr]

If you only put this into the index.js file:

declare class Native {
  open():Response;
  close():Response;
}

Than it will be complaining:

declare.js
  1:14  error  Native is defined but never used  no-unused-vars

Which is not good, since declarations can be used standalone in Flowtype:
http://flowtype.org/docs/declarations.html

On Wed, Jun 17, 2015 at 3:20 PM, Henry Zhu notifications@github.com wrote:

@mcz https://github.com/mcz Can you test again on master?

I'm only adding a check for the top level identifier (Native in your case)
and not doing anything for open():Response;, although thinking about it
now I probably should for classes/modules.

—
Reply to this email directly or view it on GitHub
#132 (comment).

[hzoo]

Well it's kind of an issue @mcz - no-undef and no-unused-vars are related - you basically get either error at this point.

I think that's why I ignored the flow declarations in the first place.

If I create a variable for it - then it will complain it's not used.
If I don't, it will complain it wasn't a variable when you use it.

Unless somehow we check if there's only declarations

[klvnptr]

Okay @hzoo. The point is it doesn't throw compiler error anymore. We ignore these declaration files in .eslintignore file.

[hzoo]

Ok should I do a new release for the change in master? I think we want to have the no-unused-vars error rather than the no-undef one. That way you can declare stuff and use it in the same file - so the only files that will error are files that only contain flow declarations (which can be ignored like you said?)

[zertosh]

I made eslint-plugin-flow-vars to deal with this.

An eslint plugin that makes flow type annotations global variables and marks declarations as used. Solves the problem of false positives with no-undef and no-unused-vars when using babel-eslint.

[hzoo]

@zertosh cool! added it to the readme