From 0dc4755920ae46e32e104d144787938d10563217 Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Thu, 19 Oct 2017 06:56:59 -0700 Subject: [PATCH] Fix #4747: Flow local variables (#4753) * Fix #4706: Comments before a PARAM_START token stay before that token * Simplify nodes * Add function-in-function test * Fix #4706: Comments after class name should go after the identifier that's after `class`, not the variable assigned to * Fix #4706: Top-level identifiers with trailing comments get wrapped in parentheses (around the comment too) so that Flow doesn't interpret it as a JavaScript label * Cleanup * If the source has parentheses wrapping an identifier followed by a block comment, output those parentheses rather than optimizing them away; this is a requirement of Flow, to distinguish from JavaScript labels * More tests for Flow comments * For local variables with trailing inline herecomments, output the comments up in the variable declarations line for Flow compatibility --- lib/coffeescript/nodes.js | 38 +++++++++++++++++++++++++++++++++----- lib/coffeescript/scope.js | 4 +++- src/nodes.coffee | 25 +++++++++++++++++++++++-- src/scope.litcoffee | 4 +++- test/comments.coffee | 34 ++++++++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 9 deletions(-) diff --git a/lib/coffeescript/nodes.js b/lib/coffeescript/nodes.js index 328d1c21..46c0d216 100644 --- a/lib/coffeescript/nodes.js +++ b/lib/coffeescript/nodes.js @@ -237,6 +237,9 @@ // `compileToFragments` method has logic for outputting comments. unshiftCommentFragment(commentFragment); } else { + if (fragments.length === 0) { + fragments.push(this.makeCode('')); + } if (commentFragment.unshift) { if ((base1 = fragments[0]).precedingComments == null) { base1.precedingComments = []; @@ -775,7 +778,7 @@ // Compile the expressions body for the contents of a function, with // declarations of all inner variables pushed up to the top. compileWithDeclarations(o) { - var assigns, declars, exp, fragments, i, j, len1, post, ref1, rest, scope, spaced; + var assigns, declaredVariable, declaredVariables, declaredVariablesIndex, declars, exp, fragments, i, j, k, len1, len2, post, ref1, rest, scope, spaced; fragments = []; post = []; ref1 = this.expressions; @@ -806,7 +809,17 @@ } fragments.push(this.makeCode(`${this.tab}var `)); if (declars) { - fragments.push(this.makeCode(scope.declaredVariables().join(', '))); + declaredVariables = scope.declaredVariables(); + for (declaredVariablesIndex = k = 0, len2 = declaredVariables.length; k < len2; declaredVariablesIndex = ++k) { + declaredVariable = declaredVariables[declaredVariablesIndex]; + fragments.push(this.makeCode(declaredVariable)); + if (Object.prototype.hasOwnProperty.call(o.scope.comments, declaredVariable)) { + fragments.push(...o.scope.comments[declaredVariable]); + } + if (declaredVariablesIndex !== declaredVariables.length - 1) { + fragments.push(this.makeCode(', ')); + } + } } if (assigns) { if (declars) { @@ -3208,7 +3221,7 @@ this.variable.error(`'${this.variable.compile(o)}' can't be assigned`); } varBase.eachName((name) => { - var message; + var commentFragments, commentsNode, message; if (typeof name.hasProperties === "function" ? name.hasProperties() : void 0) { return; } @@ -3216,14 +3229,29 @@ if (message) { name.error(message); } - // `moduleDeclaration` can be `'import'` or `'export'` + // `moduleDeclaration` can be `'import'` or `'export'`. this.checkAssignability(o, name); if (this.moduleDeclaration) { return o.scope.add(name.value, this.moduleDeclaration); } else if (this.param) { return o.scope.add(name.value, this.param === 'alwaysDeclare' ? 'var' : 'param'); } else { - return o.scope.find(name.value); + o.scope.find(name.value); + // If this assignment identifier has one or more herecomments + // attached, output them as part of the declarations line (unless + // other herecomments are already staged there) for compatibility + // with Flow typing. Don’t do this if this assignment is for a + // class, e.g. `ClassName = class ClassName {`, as Flow requires + // the comment to be between the class name and the `{`. + if (name.comments && !o.scope.comments[name.value] && !(this.value instanceof Class) && name.comments.every(function(comment) { + return comment.here && !comment.multiline; + })) { + commentsNode = new IdentifierLiteral(name.value); + commentsNode.comments = name.comments; + commentFragments = []; + this.compileCommentFragments(o, commentsNode, commentFragments); + return o.scope.comments[name.value] = commentFragments; + } } }); } diff --git a/lib/coffeescript/scope.js b/lib/coffeescript/scope.js index 97ef10cb..ad7ca8b1 100644 --- a/lib/coffeescript/scope.js +++ b/lib/coffeescript/scope.js @@ -14,7 +14,8 @@ // as well as a reference to the **Block** node it belongs to, which is // where it should declare its variables, a reference to the function that // it belongs to, and a list of variables referenced in the source code - // and therefore should be avoided when generating variables. + // and therefore should be avoided when generating variables. Also track comments + // that should be output as part of variable declarations. constructor(parent, expressions, method, referencedVars) { var ref, ref1; this.parent = parent; @@ -27,6 +28,7 @@ type: 'arguments' } ]; + this.comments = {}; this.positions = {}; if (!this.parent) { this.utilities = {}; diff --git a/src/nodes.coffee b/src/nodes.coffee index 6cf22f28..fbc13133 100644 --- a/src/nodes.coffee +++ b/src/nodes.coffee @@ -179,6 +179,7 @@ exports.Base = class Base # `compileToFragments` method has logic for outputting comments. unshiftCommentFragment commentFragment else + fragments.push @makeCode '' if fragments.length is 0 if commentFragment.unshift fragments[0].precedingComments ?= [] fragments[0].precedingComments.push commentFragment @@ -570,7 +571,13 @@ exports.Block = class Block extends Base fragments.push @makeCode '\n' if i fragments.push @makeCode "#{@tab}var " if declars - fragments.push @makeCode scope.declaredVariables().join(', ') + declaredVariables = scope.declaredVariables() + for declaredVariable, declaredVariablesIndex in declaredVariables + fragments.push @makeCode declaredVariable + if Object::hasOwnProperty.call o.scope.comments, declaredVariable + fragments.push o.scope.comments[declaredVariable]... + if declaredVariablesIndex isnt declaredVariables.length - 1 + fragments.push @makeCode ', ' if assigns fragments.push @makeCode ",\n#{@tab + TAB}" if declars fragments.push @makeCode scope.assignedVariables().join(",\n#{@tab + TAB}") @@ -2155,7 +2162,7 @@ exports.Assign = class Assign extends Base message = isUnassignable name.value name.error message if message - # `moduleDeclaration` can be `'import'` or `'export'` + # `moduleDeclaration` can be `'import'` or `'export'`. @checkAssignability o, name if @moduleDeclaration o.scope.add name.value, @moduleDeclaration @@ -2167,6 +2174,20 @@ exports.Assign = class Assign extends Base 'param' else o.scope.find name.value + # If this assignment identifier has one or more herecomments + # attached, output them as part of the declarations line (unless + # other herecomments are already staged there) for compatibility + # with Flow typing. Don’t do this if this assignment is for a + # class, e.g. `ClassName = class ClassName {`, as Flow requires + # the comment to be between the class name and the `{`. + if name.comments and not o.scope.comments[name.value] and + @value not instanceof Class and + name.comments.every((comment) -> comment.here and not comment.multiline) + commentsNode = new IdentifierLiteral name.value + commentsNode.comments = name.comments + commentFragments = [] + @compileCommentFragments o, commentsNode, commentFragments + o.scope.comments[name.value] = commentFragments if @value instanceof Code if @value.isStatic diff --git a/src/scope.litcoffee b/src/scope.litcoffee index b48aa231..d13130e2 100644 --- a/src/scope.litcoffee +++ b/src/scope.litcoffee @@ -11,10 +11,12 @@ Initialize a scope with its parent, for lookups up the chain, as well as a reference to the **Block** node it belongs to, which is where it should declare its variables, a reference to the function that it belongs to, and a list of variables referenced in the source code -and therefore should be avoided when generating variables. +and therefore should be avoided when generating variables. Also track comments +that should be output as part of variable declarations. constructor: (@parent, @expressions, @method, @referencedVars) -> @variables = [{name: 'arguments', type: 'arguments'}] + @comments = {} @positions = {} @utilities = {} unless @parent diff --git a/test/comments.coffee b/test/comments.coffee index 837391c6..c490f2d4 100644 --- a/test/comments.coffee +++ b/test/comments.coffee @@ -1073,3 +1073,37 @@ test "#4706: Flow comments for function spread", -> var method; method = (...rest/*: Array */) => {};''' + +test "#4747: Flow comments for local variable declaration", -> + eqJS 'a ###: number ### = 1', ''' + var a/*: number */; + + a = 1; + ''' + +test "#4747: Flow comments for local variable declarations", -> + eqJS ''' + a ###: number ### = 1 + b ###: string ### = 'c' + ''', ''' + var a/*: number */, b/*: string */; + + a = 1; + + b = 'c'; + ''' + +test "#4747: Flow comments for local variable declarations with reassignment", -> + eqJS ''' + a ###: number ### = 1 + b ###: string ### = 'c' + a ### some other comment ### = 2 + ''', ''' + var a/*: number */, b/*: string */; + + a = 1; + + b = 'c'; + + a/* some other comment */ = 2; + '''