From 253716dd49d2fef4c4f2d9678725de71f7dd76d3 Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Thu, 25 Oct 2018 05:44:16 -0700 Subject: [PATCH] Fix Flow generic comment positioning (#5290) * add test for ensuring that calls with zero arguments look right --- src/language-js/printer-estree.js | 51 +++++++++++++++++-- src/language-js/utils.js | 33 ++++++++---- .../__snapshots__/jsfmt.spec.js.snap | 27 ++++++++++ .../__snapshots__/jsfmt.spec.js.snap | 28 ++++++++++ .../babylon_only/class_with_generics.js | 11 ++++ .../flow_comments/babylon_only/jsfmt.spec.js | 1 + tests/flow_comments/generics.js | 11 ++++ 7 files changed, 148 insertions(+), 14 deletions(-) create mode 100644 tests/flow_comments/babylon_only/__snapshots__/jsfmt.spec.js.snap create mode 100644 tests/flow_comments/babylon_only/class_with_generics.js create mode 100644 tests/flow_comments/babylon_only/jsfmt.spec.js create mode 100644 tests/flow_comments/generics.js diff --git a/src/language-js/printer-estree.js b/src/language-js/printer-estree.js index 273dbd791..2bdd7f9d3 100644 --- a/src/language-js/printer-estree.js +++ b/src/language-js/printer-estree.js @@ -34,7 +34,10 @@ const insertPragma = require("./pragma").insertPragma; const handleComments = require("./comments"); const pathNeedsParens = require("./needs-parens"); const preprocess = require("./preprocess"); -const { hasFlowShorthandAnnotationComment } = require("./utils"); +const { + hasFlowAnnotationComment, + hasFlowShorthandAnnotationComment +} = require("./utils"); const { builders: { @@ -1115,6 +1118,19 @@ function printPathNoParens(path, options, print, args) { ]); } + // Inline Flow annotation comments following Identifiers in Call nodes need to + // stay with the Identifier. For example: + // + // foo /*:: */(bar); + // + // Here, we ensure that such comments stay between the Identifier and the Callee. + const isIdentifierWithFlowAnnotation = + n.callee.type === "Identifier" && + hasFlowAnnotationComment(n.callee.trailingComments); + if (isIdentifierWithFlowAnnotation) { + n.callee.trailingComments[0].printed = true; + } + // We detect calls on member lookups and possibly print them in a // special chain format. See `printMemberChain` for more info. if (!isNew && isMemberish(n.callee)) { @@ -1125,6 +1141,9 @@ function printPathNoParens(path, options, print, args) { isNew ? "new " : "", path.call(print, "callee"), optional, + isIdentifierWithFlowAnnotation + ? `/*:: ${n.callee.trailingComments[0].value.substring(2).trim()} */` + : "", printFunctionTypeParameters(path, options, print), printArgumentsList(path, options, print) ]); @@ -2876,7 +2895,27 @@ function printPathNoParens(path, options, print, args) { } case "TypeParameterDeclaration": - case "TypeParameterInstantiation": + case "TypeParameterInstantiation": { + const value = path.getValue(); + const commentStart = value.range + ? options.originalText.substring(0, value.range[0]).lastIndexOf("/*") + : -1; + // As noted in the TypeCastExpression comments above, we're able to use a normal whitespace regex here + // because we know for sure that this is a type definition. + const commentSyntax = + commentStart >= 0 && + options.originalText.substring(commentStart).match(/^\/\*\s*::/); + if (commentSyntax) { + return concat([ + "/*:: ", + printTypeParameters(path, options, print, "params"), + " */" + ]); + } + + return printTypeParameters(path, options, print, "params"); + } + case "TSTypeParameterDeclaration": case "TSTypeParameterInstantiation": return printTypeParameters(path, options, print, "params"); @@ -6025,7 +6064,13 @@ function willPrintOwnComments(path) { const parent = path.getParentNode(); return ( - ((node && (isJSXNode(node) || hasFlowShorthandAnnotationComment(node))) || + ((node && + (isJSXNode(node) || + hasFlowShorthandAnnotationComment(node) || + (parent && + parent.type === "CallExpression" && + (hasFlowAnnotationComment(node.leadingComments) || + hasFlowAnnotationComment(node.trailingComments))))) || (parent && (parent.type === "JSXSpreadAttribute" || parent.type === "JSXSpreadChild" || diff --git a/src/language-js/utils.js b/src/language-js/utils.js index 112255460..b8a7a946f 100644 --- a/src/language-js/utils.js +++ b/src/language-js/utils.js @@ -1,5 +1,20 @@ "use strict"; +// We match any whitespace except line terminators because +// Flow annotation comments cannot be split across lines. For example: +// +// (this /* +// : any */).foo = 5; +// +// is not picked up by Flow (see https://github.com/facebook/flow/issues/7050), so +// removing the newline would create a type annotation that the user did not intend +// to create. +const NON_LINE_TERMINATING_WHITE_SPACE = "(?:(?=.)\\s)"; +const FLOW_SHORTHAND_ANNOTATION = new RegExp( + `^${NON_LINE_TERMINATING_WHITE_SPACE}*:` +); +const FLOW_ANNOTATION = new RegExp(`^${NON_LINE_TERMINATING_WHITE_SPACE}*::`); + function hasFlowShorthandAnnotationComment(node) { // https://flow.org/en/docs/types/comments/ // Syntax example: const r = new (window.Request /*: Class */)(""); @@ -8,19 +23,15 @@ function hasFlowShorthandAnnotationComment(node) { node.extra && node.extra.parenthesized && node.trailingComments && - // We match any whitespace except line terminators because - // Flow annotation comments cannot be split across lines. For example: - // - // (this /* - // : any */).foo = 5; - // - // is not picked up by Flow (see https://github.com/facebook/flow/issues/7050), so - // removing the newline would create a type annotation that the user did not intend - // to create. - node.trailingComments[0].value.match(/^(?:(?=.)\s)*:/) + node.trailingComments[0].value.match(FLOW_SHORTHAND_ANNOTATION) ); } +function hasFlowAnnotationComment(comments) { + return comments && comments[0].value.match(FLOW_ANNOTATION); +} + module.exports = { - hasFlowShorthandAnnotationComment + hasFlowShorthandAnnotationComment, + hasFlowAnnotationComment }; diff --git a/tests/flow_comments/__snapshots__/jsfmt.spec.js.snap b/tests/flow_comments/__snapshots__/jsfmt.spec.js.snap index 6b3ebd347..c62342264 100644 --- a/tests/flow_comments/__snapshots__/jsfmt.spec.js.snap +++ b/tests/flow_comments/__snapshots__/jsfmt.spec.js.snap @@ -45,6 +45,33 @@ function foo(bar /*: T[] */, baz /*: T */) /*: S */ {} `; +exports[`generics.js - flow-verify 1`] = ` +const Component = branch/*:: */( + ({ src }) => !src, + // $FlowFixMe + renderNothing, +)(BaseComponent); + +const C = b/*:: */(foo) + c/*:: */(bar); + +foo/*::*/(baz); + +foo/*::*/(); +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +const Component = branch/*:: */( + ({ src }) => !src, + // $FlowFixMe + renderNothing +)(BaseComponent); + +const C = b/*:: */(foo) + c/*:: */(bar); + +foo/*:: */(baz); + +foo/*:: */(); + +`; + exports[`let.js - flow-verify 1`] = ` let foo /*: Groups */; let foo /*: string */ = 'a'; diff --git a/tests/flow_comments/babylon_only/__snapshots__/jsfmt.spec.js.snap b/tests/flow_comments/babylon_only/__snapshots__/jsfmt.spec.js.snap new file mode 100644 index 000000000..216786ceb --- /dev/null +++ b/tests/flow_comments/babylon_only/__snapshots__/jsfmt.spec.js.snap @@ -0,0 +1,28 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`class_with_generics.js - babylon-verify 1`] = ` +import React from 'react'; + +/*:: type Props = { + foo?: ?string, + bar: number, +}; */ + +/*:: type State = { baz: number }; */ + +class Component extends React.Component/*:: */ { +} +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +import React from "react"; + +/*:: type Props = { + foo?: ?string, + bar: number, +}; */ + +/*:: type State = { baz: number }; */ + +class Component extends React.Component /*:: */ { +} + +`; diff --git a/tests/flow_comments/babylon_only/class_with_generics.js b/tests/flow_comments/babylon_only/class_with_generics.js new file mode 100644 index 000000000..3bc8df04c --- /dev/null +++ b/tests/flow_comments/babylon_only/class_with_generics.js @@ -0,0 +1,11 @@ +import React from 'react'; + +/*:: type Props = { + foo?: ?string, + bar: number, +}; */ + +/*:: type State = { baz: number }; */ + +class Component extends React.Component/*:: */ { +} diff --git a/tests/flow_comments/babylon_only/jsfmt.spec.js b/tests/flow_comments/babylon_only/jsfmt.spec.js new file mode 100644 index 000000000..968651cdb --- /dev/null +++ b/tests/flow_comments/babylon_only/jsfmt.spec.js @@ -0,0 +1 @@ +run_spec(__dirname, ["babylon"]); diff --git a/tests/flow_comments/generics.js b/tests/flow_comments/generics.js new file mode 100644 index 000000000..183ff8a89 --- /dev/null +++ b/tests/flow_comments/generics.js @@ -0,0 +1,11 @@ +const Component = branch/*:: */( + ({ src }) => !src, + // $FlowFixMe + renderNothing, +)(BaseComponent); + +const C = b/*:: */(foo) + c/*:: */(bar); + +foo/*::*/(baz); + +foo/*::*/();