Handled overloaded Promise-returning methods correctly (dart-archive#57)

* Handled overloaded Promise-returning methods correctly

* Emitted null checks in extension methods with optional parameters to call the class method of the correct arity

Author: derekxu16

lib/declaration.ts

@@ -3,7 +3,7 @@ import * as ts from 'typescript';
 import * as base from './base';
 import {FacadeConverter} from './facade_converter';
 import {Transpiler} from './main';
-import {MergedParameter, MergedType, MergedTypeParameters} from './merge';
+import {MergedMember, MergedParameter, MergedType, MergedTypeParameters} from './merge';
 
 export function isFunctionLikeProperty(
     decl: ts.VariableDeclaration|ts.ParameterDeclaration|ts.PropertyDeclaration|
@@ -20,8 +20,9 @@ export function isFunctionLikeProperty(
 export default class DeclarationTranspiler extends base.TranspilerBase {
   private tc: ts.TypeChecker;
   private extendsClass = false;
+  private visitPromises = false;
   private containsPromises = false;
-  private promiseMethods: Set<ts.FunctionLikeDeclaration> = new Set();
+  private promiseMethods: ts.SignatureDeclaration[] = [];
 
   static NUM_FAKE_REST_PARAMETERS = 5;
 
@@ -243,7 +244,16 @@ export default class DeclarationTranspiler extends base.TranspilerBase {
     }
   }
 
-  visitMergingOverloads(members: ts.NodeArray<ts.Node>) {
+  /**
+   * Visits an array of class members and merges overloads.
+   *
+   * @returns An updated version of the members array. All overloaded methods are grouped into
+   *     MergedMember objects that contain all original declarations, as well as the merged result.
+   *     Other non-overloaded members are represented by MergedMembers with only one constituent,
+   *     which is the single declaration of the member.
+   */
+  visitMergingOverloads(members: ts.NodeArray<ts.Node>): MergedMember[] {
+    const result: MergedMember[] = [];
     // TODO(jacobr): merge method overloads.
     let groups: Map<string, ts.Node[]> = new Map();
     let orderedGroups: Array<ts.Node[]> = [];
@@ -315,9 +325,24 @@ export default class DeclarationTranspiler extends base.TranspilerBase {
       group.push(node);
     });
 
-    orderedGroups.forEach((group: Array<ts.Node>) => {
+    orderedGroups.forEach((group: Array<ts.SignatureDeclaration>) => {
+      const first = group[0];
+      // If the methods in this group return Promises and this.visitPromises is false, skip
+      // visiting these methods and add them to this.promiseMethods. If the methods in this group
+      // return Promises and this.visitPromises is true, it means that this function is being called
+      // from emitMethodsAsExtensions and the methods should now be visited.
+      if (!this.visitPromises && base.isPromise(first.type)) {
+        if (!this.containsPromises) {
+          this.containsPromises = true;
+        }
+        group.forEach((declaration: ts.SignatureDeclaration) => {
+          this.promiseMethods.push(declaration);
+        });
+        return;
+      }
       if (group.length === 1) {
-        this.visit(group[0]);
+        this.visit(first);
+        result.push(new MergedMember(group, first));
         return;
       }
       group.forEach((fn: ts.Node) => {
@@ -330,7 +355,6 @@ export default class DeclarationTranspiler extends base.TranspilerBase {
         this.maybeLineBreak();
       });
       // TODO: actually merge.
-      let first = <ts.SignatureDeclaration>group[0];
       let kind = first.kind;
       let merged = <ts.SignatureDeclaration>ts.createNode(kind);
       merged.parent = first.parent;
@@ -384,7 +408,9 @@ export default class DeclarationTranspiler extends base.TranspilerBase {
       merged.typeParameters = mergedTypeParams.toTypeParameters();
 
       this.fc.visit(merged);
+      result.push(new MergedMember(group, merged));
     });
+    return result;
   }
 
 
@@ -686,16 +712,6 @@ export default class DeclarationTranspiler extends base.TranspilerBase {
         break;
       case ts.SyntaxKind.MethodSignature:
         let methodSignatureDecl = <ts.FunctionLikeDeclaration>node;
-        if (base.isPromise(methodSignatureDecl.type)) {
-          if (this.promiseMethods.has(methodSignatureDecl)) {
-            break;
-          }
-          if (!this.containsPromises) {
-            this.containsPromises = true;
-          }
-          this.promiseMethods.add(methodSignatureDecl);
-          break;
-        }
         this.visitDeclarationMetadata(methodSignatureDecl);
         this.visitFunctionLike(methodSignatureDecl);
         break;
@@ -865,15 +881,15 @@ export default class DeclarationTranspiler extends base.TranspilerBase {
 
     this.visitClassBody(decl, name);
     this.emit('}\n');
-    if (this.promiseMethods.size) {
+    if (this.promiseMethods.length) {
       const visitName = () => {
         this.visitClassLikeName(name, typeParameters, ts.createNodeArray(), false);
       };
       const visitNameOfExtensions = () => {
         this.visitClassLikeName(name, typeParameters, ts.createNodeArray(), true);
       };
       this.emitMethodsAsExtensions(name, visitName, visitNameOfExtensions, this.promiseMethods);
-      this.promiseMethods.clear();
+      this.promiseMethods = [];
     }
     this.emit('\n');
   }
@@ -949,15 +965,14 @@ export default class DeclarationTranspiler extends base.TranspilerBase {
 
   private emitMethodsAsExtensions(
       className: ts.Identifier, visitName: () => void, visitNameOfExtensions: () => void,
-      methods: Set<ts.FunctionLikeDeclaration>) {
+      methods: ts.SignatureDeclaration[]) {
+    this.visitPromises = true;
     // Emit private class containing external methods
     this.emit(`@JS('${base.ident(className)}')`);
     this.emit(`abstract class _`);
     visitName();
     this.emit('{');
-    for (const declaration of methods) {
-      this.visitFunctionLike(declaration);
-    }
+    const mergedMembers = this.visitMergingOverloads(ts.createNodeArray(Array.from(methods)));
     this.emit('}\n');
 
     // Emit extensions on public class to expose methods
@@ -966,7 +981,8 @@ export default class DeclarationTranspiler extends base.TranspilerBase {
     this.emit('on');
     visitName();
     this.emit('{');
-    for (const declaration of methods) {
+    for (const merged of mergedMembers) {
+      const declaration = merged.mergedDeclaration;
       if (!base.isPromise(declaration.type)) {
         continue;
       }
@@ -981,10 +997,49 @@ export default class DeclarationTranspiler extends base.TranspilerBase {
       this.emit('final _');
       this.fc.visitTypeName(className);
       this.emit('tt = t;\n');
-      this.emit(`return promiseToFuture(tt.${base.ident(declaration.name)}`);
-      this.visitParameters(declaration.parameters, {namesOnly: true});
-      this.emit(');}\n');
+      this.emitExtensionBody(merged);
+      this.emit('}\n');
     }
     this.emit('}\n');
+    this.visitPromises = false;
+  }
+
+  private emitExtensionBody({constituents, mergedDeclaration}: MergedMember) {
+    // Determine all valid arties of this method by going through the overloaded signatures
+    const arities: Set<number> = new Set();
+    for (const constituent of constituents) {
+      const arity = constituent.parameters.length;
+      arities.add(arity);
+    }
+    const sortedArities = Array.from(arities).sort();
+    for (const arity of sortedArities) {
+      if (arity < mergedDeclaration.parameters.length) {
+        const firstOptionalIndex = arity;
+        const suppliedParameters = mergedDeclaration.parameters.slice(0, firstOptionalIndex);
+        const omittedParameters = mergedDeclaration.parameters.slice(
+            firstOptionalIndex, mergedDeclaration.parameters.length);
+        // Emit null checks to verify the number of omitted parameters
+        this.emit('if (');
+        let isFirst = true;
+        for (const omitted of omittedParameters) {
+          if (isFirst) {
+            isFirst = false;
+          } else {
+            this.emit('&&');
+          }
+          this.visit(omitted.name);
+          this.emit('== null');
+        }
+        this.emit(') {');
+        this.emit(`return promiseToFuture(tt.${base.ident(mergedDeclaration.name)}`);
+        this.visitParameters(ts.createNodeArray(suppliedParameters), {namesOnly: true});
+        this.emit('); }\n');
+      } else {
+        // No parameters were omitted, no null checks are necessary for this call
+        this.emit(`return promiseToFuture(tt.${base.ident(mergedDeclaration.name)}`);
+        this.visitParameters(ts.createNodeArray(mergedDeclaration.parameters), {namesOnly: true});
+        this.emit(');\n');
+      }
+    }
   }
 }

lib/merge.ts

@@ -31,7 +31,7 @@ export class MergedType {
           this.merge((t as ts.TypePredicateNode).type);
           return;
         case ts.SyntaxKind.TypeReference:
-          // We need to follow Alais types as Dart does not support them for non
+          // We need to follow Alias types as Dart does not support them for non
           // function types. TODO(jacobr): handle them for Function types?
           let typeRef = <ts.TypeReferenceNode>t;
           let decl = this.fc.getDeclaration(typeRef.typeName);
@@ -224,6 +224,22 @@ export class MergedTypeParameters {
   }
 }
 
+/**
+ * Represents a merged class member. If the member was not overloaded, the constituents array
+ * will contain only the original declaration and mergedDeclaration will just be the original
+ * declaration. If the member was an overloaded method, the constituents array will contain all
+ * overloaded declarations and mergedDeclaration will contain the result of merging the overloads.
+ */
+export class MergedMember {
+  constructor(
+      public constituents: ts.SignatureDeclaration[],
+      public mergedDeclaration: ts.SignatureDeclaration) {}
+
+  isOverloaded(): boolean {
+    return this.constituents.length > 1;
+  }
+}
+
 /**
  * Normalize a SourceFile
  */

test/declaration_test.ts

@@ -268,7 +268,7 @@ abstract class MyMath {}
 
 @JS('MyMath')
 abstract class _MyMath {
-  Promise<num> randomInRange(num start, num end);
+  external Promise<num> randomInRange(num start, num end);
 }
 
 extension MyMathExtensions on MyMath {
@@ -291,7 +291,7 @@ abstract class X<T> {}
 
 @JS('X')
 abstract class _X<T> {
-  Promise<T> f(T a);
+  external Promise<T> f(T a);
 }
 
 extension XExtensions<T> on X<T> {
@@ -326,7 +326,7 @@ abstract class Z implements Y {}
 
 @JS('Z')
 abstract class _Z {
-  Promise<String> f();
+  external Promise<String> f();
 }
 
 extension ZExtensions on Z {
@@ -337,6 +337,66 @@ extension ZExtensions on Z {
   }
 }
 
+@JS()
+abstract class Promise<T> {}`);
+      expectTranslate(`declare interface X {
+        f(a: string): Promise<number>;
+        f(a: string, b: number): Promise<number>;
+      }`).to.equal(`import "package:js/js_util.dart" show promiseToFuture;
+
+@anonymous
+@JS()
+abstract class X {}
+
+@JS('X')
+abstract class _X {
+  /*external Promise<num> f(String a);*/
+  /*external Promise<num> f(String a, num b);*/
+  external Promise<num> f(String a, [num b]);
+}
+
+extension XExtensions on X {
+  Future<num> f(String a, [num b]) {
+    final Object t = this;
+    final _X tt = t;
+    if (b == null) {
+      return promiseToFuture(tt.f(a));
+    }
+    return promiseToFuture(tt.f(a, b));
+  }
+}
+
+@JS()
+abstract class Promise<T> {}`);
+      expectTranslate(`declare interface X {
+        f(a: string): Promise<number>;
+        f(a: number, b: number): Promise<number>;
+        f(c: number[]): Promise<number>;
+      }`).to.equal(`import "package:js/js_util.dart" show promiseToFuture;
+
+@anonymous
+@JS()
+abstract class X {}
+
+@JS('X')
+abstract class _X {
+  /*external Promise<num> f(String a);*/
+  /*external Promise<num> f(num a, num b);*/
+  /*external Promise<num> f(List<num> c);*/
+  external Promise<num> f(dynamic /*String|num|List<num>*/ a_c, [num b]);
+}
+
+extension XExtensions on X {
+  Future<num> f(dynamic /*String|num|List<num>*/ a_c, [num b]) {
+    final Object t = this;
+    final _X tt = t;
+    if (b == null) {
+      return promiseToFuture(tt.f(a_c));
+    }
+    return promiseToFuture(tt.f(a_c, b));
+  }
+}
+
 @JS()
 abstract class Promise<T> {}`);
     });