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

Support instanceof checks for exported Scala class constructors #4062

Open
swachter opened this issue May 27, 2020 · 8 comments
Open

Support instanceof checks for exported Scala class constructors #4062

swachter opened this issue May 27, 2020 · 8 comments
Labels
enhancement Feature request (that does not concern language semantics, see "language")

Comments

@swachter
Copy link

swachter commented May 27, 2020

At the moment, instanceof checks do only work for classes that extend js.Object. Unfortunately case classes must not extend js.Object.

Motivation (copied from Gitter): I would like to define Scala ADTs (represented by sealed traits plus cases classes) by TypeScript union types. A common pattern for exhaustiveness checks in TypeScript is to use flow typing with instanceof checks.

@gzm0 gzm0 added the enhancement Feature request (that does not concern language semantics, see "language") label May 27, 2020
@gzm0 gzm0 changed the title support instanceof checks for case classes Support instanceof checks for exported Scala class constructors May 27, 2020
@gzm0
Copy link
Contributor

gzm0 commented May 27, 2020

To summarize our conversation on Gitter:

It is possible for us to do so by setting .prototype on exported constructors. We'd need a new IR node for this.

The downside of this is that we would lock-in certain structural constraints on how Scala.js implements Scala classes in JavaScript.

@sjrd does that summarize the discussion well?

@sjrd
Copy link
Member

sjrd commented May 27, 2020

Yes, that is a fairly accurate summary.

I'll add that I think the most promising way to implement this is on top of Symbol.hasInstance, although that means it would only work when emitting ECMAScript 2015 code.

@gzm0
Copy link
Contributor

gzm0 commented May 27, 2020

Oh, also to double check my understanding: The reason we are reluctant to support case classes that extend js.Object is because equals and hashCode would not work, right? (apply and unapply could be made to work, right)?

@gzm0
Copy link
Contributor

gzm0 commented May 27, 2020

I'll add that I think the most promising way to implement this is on top of Symbol.hasInstance, although that means it would only work when emitting ECMAScript 2015 code.

This is interesting now that I think about this: Wouldn't this give us a way forward if we chose a different encoding? In ES5.1 we could rely on .prototype. This would lock us in on the encoding, but we could make any potential new encoding dependent on being in ES6.

@sjrd
Copy link
Member

sjrd commented May 27, 2020

Oh, also to double check my understanding: The reason we are reluctant to support case classes that extend js.Object is because equals and hashCode would not work, right? (apply and unapply could be made to work, right)?

That's right. And if equals doesn't work for a case class, I'm sure a lot of expectations in users would be very broken.

I'll add that I think the most promising way to implement this is on top of Symbol.hasInstance, although that means it would only work when emitting ECMAScript 2015 code.

This is interesting now that I think about this: Wouldn't this give us a way forward if we chose a different encoding? In ES5.1 we could rely on .prototype. This would lock us in on the encoding, but we could make any potential new encoding dependent on being in ES6.

In theory, yes. In practice, that would mean more code paths if we change the encoding for ES6 but we have to preserve the old one for ES5.1.

Another problem with .prototype is that it accidentally gives too much power. For example, it would kind-of allow a user to extend a Scala class from JavaScript, if the stars align. For example if the class is eligible for the inline-init optimization, like the following:

@JSExportTopLevel("Foo")
@JSExportAll
class Foo(val x: Int)

it would be rougly translate as

class $c_Foo extends $c_jl_Object {
  constructor(x) {
    super()
    this.f_x = x;
  }
  get "x"() {
    return this.f_x;
  }
}
function Foo(x) {
  return new $c_Foo(x);
}
Foo.prototype = $c_Foo.prototype;

which means that the following user JS code would happen to "work":

class Bar extends Foo {
  constructor(x, y) {
    super(x);
    this.y = y;
  }
}

var bar = new Bar(5, 6)
bar.x // 5
bar.y // 6

So it would appear that one can meaningfully extend an exported class, even though that's completely undefined behavior, and will break in obscure situations (not final anymore, 2 reachable constructors, etc.).

@gzm0
Copy link
Contributor

gzm0 commented May 27, 2020

Maybe then we should just make it an ES2015 feature only. We do have precedent for this with static member inheritance, right?

@sjrd
Copy link
Member

sjrd commented May 27, 2020

Yes, that's right.

@sjrd
Copy link
Member

sjrd commented Aug 13, 2022

I dug up ce17362 , in which we actually removed that (then unspecified) behavior. 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request (that does not concern language semantics, see "language")
Projects
None yet
Development

No branches or pull requests

3 participants