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

Bad error message when trying to define apply in an anonymous class extending js.Function #4281

Closed
gzm0 opened this issue Nov 11, 2020 · 6 comments · Fixed by #4288
Closed
Assignees
Labels
bug Confirmed bug. Needs to be fixed.
Milestone

Comments

@gzm0
Copy link
Contributor

gzm0 commented Nov 11, 2020

While trying to break our detection of JS functions, I found this:

@js.native
@JSGlobal("bad")
abstract class BadFunction extends js.Function1[Int, String]

object Test {
  new BadFunction {
    def apply(x: Int): String = "f"
  }
}

This fails to compile with the following error:

Concrete members of JS native types may only call js.native.
      def apply(x: Int): String = "f"
                                  ^

Note that this does not happen for other native traits:

@js.native
trait MyTrait[T] extends js.Object {
  def apply(): T
}

@js.native
@JSGlobal("bad")
abstract class BadMyTrait extends MyTrait[Int]

object Test {
  new BadMyTrait {
    def apply(): Int = 1
  }
}

This fails as expected with:

A non-native JS class cannot declare a method named `apply` without `@JSName`
      def apply(): Int = 1
          ^
@gzm0 gzm0 added the bug Confirmed bug. Needs to be fixed. label Nov 11, 2020
@sjrd
Copy link
Member

sjrd commented Nov 11, 2020

We should not even allow extending any subclass of any js.FunctionN.

@gzm0
Copy link
Contributor Author

gzm0 commented Nov 11, 2020

We should not even allow extending any subclass of any js.FunctionN.

As long as all that stuff stays native, it's probably OK? Or would the expectation for a facade be to define it's own applys? (FWIW, js.Object's compation does that without extending the relevant js.FunctionNs or even js.Function).

@sjrd
Copy link
Member

sjrd commented Nov 11, 2020

I mean, it's not really possible for a class, native or not, to truly extend a js.FunctionN. One cannot create any kind of class (with class or function) whose instances would be callable. Native objects can meaningfully provide an apply because they're functions themselves (they are not of a class that extend Function); native traits can define an apply, because in practice that means it's a function on which we've patched the other members of the interface. But a class can never provide an apply.

@sjrd
Copy link
Member

sjrd commented Nov 11, 2020

Hum ... it appears there is a way to define a class that could provide a call syntax:

>node
Welcome to Node.js v14.15.0.
Type ".help" for more information.
> class Foo extends Function { constructor(...args) { super(...args); } }
undefined
> var f = new Foo("x", "return x + 1;");
undefined
> f(5)
6
> typeof f
'function'

@gzm0
Copy link
Contributor Author

gzm0 commented Nov 12, 2020

Maybe, for now, we should simply disallow non-native subclasses of js.FunctionN?

@sjrd
Copy link
Member

sjrd commented Nov 12, 2020

Yes, that sounds reasonable.

@gzm0 gzm0 self-assigned this Nov 12, 2020
gzm0 added a commit to gzm0/scala-js that referenced this issue Nov 13, 2020
@gzm0 gzm0 added this to the v1.3.1 milestone Nov 13, 2020
gzm0 added a commit to gzm0/scala-js that referenced this issue Nov 13, 2020
@gzm0 gzm0 linked a pull request Nov 13, 2020 that will close this issue
sjrd added a commit that referenced this issue Nov 14, 2020
Fix #4281: Do not treat anonymous JS classes as lambdas
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug. Needs to be fixed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants