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

Multiple overloads for same (binary) operator #1821

Closed
wants to merge 7 commits into from

Conversation

data-ux
Copy link

@data-ux data-ux commented Apr 22, 2021

  • I've read the contributing guidelines

Use case

It would be useful if we could have multiple overloads for the same operator, allowing eg. Vector class that can be multiplied both by scalar and another Vector:

class Vec3 {
    constructor(public x: f32 = 0, public y: f32 = 0, public z: f32 = 0) {}

    @operator('*')
    multScalar(right: f32): Vec3 {
        return new Vec3(this.x * right, this.y * right, this.z * right)
    }
    @operator('*')
    multVector(right: Vec3): Vec3 {
        return new Vec3(this.x * right.x, this.y * right.y, this.z * right.z)
    }
}

Currently the compiler complains "ERROR TS2393: Duplicate function implementation."

Approach

  • Change Class and ClassPrototype so that overloads is an array
  • When compiling/resolving, choose the correct method implementation from the array based on right hand side type
  • When adding overloads to array, check that combinations are allowed:
    • Allow multiple overloads only for binary operators
    • Allow multiple overloads only for instance (not static) methods
    • Allow only one implantation per right hand side type

Progress

This is a draft PR. I'm currently implementing the above for ADD operator as proof of concept. I'm calling resolver.resolveExpression(...) in compiler.ts, line 4068 to find the type for the right hand side, but the call itself is causing the compiler to fail seemingly unrelated tests. So I'm guessing the method cannot be called in this phase of compilation or I'm calling it the wrong way?

Does this approache seem feasible in general?

@MaxGraey
Copy link
Member

MaxGraey commented Apr 22, 2021

Thanks for PR!
I'm not sure about multi-overloading and I guess better solve this in more idiomatic way:

class Vec3 {
  constructor(public x: f32 = 0, public y: f32 = 0, public z: f32 = 0) {}

  @operator('*')
  mul<T>(right: T): Vec3 {
    if (right instanceof f32) {
      return new Vec3(this.x * right, this.y * right, this.z * right);
    } else if (right instanceof Vec3) {
      return new Vec3(this.x * right.x, this.y * right.y, this.z * right.z);
    }
    assert(false);
  }
}

But this may fail currently.

@data-ux
Copy link
Author

data-ux commented Apr 24, 2021

Thanks for the example, but the code actually crashes the compiler.

It fails the assert in resolver.js, resolveFunction(), line 2674: assert(!typeParameterNodes || typeParameterNodes.length == 0);. So I guess type parameters are not possible on operator overloaded methods?

Anyway, IMO static dispatch would be preferable so that we can have compile time type checking.

@MaxGraey
Copy link
Member

MaxGraey commented Apr 24, 2021

Ad-hoc overloading which works only for instance-based custom operators and only for right hand side is too unclear and violates the integrity in general. I propose read this thread first: #816.

But it will be awesome if you fix current issue with generic-based operator overloading which much more idiomatic and more expected from everyone who familiar with TypeScript.

@data-ux data-ux closed this May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants