Fix #4706: Flow generics

[GeoffreyBooth]

Fixes #4706. Placement of comments around function parameter parentheses and around class names is improved; top-level identifiers with trailing comments are wrapped in parentheses, as required by Flow to distinguish them from JavaScript labels.

@thejameskyle do you mind reviewing this? And especially testing it on a few non-trivial cases that you may have in your code. I think I covered all the cases you mentioned in #4706, except for the case of identifiers in an assignment wrapped in parentheses: other = (arr ###: any ###). I think other = arr: any is always a syntax error, never a JavaScript label, so does it need parentheses? But all the other cases are covered; see test/comments.coffee in this PR.

EDIT: Covered that last case.

[jamiebuilds]

This looks awesome :)

I think other = arr: any is always a syntax error, never a JavaScript label, so does it need parentheses?

I'm afraid Flow will only parse it as other = (arr: any), the parenthesis are always required.

[GeoffreyBooth]

Okay, while I ponder that do you mind testing the other cases?

[GeoffreyBooth]

Update: I figured out how to handle the other = arr/*: any */ case. I think they’re all covered now.

[stevefan1999-personal]

The use of Flow with CS2 is truly horrific. Nobody wants to see ### all around. Instead Facebook assumed developers to miss /* and */ magically...You are totally ruining this.

[jamiebuilds]

@stevefan1999 Sorry all these people are creating free stuff for you and expecting nothing in return.

Sorry people like @GeoffreyBooth and myself spend thousands of hours writing code, answering issues, writing documentation, speaking at conferences. Spending hundreds of thousands of dollars of our time and creating projects worth millions of dollars for which you pay nothing because we think it's the right thing to do. I can't imagine the pain you must be going through.

I have an idea for you: Don't use it.

@GeoffreyBooth Awesome, it's late now, but I'll take a look at this soon and try a bunch of stuff locally and report back.

[stevefan1999-personal]

@thejameskyle I'm not trying to taunt @GeoffreyBooth but I would ditch Flow if I'm ever going to see a lot of ###s, I'd call this "pure anti-pattern". And this would apply to probably everybody unless he/she is a sharps fanatic.

On the other hand, I'm sorry if I offended both you @thejameskyle @GeoffreyBooth

[GeoffreyBooth]

@connec Do you think there’s any concern about me removing this line? I removed it in order to not necessarily always optimize away redundant parentheses (which is what we’re dealing with in this case).

[connec]

@GeoffreyBooth why would we not want to optimize away redundant parentheses? Otherwise nothing jumps out at me!

[GeoffreyBooth]

Because of this particular case. When parentheses wrap an IdentifierLiteral that has a comment, we want those parentheses to be output because Flow requires them. See #4706 (comment).

I’m assuming that removing this optimization doesn’t break anything else, though, right?

[GeoffreyBooth]

@thejameskyle Thanks. I’m thinking it would be good to have a CoffeeScript file that included every part of the Flow comments-based syntax. Then I could test that the entire Flow syntax works by compiling that file to JS, then having Flow check it. This might become part of cake test:integrations.

But anyway that can be a separate pull request. If you can confirm that this PR works, I’ll merge it in, and if you have the time to generate such an example file, I’d love to include it in the project.

[GeoffreyBooth]

@thejameskyle Do you think you’ll have time to check this soon?

[GeoffreyBooth]

@thejameskyle or @connec, any notes on this? This is the last thing before we release 2.0.2.

[jamiebuilds]

Hey sorry, been out of commission the last two days. I was looking for the parser tests in Flow but couldn't find them. I'll check this out now.

[jamiebuilds]

test "#4706: Flow comments before class methods", ->
  eqJS '''
  class Container
    ###::
    method: (number) => string;
    method: (string) => number;
    ###
    method: -> true
  ''', '''
  var Container;

  Container = class Container {
    /*::
    method: (number) => string;
    method: (string) => number;
    */
    method() {
      return true;
    }

  };'''

test "#4706: Flow comments for class method params", ->
  eqJS '''
  class Container
    method: (param ###: string ###) -> true
  ''', '''
  var Container;

  Container = class Container {
    method(param/*: string */) {
      return true;
    }

  };'''


test "#4706: Flow comments for class method returns", ->
  eqJS '''
  class Container
    method: () ###: string ### -> true
  ''', '''
  var Container;

  Container = class Container {
    method()/*: string */ {
      return true;
    }

  };'''

Here's some more tests cases for class methods

[jamiebuilds]

There's also the question for how to handle class properties:

class Foo
  prop: 1;

Get's compiled to:

class Foo {}
Foo.prototype.prop = 1;

But for that to work in Flow it needs a type annotation:

class Foo {
  /*:: prop: number */
}
Foo.prototype.prop = 1;

The easiest way to do that would be to write it like this:

class Foo
  ###:: prop: number ###
  prop: 1;

But the output is currently:

class Foo {};

/*:: prop: number */
Foo.prototype.prop = 1;

[jamiebuilds]

Also with variables:

a ###: number ### = 1;

Generates:

var a;

a/*: number */ = 1;

But for that to work in Flow it should be:

var a/*: number */;

a = 1;

[jamiebuilds]

Another test case:

test "#4706: Flow comments for function spread", ->
  eqJS '''
  method = (...rest ###: Array<string> ###) =>
  ''', '''
  var method;

  method = (...rest/*: Array<string> */) => {};'''

[GeoffreyBooth]

Class properties are still an experimental syntax, so they're only supported via backticks currently. #4724. So within that backticked block you could add the comment.

So aside from that, I should add these tests you're proposing, which I suppose all pass? Or you're welcome to, I've given you access.

And then the only remaining thing for Flow is variable declarations? I'd rather tackle that in its own PR. Is everything in this one okay?

[GeoffreyBooth]

@thejameskyle I’ve added the tests you posted above. See my last comment for class properties, and I’ve opened #4747 to track fixing comments to support Flow for local variable declarations.

Anything else for this PR?

[GeoffreyBooth]

@thejameskyle Please see also #4753. Once these two PRs are merged in I’ll cut a new release. Please try them on your project and let me know how it goes?

[jamiebuilds]

I think this PR is good to ship :)