visit flow types - fixes #108

[hzoo]

#108

[hzoo]

So then we need to remove this line? #89 (comment)

[sebmck]

This should do the trick. At least for very simple cases, although you can have deeply nested type annotations that this wont cover.

[hzoo]

So then we would need to do the same for say variable declaration type, parameter return type, etc? Yeah I guess cover the common cases?

// fails at the moment
it("flow type var declaration", function () {
      verifyAndAssertMessages([
          "import type Foo from 'foo';",
          "var x: Foo = 1;",
          "x;"
        ].join("\n"),
        { "no-unused-vars": 1 },
        []
      );
    });

[sebmck]

escope really is terrible for scope tracking. Yeah, those all will have to be visited too.

[hzoo]

Ok this is kind of crazy - didn't cover everything but yeah. Are there any github projects using flow to test this stuff on?

[wincent]

@hzoo: I'll ask in the internal Flow group at FB to see if anybody has any good open source examples.

[wincent]

Somebody suggested the React Native example apps (eg. https://github.com/facebook/react-native/tree/master/Examples/2048 etc).

[hzoo]

Ok cool I'l check it out - nice that it already has a .eslintrc too! (although rather old version) - maybe I'l do a PR.

[hzoo]

Maybe we should just push a simple change (already done in this PR) to fix just the function a(foo: Foo) { (the original issue) and work on supporting everything else in another commit?

[hzoo]

Ok rewrote most of it since there were a lot of random issues. Should of got most of the types in Other https://gist.github.com/hzoo/e3a7db9be7a3b0c2094e

---

To fix still: #66 again because t.isFlow(node) is added back.

// have to visit TypeAlias?
// no-undef ExampleClass
type ExampleClass = {
  Component: ReactClass<any, any, any>,
  title: string,
  description: string,
};
var exampleClasses: Array<ExampleClass> = [];

Seems like you either get no-undef on the TypeAlias or no-unused-vars on the flow type annotation.

---

// don't visit declare or remove nodes?
// no-undef uri, isStatic
declare module 'image!story-background' {
  declare var uri: string;
  declare var isStatic: boolean;
}

Should we be removing the declares? Then you would add all variables to globals in .eslintrc?

if (t.isDeclareModule(node)) {
      return this.remove();
    }

Ok removing the declares.

[hzoo]

Ok so I guess we want type x = any; to act like a variable declaration? Ok so found that escope just has a scope.variables and looked for scope.variables.push(), which lead me to currentScope().__define which creates a variable.

From https://github.com/estools/escope/blob/master/src/variable.js#L72-L79 I found different variable types so I'm just using Variable.Variable which is just "Variable"

And then found out you can just do referencer.prototype.TypeAlias after removing the old code to not visit it to create a new variable definition!

Still need to add all the tests as well as visit the TypeAlias.right...

It looks like there is use of type x = mixed, or _search(text: mixed) { - is that supposed to be a built in type like MixedTypeAnnotation? @sebmck

[sebmck]

@hzoo Nope. The mixed intype x = mixed;is just a normalGenericTypeAnnotation`.

[hzoo]

Ok I guess there's just a lot of types that aren't imported explicitly in each file itself like ReactComponent, ReactElement, ReactPropsCheckType, ReactPropsChainableTypeChecker, SyntheticEvent in the react-native examples/libraries so eslint will give an error of no-undef (I'm assuming flow itself has all of these globally or from a file outside).

Examples of no-undef

// $Enum
{
  property: $Enum<typeof Properties>;
}
// mixed
  _search(text: mixed) {
// D, P, S
function renderApplication<D, P, S>(
  RootComponent: ReactClass<D, P, S>,
  initialProps: P,
  rootTag: any
) {
// T
getNode: function<T>(id: T): T {
    return id;
  }

[hzoo]

I'm just going to add tests for everything next - do you think I need add/change anything at the moment @sebmck?

[sebmck]

Doesn't look too bad. Only comment would be on the use of .bind(this) which is a bit inefficient.

[hzoo]

Yeah ok, I should replace all those will just regular for loops.
And I guess update the PR based on the API changes you just did in #113?

Maybe the code would be better by refactoring it to use the visitor-keys data: https://github.com/babel/babel/blob/master/src/babel/types/visitor-keys.json#L79-L111?

[hzoo]

Ok refactored the visitTypeAnnotation method in https://gist.github.com/hzoo/15675611b9c7a65d3bb0. Ended up still being kind of long so don't know if that's a better idea or not since it's not as straightforward as before?

[sebmck]

@hzoo Looks nicer to me, easier to add additional types later too.

[hzoo]

Ok I'l go ahead and use that then. Is there a way to use t.VISITOR_KEYS but only for the flow types and ones with typeAnnotation? Or is it better to just make a copy like I already did?

[sebmck]

I guess you could loop over and filter them. require("babel-core").types.FLIPPED_ALIAS_KEYS.Flow contains all the flow nodes.

[hzoo]

Yeah don't know if it's that great

  var visitorKeysMap = pick(t.VISITOR_KEYS, function(k) {
    return t.FLIPPED_ALIAS_KEYS.Flow.concat([
      "ArrayPattern",
      "ClassDeclaration",
      "ClassExpression",
      "FunctionDeclaration",
      "FunctionExpression",
      "Identifier",
      "ObjectPattern",
      "RestElement"
    ]).indexOf(k) === -1;
  });

  // don't check "id" to prevent "no-undef" for 'Component' with: 'let x: React.Component'
  visitorKeysMap.QualifiedTypeIdentifier = ["qualification"];

[hzoo]

I added Type Annotations, Array Types, Tuples from the acorn flow tests https://gist.github.com/hzoo/c0ec600494025de9c192 and found/fixed a few issues because of them. Don't know if we need to do all of them but I can. @sebmck

---

For react-native, some variables aren't undefined so to fix I would add them to the .eslintrc globals or actual define them (so no issues there):

    "ReactClass": false,
    "ReactElement": false

Not sure about x: mixed;, easing: ($Enum<typeof AnimationUtils.Defaults> | EasingFunction);, or

function renderApplication<D, P, S>(
  RootComponent: ReactClass<D, P, S>,
 ...

I don't know if those are built-ins/temps for flow or something but it will also create an error (intended).

[sebmck]

Unsure if they all needed to be added, just do whatever you feel comfortable with. Also, it might be best to reuse as much as you can from babel.types.VISITOR_KEYS so if extensions are done to the node types in the future then they'll automatically work in babel-eslint.

[hzoo]

Ok I can add back something like the previous comment #109 (comment)?

[sebmck]

Sure, something like that would work.

[hzoo]

Ok added babel.types.VISITOR_KEYS, removed more .bind(this), and also trying to visit type casts
by removing change in #102 which fixes all the apparent issues I'm finding in react-native.

I think I'm ready to merge other than my confusion of if we need to do anything about var x: mixed since it will say it's not defined? @sebmck @wincent

[sebmck]

I think I'm ready to merge other than my confusion of if we need to do anything about var x: mixed since it will say it's not defined?

Seems fine to me. If this is actually an issue then it should be fixed by parsing it as a MixedTypeAnnotation node so there's nothing that needs to be handled on the babel-eslint side.

[hzoo]

Ok makes sense... so good to merge then? And would this still be a patch version?

[sebmck]

Looks GTM to me. Doesn't really matter what version it's released as since it's not breaking or "adding features" really.

[jwangnz]

Hi, I tried the 3.1.10, it seems that it will got no-undef on polymorphic functions.

/* @flow */

export function foo<X>(x: X): X {
  return x;
}

the code above will got three no-undef errors:

  3:20  error  "X" is not defined  no-undef
  3:26  error  "X" is not defined  no-undef
  3:30  error  "X" is not defined  no-undef

[hzoo]

I didn't figure out how to get polymorphic functions to work yet (stuff like <T>) - so, sorry I knew about those but still need to work on it.

[jwangnz]

Could bable-eslint remove this <T> stuff such as it did in the previous versions until it can properly get the polymorphic functions to work ?

[hzoo]

The previous versions removed all flow types entirely but then you would get (no-unused-vars).

Since they aren't going to refer to any types (Foo, React), we might as well remove them. Not sure how we can do this other than checking if the Identifier is a single capital letter (which really sounds like a hack) and deleting it (or not visiting based on that)? @sebmck

} else if (node.type === "Identifier") {
      // check if a polymorphic type: <T>, <A>, etc
      if (node.name.length === 1 && node.name === node.name.toUpperCase()) {
        return;
      }
      this.visit(node);
    } 

Could do a similar thing for mixed at the moment.

[hzoo]

Ok @Tsing see if 3.1.11 fixes things for you.

[jwangnz]

@hzoo Thanks, it works well!