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
This commit is contained in:
Geoffrey Booth 2017-10-19 06:56:59 -07:00 committed by GitHub
parent 6faa7f2b35
commit 0dc4755920
5 changed files with 96 additions and 9 deletions

View File

@ -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. Dont 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;
}
}
});
}

View File

@ -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 = {};

View File

@ -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. Dont 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

View File

@ -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

View File

@ -1073,3 +1073,37 @@ test "#4706: Flow comments for function spread", ->
var method;
method = (...rest/*: Array<string> */) => {};'''
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;
'''