Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate method & constructor overloading (AdHoc Polymorphism) #816

Open
MaxGraey opened this issue Sep 4, 2019 · 25 comments
Open

Investigate method & constructor overloading (AdHoc Polymorphism) #816

MaxGraey opened this issue Sep 4, 2019 · 25 comments

Comments

@MaxGraey
Copy link
Member

MaxGraey commented Sep 4, 2019

This little bit verbose but valid TypeScript syntax for constructor overloading. Will be nice to have this

class Field {
  public name: string;
  public surname: string;

  constructor();
  constructor(name: string);
  constructor(name: string, surname: string);
  constructor(code: i32);
  constructor(name?: number | string, surname?: string) { // this could be ignored by AS. Just need for linter / diagnostiq
    if (name instanceof String && surname instanceof String) {
      this.name    = name;
      this.surname = surname;
    } else if (name instanceof String && !surname) {
      this.name    = name;
      this.surname = "<unknown>";
    } else if (name instanceof i32) {
      this.name    = name.toString();
      this.surname = "<unknown>";
    } else {
      this.name    = "<unknown>";
      this.surname = "<unknown>";
    }
  }
}

console.log(new Field('Jonh','Smith')); // Field { name: 'Jonh', surname: 'Smith' }
console.log(new Field('Jonh')); // Field { name: 'Jonh', surname: '<unknown>' }
console.log(new Field(123)); // Field { name: '123', surname: '<unknown>' }
console.log(new Field()); // Field { name: '<unknown>', surname: '<unknown>' }
@MaxGraey
Copy link
Member Author

MaxGraey commented Sep 4, 2019

This feature unblock some standard library methods like Array.from with different signatures

@MaxGraey MaxGraey changed the title [Proposal] Method & constructor overloading [Proposal] Method & constructor overloading (AdHoc Polymorphism) Sep 4, 2019
@MaxGraey
Copy link
Member Author

MaxGraey commented Sep 5, 2019

Currently we can partially emulate this via generics but this not intellisense-friendly and not possible for constructors

@willemneal
Copy link
Contributor

I know that we want to try to match TS as much as possible, especially because the IDE support, but one area where we can be an improvement over TS is with proper method overloading. When there is still an intersection of the types used at the call site and the various signatures, e.g. here it would be new Field("John", "Smith"); which could be the second or the forth constructor. However, in the cases where they are disjoint, e.g. (assuming that the last constructor only took a string) new Field(123) could be it's own function.

class Field {
  public name: string = "<unknown>";
  public surname: string = "<unknown>";

  constructor() {}
  constructor(name: string) {
    this.name = name;
  }
  constructor(name: string, surname: string = "<unknown>") {
    this.name = name;
    this.surname = surname;
  }
  constructor(name: i32, surname: string = "<unknown>") {
    this.name = name.toString();
    this.surname = surname;
  }  
}

I do understand the downside to moving away from complete TS support in IDE's, but in my opinion the above is much clearer and we could even allow for them to call each other. e.g.

class Field {
  public name: string = "<unknown>";
  public surname: string = "<unknown>";

  constructor() {}
  constructor(name: string) {
    this.name = name;
  }
  constructor(name: string, surname?: string) {
    constructor(name);
    if (!!surname){
      this.surname = surname;
    }
  }
  constructor(name: i32, surname?: string) {
    constructor(name.toString(), surname);
  }  
}

The benefit here is that they could be used in interfaces this way too.

@dcodeIO
Copy link
Member

dcodeIO commented Sep 6, 2019

Maybe we can make breaking with TS optional. So if someone wants it, they can do that, or if they want compatibility, they'd use TS's notation and accept the limitations?

@MaxGraey
Copy link
Member Author

MaxGraey commented Sep 6, 2019

@dcodeIO yeah, makes sense.

@willemneal Great! Just one suggestion

class Field {
  public name: string = "<unknown>";
  public surname: string = "<unknown>";

  constructor() {}

  constructor(name: string) {
    this.name = name;
  }

  constructor(name: string, surname?: string) {
    this(name); // or this.constructor ?
    if (surname) this.surname = surname;
  }

  constructor(name: i32, surname?: string) {
    this(name.toString(), surname);
  }  
}

Which will be more consistent with super()

@willemneal
Copy link
Contributor

Thanks! this() is what Java and C# do, but this.constructor() is clearer. Perhaps stick with this since users that are comfortable with constructor overloading would expect it.

@DanielRX
Copy link

@dcodeIO yeah, makes sense.

@willemneal Great! Just one suggestion

class Field {
  ...
}

Which will be more consistent with super()

Wouldn't

 constructor(name: string, surname?: string) {
  this(name); // or this.constructor ?
  if (surname) this.surname = surname;
}

be

constructor(name: string, surname: string) {
    this(name); // or this.constructor ?
    this.surname = surname;
}

since as it stands, this can already be merged with the existing constructor (without surname)?

@MaxGraey
Copy link
Member Author

MaxGraey commented Apr 26, 2020

So as I promised on recent meeting I suggest new approach for overloading methods which partially compatible with TS.

Example:

import utils from './utils';

export class Color {
    static fromNum(color: u32): Color {
        return new Color(
            (color >> 24) & 0xFF,
            (color >> 16) & 0xFF,
            (color >>  8) & 0xFF,
            (color >>  0) & 0xFF
        ); // implicitly call `constructor(a: u32, r: u32, g: u32, b: u32);`
    }

    static fromStr(color: string): Color {
        return new Color(utils.str2num(color)); // implicitly call Color.fromNum
    }

    @method(Color.fromNum) constructor(color: number);
    @method(Color.fromStr) constructor(color: string);
    constructor(a: u32, r: u32, g: u32, b: u32);
    constructor() {
        // default implementation
        this.a = a; this.r = r;  this.g = g; this.b = b;
    }

    /*private?*/ setColorFromNum(color: number) { /*...*/ }
    /*private?*/ setColorFromStr(color: string) { /*...*/ }

    @method(this.setColorFromNum) setColor(color: number): void;
    @method(this.setColorFromStr) setColor(color: string): void;
    setColor(a: u32, r: u32, g: u32, b: u32): void {
       /*...*/
    }
}

link to playground

Instead @method(x) decorator it could be @link, @overload, @redirect, @backward

also not sure about @method(Color.fromNum) / @method(this.setColorFromNum) probably better use @method("fromNum") / @method("setColorFromNum")

Thoughts?

@dcodeIO
Copy link
Member

dcodeIO commented Apr 26, 2020

Might work, but looks quite complicated. Personally I'd most likely do something more reliable like this:

export class Color {
  static create<T>(value: T): Color {
    if (isInteger<T>()) { ... }
    else if (isString<T>()) { ... }
    else ERROR("...");
  }
  constructor(a: u32, r: u32, g: u32, b: u32)
    this.a = a; this.r = r;  this.g = g; this.b = b;
  }
}

If arguments differ in more than just a T, I'd most likely favor defining a method multiple times with different signatures. Requires our custom language server for proper highlighting first, though, but would be fine for me considering that one can still write code that is compatible with tsc by not using the feature.

class Color {
  constructor(a: u32, r: u32, g: u32, b: u32)
    this.a = a; this.r = r;  this.g = g; this.b = b;
  }
  constructor(s: string) {
    this(...);
  }
}

@MaxGraey
Copy link
Member Author

Yes, but what if you want different implementation for different arity (number of args)?

@dcodeIO
Copy link
Member

dcodeIO commented Apr 26, 2020

Ah, sorry, covered this in an edit :)

@MaxGraey
Copy link
Member Author

MaxGraey commented Apr 26, 2020

class Color {
  constructor(a: u32, r: u32, g: u32, b: u32)
    this.a = a; this.r = r;  this.g = g; this.b = b;
  }
  constructor(s: string) {
    this(...);
  }
}

Will be ideal but it completely uncompatible with TS. Perhaps with our own language server it will be possible, but we loose portability mode

EDIT In other hand we already has precedence with top level decorators and operator overloads

@dcodeIO
Copy link
Member

dcodeIO commented Apr 26, 2020

There's also the option to support union types in scenarios like this (just not for variables), compiling them similar to generics:

  constructor(a: string | u32, r: u32 = 0, g: u32 = 0, b: u32 = 0)
    if (isString(a)) {
      ...
    } else {
      this.a = a; this.r = r;  this.g = g; this.b = b;
    }
  }

giving us a constructor<string,u32,u32,u32> and a constructor<u32,u32,u32,u32> depending on arguments.

@MaxGraey
Copy link
Member Author

Yeah, but it much more complicated and potentially error-prone approach

@dcodeIO
Copy link
Member

dcodeIO commented Apr 26, 2020

Perhaps we also can

export class Color {
    constructor(s: string);
    constructor(a: u32, r: u32, g: u32, b: u32);
    constructor() {
        // inject arguments[0..3] and arguments.length statically?
        // so either arguments.length == 1 and arguments[0] -> string
        // or arguments.length == 4 and arguments[0..3] -> u32
    }
}

@MaxGraey
Copy link
Member Author

Yeah. And in this case all checks better do statically. That's most TypeScrip-tish way

@MaxGraey MaxGraey pinned this issue May 9, 2020
@MaxGraey MaxGraey changed the title [Proposal] Method & constructor overloading (AdHoc Polymorphism) Method & constructor overloading (AdHoc Polymorphism) May 9, 2020
@dcodeIO dcodeIO changed the title Method & constructor overloading (AdHoc Polymorphism) Investigate method & constructor overloading (AdHoc Polymorphism) May 28, 2020
@trusktr
Copy link
Member

trusktr commented Jun 10, 2020

this(...);

That's only compatible with extending Function in JS:

class Foo extends Function {
  constructor(...args) {
    super(...args)
    this()
  }
}

new Foo('console.log("foo")') // logs "foo".

My vote is for not straying so far away from TS.

Can we have an option to enforce strict TS-compatible syntax so that we can get a compile error when not writing TS-compatible code? I think that'd be great!

@MaxGraey
Copy link
Member Author

MaxGraey commented Jul 5, 2020

Another possible alternative:

export class Color {
    @overload((s: string) => this);
    constructor(a: u32, r: u32, g: u32, b: u32) {
    }

    @overload((color: string) => void)
    setColor(a: u32, r: u32, g: u32, b: u32): void {
        // @ts-ignore
        if (color instanceof string) {
            // parse from string
        } else {
            // use a, r, g, b
        }
    }
}

And with unions and multi-values:

export class Color {
    constructor(a: u32 | string, r?: u32, g?: u32, b?: u32) {
    }

    setColor(a: u32 | string, r?: u32, g?: u32, b?: u32): void {
        if (a instanceof string) {
            // parse from string
        } else {
            // use a, r, g, b
        }
    }
}

@munrocket
Copy link

Constructor overloading can be very useful if your compilation target is only webassembly. Obviously that some switch will be better for those who don't want to compile sources to javascript. Sometime this is very handy because the language itself become more powerful.

@trusktr
Copy link
Member

trusktr commented Nov 19, 2020

I think AS just needs unions. Then using TypeScript overload syntax will be relatively easy.

Also, supporting multiple (compatible and incompatible) syntaxes would make more maintainance burden, right?

@aminya
Copy link

aminya commented Jan 14, 2021

Whichever approach is taken it should be compile time dispatch. Using if/else means it is runtime dispatch which does not have any benefits.

Regarding the syntax, I suggest breaking TS syntax here which is in my opinion very ugly and strange.

I like this syntax the most:
#816 (comment)

@trusktr
Copy link
Member

trusktr commented Apr 28, 2021

Using if/else means it is runtime dispatch which does not have any benefits.

Note that with strict types, conditional branches are eliminated during compile time based on what types the compiler detects are used at the call sites.

Regarding the syntax, I suggest breaking TS syntax here which is in my opinion very ugly and strange.

We should not break from TypeScript syntax (in any way we already have, we should provide compatible alternatives). The compatibility with existing tooling is a huge benefit of AssemblyScript.

The syntactic difference is only a small inconvenience, easy for any engineer to live with, and I believe it would not be worth the potential fragmentation (more on that below).

@trusktr
Copy link
Member

trusktr commented Apr 28, 2021

Maybe we can make breaking with TS optional. So if someone wants it, they can do that, or if they want compatibility, they'd use TS's notation and accept the limitations?

This opens the door to possible community fragmentation:

It may be nice for authors to choose compatible or incompatible syntax (and for every incompatible syntax we'd expect there to be a compatible syntax option), but what happens when downstream library users import incompatible code into their environments?

Suppose a library user wants compatibility with existing TypeScript tooling (VS Code, ESLint, etc) and they wish to write all their code using compatible features. What happens when they import a library that doesn't use compatible syntax?

^ If this doesn't work, it will cause fragmentation.

  • We may have to start labeling libraries as TS-compatible or TS-incompatible.
  • Or we would have to put effort into declaration emit and ensuring that both compatible and incompatible features have the same output format within declaration files, so that TypeScript tooling will work well based on that
    • asc compiler would continue type checking against actual sources (with possibly TS-incompatible features) like it currently does

Without declaration files, then TypeScript will need to type check sources from inside node_modules and will throw errors on incompatible syntax. To solve this fragmentation issue, we would need to ensure we can emit compatible declaration output regardless of the source syntax. It could be possible that some incompatible syntax may not have any way to be represented compatibly in declaration output, and we would need to absolutely avoid doing that or otherwise cause permanent fragmentation. At that point, someone would surely hard-fork AssemblyScript.

Furthermore, who would be willing to ensure that compatible and incompatible features output the same declaration output? This is work someone will need to take on.

Instead that effort could be used on the easier path forward: using compatible syntax and working on supporting more TS-compatible language features (closures, unions, etc), while not worrying about declaration emit.

@trusktr
Copy link
Member

trusktr commented Apr 28, 2021

I know that a * b is a tiny bit more convenient than a.times(b), but in the grand scheme of things, looking at the big picture, the less concise syntax is a very tiny price to pay for the big benefits that compatibility brings.

@aminya
Copy link

aminya commented May 5, 2021

Well, the description of AssemblyScript is "Definitely not a TypeScript to WebAssembly compiler". Trying to stay compatible with TypeScript's handling of functions/methods that is designed for JavaScript is just continuing a mistake in the design. I am all for backward compatibility as long as it is not a technical debt. AssemblyScript is young. It should not limit itself to something that was decided in 1995 for JavaScript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants