Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type annotations in comments? #3

Closed
mhart opened this issue Nov 18, 2014 · 69 comments
Closed

Type annotations in comments? #3

mhart opened this issue Nov 18, 2014 · 69 comments

Comments

@mhart
Copy link

mhart commented Nov 18, 2014

Didn't see support for this in the documentation, but it would be great if Flow supported type annotations in comments (or similar) – with the aim of allowing everything to be written in pure JavaScript.

This would allow people to keep the same tools they currently use, and skip the transpilation step.

Has this idea been floated at all? Is it doable?

Edit:
Looks like this is doable – perhaps the documentation just needs to be updated to expound on this?
Edit again:
OK, so atm it's just conversion from docblock to flow – would be great if it could be done in-place... or something

@Raynos
Copy link
Contributor

Raynos commented Nov 18, 2014

👍 Reading https://github.com/facebook/flow/blob/master/src/typing/comments_js.ml it looks like its supported.

@benjamingr
Copy link

+1 for this. Would definitely be helpful in evaluating migration.

@mhart
Copy link
Author

mhart commented Nov 18, 2014

@Raynos Oh cool – just didn't see it mentioned in the docs (maybe it's not yet?)

@mhart
Copy link
Author

mhart commented Nov 18, 2014

Oooh, think I found some tests: https://github.com/facebook/flow/blob/master/tests/docblock/docblock.js

Might change this to be a documentation issue (edit: done)

@Raynos
Copy link
Contributor

Raynos commented Nov 18, 2014

cool :) Sounds like we just need documentation for this.

@arthurschreiber
Copy link
Contributor

This is not a documentation issue. flow port can convert your code that uses "docblock" based annotation into the flow annotation syntax:

/* @flow */

/**
  @param {string} x
 */
function foo(x) {
  return x*10;
}

foo("Hello, world!");

will give the following output:

$ ./flow port examples/01_HelloWorld/hello.js 
/Users/arthur/Downloads/flow/examples/01_HelloWorld/hello.js
--- old
+++ new
@@ -3,7 +3,7 @@
 /**
   @param {string} x
  */
-function foo(x) {
+function foo(x: string) {
   return x*10;
 }

But these annotations can't be directly used for type checking. But in theory, it should be possible to change flow to do this conversion in-place.

@mhart
Copy link
Author

mhart commented Nov 18, 2014

Ah cool, makes sense.

Ok – will keep this issue open then

@Raynos
Copy link
Contributor

Raynos commented Nov 18, 2014

Ah I see. Fair enough.

@avikchaudhuri
Copy link
Contributor

Hah, at Facebook we have existing code using a comment syntax that we try to convert into Flow annotations. This feature should be treated as experimental, not sure if our comments syntax is anything close to standard (jsdoc?)

@cletusw
Copy link

cletusw commented Nov 19, 2014

It just makes so much more sense to me to add these type annotations in comments. Then you can reuse existing comment-stripping build tools instead of inventing a new one. The only benefit I can see of using a new syntax is that it forces the compile step (and its accompanying type check).

@gabelevi
Copy link
Contributor

Comments are kind of a parsing nightmare. Knowing which node a comment belongs to is a Hard Problem, and often is solved by "guessing" based on whitespace. The way around this is to stick to conventions, but then you need something to enforce these conventions or risk silently misbehaving when they're broken.

So yeah, adding type annotations to the grammar simplifies things, though it does add a build step. It's a definite tradeoff, though.

@cletusw
Copy link

cletusw commented Nov 19, 2014

Hmm, what if you ignored jsdoc comments and only respected inline comments, like here? That shouldn't be too hard to parse, right? You could potentially even give a warning for any comment inlined that way that didn't parse correctly to prevent silent misbehavior. Or if that inline comment syntax is already used in lots of places maybe require a "flow:" prefix, e.g. function foo(/*flow:string*/ text, /*flow:number*/ count) /*flow:string*/ {}

@cletusw
Copy link

cletusw commented Nov 19, 2014

In other words, why is

function foo(a: mixed, b: number): void { ... }
var x: boolean;
class Bar {
  y: string;
}

parseable, but not

function foo(a /*: mixed*/, b /*: number*/) /*: void*/ { ... }
var x /*: boolean*/;
class Bar {
  y /*: string*/;
}

?

(Not that I recommend that exact syntax. Just wondering :-)

@Raynos
Copy link
Contributor

Raynos commented Nov 19, 2014

Having restrict rules around comments is fine.

// foo(a: mixed, b: mixed) : void
function foo(a, b) {
  // ...
}

The real problem in parsing comes when you combine the type definition and documentation in one comment block.

Supporting just type definitions in a single comment statement where the author of the type definition is not allowed to leave "comments" in the comments or "documentation" in the comments makes this problem far simpler.

We can also use a flow: prefix as mentioned above and warn about any flow: comments that were not parsed properly.

@concavelenz
Copy link

It is actually fairly straightforward for type annotations. You can steal
the logic from the closure compiler parser.
On Nov 18, 2014 4:14 PM, "Gabe Levi" [email protected] wrote:

Comments are kind of a parsing nightmare. Knowing which node a comment
belongs to is a Hard Problem, and often is solved by "guessing" based on
whitespace. The way around this is to stick to conventions, but then you
need something to enforce these conventions or risk silently misbehaving
when they're broken.

So yeah, adding type annotations to the grammar simplifies things, though
it does add a build step. It's a definite tradeoff, though.


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

@samreid
Copy link

samreid commented Nov 19, 2014

I agree it would be very useful for flow to support @param annotations (as an alternative to the provided :type syntax) in order to deduce the types. This will make it significantly easier for existing projects to benefit from Flow, and will avoid the transpiler step for live code + the compilation step for minified code.

@dchambers
Copy link

Avoiding having to use a transpiler is a big deal in making this usable on existing projects, and while I think @cletusw's suggestion was also really neat, for example:

function foo(a /*: string*/, b /*: number*/) /*: number*/ {
  // function body...
}

in terms of working with existing idiomatic JavaScript, which is one of Flow's stated goals, then supporting jsdoc annotations would be a huge win also, so:

/**
 * @param {String} a
 * @param {Number} b
 * @returns {Number}
 */
function foo(a, b) {
  // function body...
}

@j201
Copy link

j201 commented Nov 19, 2014

Using comment type annotations might be necessary if using simple compile steps like browserify or sweet.js, where JS type annotations are relevant but adding them outside of comments would break the compile steps.

@jareware
Copy link

@cletusw's suggestion, and the rest of the arguments on this issue are extremely sound. For a bit more typing (and maybe a tad uglier code), you could get all the benefits of Flow without having to leave standard JavaScript syntax compatibility. This is a HUGE win in terms of bringing existing projects onboard. Also, many people may not want to make the commitment even in greenfield projects, as going over to Flow (or TypeScript) will need support from most other tools and IDE's you might want to use, and will massively limit the ecosystem available to you. Finally, as stated above, this aligns perfectly with Flow's stated goal of being able to work with any JS, and helping out with type annotations only gradually and when needed.

As to the concrete proposal, I would go even further and drop the colons:

/* @flow */
function foo(a /*mixed*/, b /*number*/) /*void*/ { ... }

...as it's highly unlikely inline comments would exist at those exact locations unless they're meant for this exact purpose.

If @avikchaudhuri or @gabelevi could comment as to how likely adding a 100% JS compatible syntax to the project would be, that'd be hugely appreciated! Because if not, someone in the community can take up building a pre-processor for instance.

@benjamingr
Copy link

+1 for @Raynos 's suggestion. I think it's very clear and allows smoother migration.

@andrewrota
Copy link

+1, I agree that this would be an awesome feature with whichever comment syntax was most feasible.

Just to add another possible syntax, the inline Closure Compiler annotations look pretty similar to flow except for the return type before the function rather than after. I don't know if the logic from Closure Compiler to parse this would be helpful at all in porting the capability to Flow, though.

function /** string */ foo(/** string */ a, /** number */ b) {}

Thanks for considering this feature!

@darrenderidder
Copy link

+1, I made the same comment on HN when the announcement came out. Annotations belong in annotations (aka. comments); transpiling is quixotic.

@bcherny
Copy link

bcherny commented Nov 27, 2014

@Raynos @jareware @andrewrota

what's the benefit of using a new, custom syntax as opposed to already widely adopted jsdoc?

/**
 * Foo
 * @param {String} foo
 * @param {Number} bar
 * @return {Array} An array
 */
function foo (foo, bar) {}

Or (not sure if flow is this smart):

/**
 * Bar
 * @param  {Array} foo An array
 * @param  {Array<Array>} bar An array of arrays
 * @param  {DOMElement} baz An element
 * @return {Promise}
 */
function bar (foo, bar, baz) {}

@andrewrota
Copy link

@bcherny, I think JSDoc syntax would be great, I just thought it would be mentioning the more inline syntax (especially since Flow's type annotations are inline). Closure Compiler supports both the JSDoc style and the inline comment style.

@Raynos
Copy link
Contributor

Raynos commented Nov 27, 2014

@bcherny

The benefit is that the JSDoc language is different from the flow language.

Using something that looks like JSDoc but is not JSDoc is confusing.

With that said you might as well use a comment language that maps as cleanly onto flow as possible. This means you have zero legacy baggage from JSDoc and zero legacy baggage from JavaDoc.

Coupling yourself to JSDoc has no benefits and just limits flow.

@bcherny
Copy link

bcherny commented Nov 27, 2014

@Raynos I already use jsdoc for docs and automated interface testing, so my ideal workflow would be to continue using jsdoc, and add in flow as a static typechecking tool that makes use of my existing jsdoc annotations.

The parser is already decoupled from the rest of flow's source, and it would be cool to have a few different parsers available (flow, jsdoc, closure, etc.).

Coupling yourself to JSDoc has no benefits and just limits flow.

Can you explain this a bit more?

@Raynos
Copy link
Contributor

Raynos commented Nov 27, 2014

@bcherny

JSDoc has no way to express overloaded functions. i.e.

((x: number) => void) & ((x: string) => void)

Trying to express this syntax in the existing "framework" of a comment block with a list of new line seperate @param statements is going to be frustrating.

What you really want is a one line comment above the function that is the exact same as the flow language itself.

@bcherny
Copy link

bcherny commented Nov 27, 2014

@Raynos By "overloaded" you mean a function which can return multiple types, with params that might accept multiple types? Jsdoc does support that use case, but you're right that it does not support mapping inputs to outputs.

/**
 * @param {String|Number} foo
 * @param {Any} bar
 * @return {Array|Object} An array or an object
 */
function (foo, bar) {}

@mhart
Copy link
Author

mhart commented Feb 21, 2015

Yessss, excellent news!

@mhart
Copy link
Author

mhart commented Feb 21, 2015

Going to close this – if anyone's got issues with the syntax or anything else they can open up separate issues.

@mhart mhart closed this as completed Feb 21, 2015
@jareware
Copy link

This is brilliant! 🙇

@quantuminformation
Copy link

Good job!

@ngduc
Copy link

ngduc commented Jan 15, 2016

flotate looks great! Please support JSDoc syntax also. Thanks.

@kamek-pf
Copy link

JSDoc would be nice indeed.
What's the status on this ? Is it going to happen at all ?

@metrofun
Copy link

+1
JSDoc would be the right direction for increased interoperability of Flow. Having one more magic syntax for types is quite obtrusive. Right now JSDoc is a clear advantage of Closure Compiler when it comes to type checking.

@quantuminformation
Copy link

Any update on jsdoc being wired to flow?

@ngduc
Copy link

ngduc commented Apr 13, 2016

Does flow support flotate one line syntax yet? I haven't found an example online:

/* @flow */
/*: (x: string, y: number): boolean */
function foo(x, y) {}

@jareware
Copy link

@ngduc, I don't think it was ever slated for inclusion. And to be fair, while personally I find that one-line syntax very convenient, it's quite different from the standard Flow syntax, and actually isn't as widely applicable as the standard one.

If the team were to consider the addition of completely new syntaxes, I'm guessing JSDoc support would be the first thing to shoot for.

@ronkorving
Copy link

I for one would love to see support for the flotate syntax that @ngduc refers to there. It's the one thing I hoped I could use (the inline comments are a bit messy imho), and would be a fair replacement for JSDoc (for some of my use cases), and much more compact.

@rrrnld
Copy link

rrrnld commented Aug 15, 2016

There's support for converting existing code using jsdoc syntax into the flow syntax, which could then be used for checking, is that correct? Does that conversion handle modules? And how does it handle mixed code where some files are written using the flow syntax and some use jsdoc?

If all that could work seamlessly it would be a killer feature, enabling correct autocompletion and type-checking for existing well-documented libraries, so I could just import foo from 'external/library' and have the documentation right in my editor. 🚀

@dgreensp
Copy link

I'd like to echo what @ronkorving said. Being able to annotate a function with a single flow comment would be a huge benefit and make it more likely my team would adopt Flow. Flow comments (as opposed to real annotations) are super useful not just to avoid preprocessing but also to not block on editor support on a diverse team.

The expectation that this would be implemented comes from the fact that, in theory, flotate has been deprecated and its functionality folded into Flow, but this aspect of flotate apparently did not make it over.

@kegsay
Copy link

kegsay commented Sep 6, 2016

@heyarne :

There's support for converting existing code using jsdoc syntax into the flow syntax, which could then be used for checking, is that correct?

As far as I know, the flow binary does not support this. However, I have a project which will do this conversion for you: Kegsay/flow-jsdoc.

Does that conversion handle modules?

No, it doesn't add in modules which are required for the type checking to work. It's an addition I want to add in the future though. The tool can operate on an entire directory, but each file is currently treated in isolation.

And how does it handle mixed code where some files are written using the flow syntax and some use jsdoc?

Badly. Esprima doesn't support Flowtype syntax last time I checked, so the converter won't think it is valid JS.

@Pyrolistical
Copy link

I agree with @dgreensp. If support was added for a single line comment above the functions I would propose my team to use flow

@neemzy
Copy link

neemzy commented Sep 20, 2016

Hi,

May I ask whether you guys are considering parsing/supporting JSDoc comments to infer type info from it? Using yet another external tool's kinda cumbersome, it means copying my whole code to a third place (adding to original source and browser-ready code) just for the sake of type-checking it, and it also has its own drawbacks (I can only imagine resolving require calls when the files aren't in their original location anymore is a kinda small but still very real PITA, and that's only the first thing off the top of my head).

@kegsay's tool looks nice, but doesn't support JSX for starters, which is something I need: using it right now would mean setting up a different flavor of my JS build to compile JSX but not remove comments or anything, copy it to a specific location while converting JSDoc to Flow syntax with said tool, and if everything is OK, run my standard build to generate browser-ready code as I already do. I love JS tooling, but this is one step too far for me.

My codebase is full of JSDoc, which indeed is the language's standard type notation. It may not cover everything your own syntax does, but will do the job for a lot of use cases. C'mon, Facebook fellas wrote brand new PHP specs, don't tell me you can't contribute to JSDoc ;) (jk ofc)

I have no idea of the required amount of work this needs, but I'm positive this is the way to go if you want existing, large codebases to take the leap (I want mine to anyway).

@kegsay
Copy link

kegsay commented Sep 21, 2016

For people who want a 1-line comment of the form:

/*: (x: string, y: number): boolean */
function(x, y) {
 // ...
}

Presumably this is desirable due to compactness. If so, presumably the following would be even better (else you need to make sure if you rename a variable that you also rename the comment form):

/*: (string, number): boolean */
function(x, y) {
 // ...
}

If there is enough interest in a one-line form, I should be able to very easily fold this behaviour into flow-jsdoc given I'm already parsing comments at this stage. This would effectively map:

/*: (string, number): boolean */
function(x, y) {
 // ...
}

Into:

/*: (string, number): boolean */
function(x: string, y: number) : boolean {
 // ...
}

@Pyrolistical
Copy link

Pyrolistical commented Sep 21, 2016

@kegsay I love it! We can even make it more compact by introducing a single line comment syntax:

//: (string, number): boolean
function(x, y) {
 // ...
}

@dgreensp
Copy link

My only suggestion on the flow-jsdoc front is consider that Flow could one day support flotate one-line syntax. The currently proposed flow-jsdoc feature (which is probably best discussed in that repo, not here) is yet another meaning of /*: and would cause trouble in that case.

@samreid
Copy link

samreid commented Sep 22, 2016

The preceding one-line syntax examples are misleading because they lack documentation. For instance, a documented version of the preceding example might be something like:

  /**
   * Makes a prediction using decision trees whether a person would have survived a trip on the Titanic.
   * @param {string} x - last name of a person who rode on the Titanic (case insensitive)
   * @param {number} y - age of the person who rode on the Titanic in years
   * @return {boolean} - true if the prediction from the model is survival
   */
  //: (string, number): boolean
  function(x, y){
    // ...
  }

The one-line syntax (or even flow annotations) is redundant, let's comment our code and add JSDoc type checking for Flow.

@kegsay
Copy link

kegsay commented Sep 22, 2016

@samreid That is exactly the rationale behind flow-jsdoc. The one line syntax is intended as an alternative, I have no desire to replace the exisiting logic to parse JSDoc. One line syntax works particularly well for private methods which people may not be documenting in JSDoc.

@Pyrolistical I would probably support both forms, as I find it somewhat magical if a tool treats block/line comments differently to each other when the actual language doesn't.

As for "Flow may one day sprout X", I'll cross that bridge if it ever happens. Worst case is a major version bump and a disclaimer that it only works on Flow vX and above. I'm not going to do nothing just because of speculative features.

@samreid
Copy link

samreid commented Sep 22, 2016

@kegsay I agree with everything you said, keep up the good work!

@kegsay
Copy link

kegsay commented Sep 24, 2016

flow-jsdoc now supports inline syntax of the form:

//: (string): Object
function foo(bar) {
  return {};
}

Parameters are paired in order and will currently panic if there are mismatched lengths. I'll probably add a flag to make that optional (and it just won't add types to subsequent params / ignore additional types in comment).

@kegsay
Copy link

kegsay commented Oct 4, 2016

@kegsay's tool looks nice, but doesn't support JSX

Does now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests