fix: add implicit function call parens in the normalize step (#302)

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 Rich-Harris/magic-string#89

Closes #301.
Closes #269.

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());
   }
 
   /**

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);
   }
 
   /**

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;
 

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<NodePatcher>;
+
+  constructor(node: Node, context: ParseContext, editor: Editor, fn: NodePatcher, args: Array<NodePatcher>) {
+    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);
+  }
+}

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
     `, `

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();
+      }
+    `));
 });

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;
+    `));
 });

test/object_test.js

@@ -81,7 +81,8 @@ describe('objects', () => {
       a(b, {
         c: d,
         e: f
-      });
+      }
+      );
     `);
   });