Skip to content

Fix initializing parents of AnonClasses #15946

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

Closed
wants to merge 1 commit into from

Conversation

KacperFKorban
Copy link
Member

Previously creating ClassDefs of AnonClasses was only possible if the
parent class had a constructor with an empty parameter list. Otherwise,
the compiler would crash because of calling asTerm on a NoSymbol.
This PR adds logic that handles calling parent constructors with default
or repeated parameters. And also fails more gracefully if no instance can
be created.

fixes lampepfl#15855 (attempt 2)

Previously creating ClassDefs of AnonClasses was only possible if the
parent class had a constructor with an empty parameter list. Otherwise,
the compiler would crash because of calling `asTerm` on a `NoSymbol`.
This PR adds logic that handles calling parent constructors with default
or repeated parameters. And also fails more gracefully if no instance can
be created.

fixes lampepfl#15855 (attempt 2)
@KacperFKorban KacperFKorban marked this pull request as ready for review September 2, 2022 20:57
@odersky
Copy link
Contributor

odersky commented Sep 3, 2022

The fundamental problem is that we don't have a spec what a SAM type is. [If someone can point me to a spec, please do! I have not been able to find one myself]. As it stands I notice some inconsistencies:

abstract class MyFunction[+R](args: String*) {
  def apply(): R
}

trait MyFunction0[+R] extends MyFunction[R]

object Test extends App {
  val f: MyFunction[Int] = () => 1  // error: type mismatch
  val g: MyFunction0[Int] = () => 1 // OK
}

The definition of f will give an error (in both Scala 2 and Scala 3).

8 |  val f: MyFunction[Int] = () => 1  // error: type mismatch
  |                           ^^^^^^^
  |                           Found:    () => Int
  |                           Required: MyFunction[Int]
  |
  | longer explanation available when compiling with `-explain`

So, clearly, a class with vararg parameter does not count as a "zero-parameter-class" so, it is not a SAM type. The same for a class with default arguments.

But if I inherit from the class with an empty trait, it does work! This makes no sense. I think it is a corner case that was just overlooked.

The closest thing to a spec that we have is the following doc comment of object SAMType in Types.scala:

  /** An extractor for single abstract method types.
   *  A type is a SAM type if it is a reference to a class or trait, which
   *
   *   - has a single abstract method with a method type (ExprType
   *     and PolyType not allowed!) whose result type is not an implicit function type
   *     and which is not marked inline.
   *   - can be instantiated without arguments or with just () as argument.

Is MyFunction0 a SAM type? It looks like it can be instantiated without arguments, but that's not true. A new MyFunction0 { ... } gets expanded to new MyFunction([]) with MyFunction0 { ... }, so there is an argument passed to MyFunction, which is the empty sequence.

The question that needs to be clarified here is what does can be instantiated without arguments or with just () as argument mean? Is it in the source program? But then MyFunction is also a SAM type, which does not match implementations. Or is it in the expanded program? Then both MyFunction and MyFunction0 need to be rejected.

My vote would be for the second option. SAM type conversion is a conversion so inherently dangerous. We should not generalize that one willy nilly if we don't have to.

That would mean the right way to fix #15855 is to reject the program. I propose to change the ambiguous doc comment above to

  /** An extractor for single abstract method types.
   *  A type is a SAM type if it is a reference to a class or trait, which
   *
   *   - has a single abstract method with a method type (ExprType
   *     and PolyType not allowed!) whose result type is not an implicit function type
   *     and which is not marked inline.
   *   - can be instantiated without arguments or with just () as argument.
   *     Note that this criterion applies to the instantiation after desugaring and typing.
   *     Empty varargs and default arguments do count as real arguments. Likewise,
   *     a trait inheriting from a class that takes arguments cannot be instantiated
   *     without arguments. See neg/i15922.scala

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote for rejecting the program instead by tweaking zeroParamClass in object SAMType.

@odersky odersky assigned KacperFKorban and unassigned odersky Sep 3, 2022
@odersky
Copy link
Contributor

odersky commented Sep 3, 2022

Another dimension which we have not covered yet would be implicit arguments. If we adhere to the hypothesis that "having no arguments" should be evaluated for the source program, then classes taking implicit arguments are also SAMs. But I think that goes down into a very deep rabbit hole.

@odersky
Copy link
Contributor

odersky commented Sep 3, 2022

Should we reject the program and still keep all the additional code in ClassDef? I don't think so. ClassDef is in tpd, where everything else we do is very explicit. Nowhere else would be synthesize default arguments out of the blue. All that should be done before we invoke code in tpd. So I think we could have an assert that is more understandable than "calling asTerm on a NoSymbol", but that should be it.

@sjrd
Copy link
Member

sjrd commented Sep 3, 2022

If you reject the MyFunction0 case, then, as it stands, you reject all lambdas used for js.FunctionNs and js.ThisFunctionNs in the Scala.js ecosystem. For example, all the tests in
https://github.com/scala-js/scala-js/blob/v1.10.1/test-suite/js/src/test/require-sam/org/scalajs/testsuite/jsinterop/CustomJSFunctionTest.scala
will fail.

@odersky
Copy link
Contributor

odersky commented Sep 3, 2022

OK, I see. But then we need to come up with a spec what a SAM is, and that spec should treat MyFunction and MyFunction0 the same. For js, we need to allow empty varargs as zero arguments. That's doable. I am very skeptical about default arguments, and I think accepting implicit arguments goes way too far. For that reason, we still need to specify "does not need any arguments" on the typed code.

The simplest way forward would be to classify the empty sequence passed to a vararg as "does not need an argument" and to reject the rest. I.e. add the following line to our spec:

  Note that `()` is also an admissible argument for a vararg parameter.

Do you agree?

@sjrd
Copy link
Member

sjrd commented Sep 3, 2022

I definitely agree about the implicit parameters.

I don't know about default values. They don't seem harmful to me, since we can test for them on typed code.

@odersky
Copy link
Contributor

odersky commented Sep 3, 2022

@srjd But there's no mechanism to insert default values after typer, and I believe we are creating ourselves a can of worms if we start doing that.

constr.symbol.paramSymss.filter(!_.exists(_.isType)).foldLeft(constr) { (acc, paramList) =>
acc.appliedToTermArgs(paramList.zipWithIndex.collect {
case (paramSym, idx) if paramSym.flags.is(Flags.HasDefault) =>
typer.Applications.defaultArgument(acc, idx, false)
Copy link
Contributor

@odersky odersky Sep 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unusual and could be dangerous. For the sake of modularity, we should not refer to typer internals from transform phases. TypeAssigner is fine, but typer should be off limits.

case (paramSym, idx) if paramSym.flags.is(Flags.HasDefault) =>
typer.Applications.defaultArgument(acc, idx, false)
case (paramSym, idx) if fromRepeated(paramSym.info) != NoType =>
tpd.ref(defn.NilModule)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be an empty SeqLiteral instead.

@sjrd
Copy link
Member

sjrd commented Sep 3, 2022

@srjd But there's no mechanism to insert default values after typer, and I believe we are creating ourselves a can of worms if we start doing that.

Ah well maybe. I guess we can exclude default values.

@ckipp01 ckipp01 marked this pull request as draft May 9, 2023 18:02
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.

3 participants