From 5fb0898ae7cb979e0c1a0392ba9cf43b3be03049 Mon Sep 17 00:00:00 2001 From: Alan Pierce Date: Sun, 10 Jul 2016 20:37:03 -0700 Subject: [PATCH] fix: add implicit function call parens in the normalize step The code in #301 is actually parsed incorrectly by the CoffeeScript parser (even before decaffeinate-parser does anything interesting). It says that the function body ends at the end of the file, so decaffeinate was inserting a close-curly-brace at the end of the file, which was incorrect. However, the function application was correctly parsed, and explicitly putting parens for the function application causes the CoffeeScript parser to work again, so we can work around this issue by inserting parens for all implicit functions before the MainStage. I think this will also fix a bunch of other problems caused by implicit function calls. For example, it also happens to fix #269. In the codebase that I'm trying to decaffeinate, it fixes 49 out of the 104 files with decaffeinate failures that I hadn't categorized. I added the heuristic that we put the close-paren on the next line if the last arg is a multi-line expression. This fixed the formatting in almost every test, except the change introduced two test issues: * Object literal formatting sometimes had an excessive newline in non-implicit function calls ( http://decaffeinate-project.org/repl/#?evaluate=true&code=a(b%2C%0A%20%20c%3A%20d%0A%20%20e%3A%20f%0A%29 ) so adding parens exposed the issue. It's pretty minor, though. I think. * In another test, the added parens interfered with some other operations in the normalize step in a way that will be fixed by https://github.com/Rich-Harris/magic-string/pull/89 Closes #301. Closes #269. --- src/patchers/NodePatcher.js | 2 +- .../patchers/FunctionApplicationPatcher.js | 32 ++--------- src/stages/normalize/index.js | 5 ++ .../patchers/FunctionApplicationPatcher.js | 53 +++++++++++++++++++ test/conditional_test.js | 9 ++++ test/for_test.js | 12 +++++ test/function_test.js | 20 +++++++ test/object_test.js | 3 +- 8 files changed, 107 insertions(+), 29 deletions(-) create mode 100644 src/stages/normalize/patchers/FunctionApplicationPatcher.js diff --git a/src/patchers/NodePatcher.js b/src/patchers/NodePatcher.js index e089aea01..f3136c55e 100644 --- a/src/patchers/NodePatcher.js +++ b/src/patchers/NodePatcher.js @@ -656,7 +656,7 @@ export default class NodePatcher { * Determines whether this patcher's node spanned multiple lines. */ isMultiline(): boolean { - return /[\r\n]/.test(this.getOriginalSource()); + return !this.node.virtual && /[\r\n]/.test(this.getOriginalSource()); } /** diff --git a/src/stages/main/patchers/FunctionApplicationPatcher.js b/src/stages/main/patchers/FunctionApplicationPatcher.js index 060826b90..57bf7f25a 100644 --- a/src/stages/main/patchers/FunctionApplicationPatcher.js +++ b/src/stages/main/patchers/FunctionApplicationPatcher.js @@ -1,6 +1,6 @@ import NodePatcher from './../../../patchers/NodePatcher.js'; import type { Editor, Node, ParseContext } from './../../../patchers/types.js'; -import { CALL_START, COMMA } from 'coffee-lex'; +import { COMMA } from 'coffee-lex'; import { isSemanticToken } from '../../../utils/types.js'; export default class FunctionApplicationPatcher extends NodePatcher { @@ -18,29 +18,15 @@ export default class FunctionApplicationPatcher extends NodePatcher { this.args.forEach(arg => arg.setRequiresExpression()); } + /** + * Note that we don't need to worry about implicit function applications, + * since the normalize stage would have already added parens. + */ patchAsExpression() { - let implicitCall = this.isImplicitCall(); let { args, outerEndTokenIndex } = this; this.fn.patch(); - if (implicitCall && args.length === 0) { - this.insert(this.fn.outerEnd, '()'); - return; - } - - if (implicitCall) { - let firstArg = args[0]; - let hasOneArg = args.length === 1; - let firstArgIsOnNextLine = !firstArg ? false : - /[\r\n]/.test(this.context.source.slice(this.fn.outerEnd, firstArg.outerStart)); - if ((hasOneArg && firstArg.node.virtual) || firstArgIsOnNextLine) { - this.insert(this.fn.outerEnd, '('); - } else { - this.overwrite(this.fn.outerEnd, firstArg.outerStart, '('); - } - } - args.forEach((arg, i) => { arg.patch(); let isLast = i === args.length - 1; @@ -60,14 +46,6 @@ export default class FunctionApplicationPatcher extends NodePatcher { this.insert(arg.outerEnd, ','); } }); - - if (implicitCall) { - this.insert(this.innerEnd, ')'); - } - } - - isImplicitCall() { - return !this.fn.hasSourceTokenAfter(CALL_START); } /** diff --git a/src/stages/normalize/index.js b/src/stages/normalize/index.js index 23420c74c..66105be31 100644 --- a/src/stages/normalize/index.js +++ b/src/stages/normalize/index.js @@ -1,6 +1,7 @@ import ConditionalPatcher from './patchers/ConditionalPatcher.js'; import ForInPatcher from './patchers/ForInPatcher.js'; import ForOfPatcher from './patchers/ForOfPatcher.js'; +import FunctionApplicationPatcher from './patchers/FunctionApplicationPatcher.js'; import NodePatcher from '../../patchers/NodePatcher.js'; import PassthroughPatcher from '../../patchers/PassthroughPatcher.js'; import ProgramPatcher from './patchers/ProgramPatcher.js'; @@ -34,6 +35,10 @@ export default class NormalizeStage extends TransformCoffeeScriptStage { case 'ForOf': return ForOfPatcher; + case 'FunctionApplication': + case 'NewOp': + return FunctionApplicationPatcher; + case 'While': return WhilePatcher; diff --git a/src/stages/normalize/patchers/FunctionApplicationPatcher.js b/src/stages/normalize/patchers/FunctionApplicationPatcher.js new file mode 100644 index 000000000..f95abb383 --- /dev/null +++ b/src/stages/normalize/patchers/FunctionApplicationPatcher.js @@ -0,0 +1,53 @@ +import NodePatcher from './../../../patchers/NodePatcher.js'; +import type { Editor, Node, ParseContext } from './../../../patchers/types.js'; +import { CALL_START } from 'coffee-lex'; + +export default class FunctionApplicationPatcher extends NodePatcher { + fn: NodePatcher; + args: Array; + + constructor(node: Node, context: ParseContext, editor: Editor, fn: NodePatcher, args: Array) { + super(node, context, editor); + this.fn = fn; + this.args = args; + } + + patchAsExpression() { + let implicitCall = this.isImplicitCall(); + let { args } = this; + + this.fn.patch(); + + if (implicitCall && args.length === 0) { + this.insert(this.fn.outerEnd, '()'); + return; + } + + if (implicitCall) { + let firstArg = args[0]; + let hasOneArg = args.length === 1; + let firstArgIsOnNextLine = !firstArg ? false : + /[\r\n]/.test(this.context.source.slice(this.fn.outerEnd, firstArg.outerStart)); + if ((hasOneArg && firstArg.node.virtual) || firstArgIsOnNextLine) { + this.insert(this.fn.outerEnd, '('); + } else { + this.overwrite(this.fn.outerEnd, firstArg.outerStart, '('); + } + } + + args.forEach(arg => arg.patch()); + + if (implicitCall) { + let lastArg = args[args.length - 1]; + if (lastArg.isMultiline()) { + this.appendLineAfter(')'); + } else { + this.insert(this.innerEnd, ')'); + } + } + } + + isImplicitCall() { + return !this.fn.hasSourceTokenAfter(CALL_START); + } +} diff --git a/test/conditional_test.js b/test/conditional_test.js index bf424a3a4..3f7aa1752 100644 --- a/test/conditional_test.js +++ b/test/conditional_test.js @@ -266,6 +266,15 @@ describe('conditionals', () => { }); it('works with nested POST-`if`', () => { + check(` + a(b) if c(d) unless e(f) + `, ` + if (!e(f)) { if (c(d)) { a(b); } } + `); + }); + + // TODO: Enable once https://github.com/Rich-Harris/magic-string/pull/89 lands. + it.skip('works with nested POST-`if` with implicit calls', () => { check(` a b if c d unless e f `, ` diff --git a/test/for_test.js b/test/for_test.js index 9cdc6182e..f2dcfd1b4 100644 --- a/test/for_test.js +++ b/test/for_test.js @@ -547,4 +547,16 @@ describe('for loops', () => { } } `)); + + it('handles for loops over implicit function calls', () => + check(` + for a in b c + d() + `, ` + let iterable = b(c); + for (let i = 0; i < iterable.length; i++) { + let a = iterable[i]; + d(); + } + `)); }); diff --git a/test/function_test.js b/test/function_test.js index b9b1ddf94..b01434ced 100644 --- a/test/function_test.js +++ b/test/function_test.js @@ -156,4 +156,24 @@ describe('functions', () => { it('keeps function with a single assignment as a parameter in braces', () => { check(`(args=false) =>`, `(args=false) => {};`); }); + + it('places the function end in the right place when ending in an implicit function call', () => + check(` + A = { + b: -> + return c d, + e, + f + } + G + `, ` + const A = { + b() { + return c(d, + e, + f); + } + }; + G; + `)); }); diff --git a/test/object_test.js b/test/object_test.js index 0bef9de9f..40e309862 100644 --- a/test/object_test.js +++ b/test/object_test.js @@ -81,7 +81,8 @@ describe('objects', () => { a(b, { c: d, e: f - }); + } + ); `); });