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

Experimental Single Abstract Method support (sammy meets world) #3018

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@adriaanm
Copy link
Member

adriaanm commented Oct 4, 2013

Under -Xexperimental, typedFunction invokes synthesizeSAMFunction when the expected type for the function literal (pt) is not the built-in FunctionN type of the expected arity, but pt does have a SAM with the expected number of arguments.

synthesizeSAMFunction expands the following tree:

{ (p1: T1, ..., pN: TN) => body } : S

to:

{
 def apply$body(p1: T1, ..., pN: TN): T = body

 new S { def apply(p1: T1, ..., pN: TN): T = apply$body(p1,..., pN) }
}

The expansion assumes S (the expected type) defines a single abstract method (let's call that method apply for simplicity).

  1. If T is not fully defined, it is inferred by type checking def apply$body without a result type before type checking the block. The method's inferred result type is used instead of T. See test case
  2. To more easily enforce that S's members are not in scope in body, that tree goes to the apply$body method that's outside the anonymous subclass of S. (The separate apply$body method simplifies the implementation of 1&2.) See test case
  3. The following restrictions apply to S:
    1. Its primary constructor (if any) must be public, no-args, not overloaded.
    2. S must have exactly one abstract member, its SAM
    3. SAM must take exactly one argument list
    4. SAM must be monomorphic

We may later relax these requirements to allow an implicit argument list, both on the constructor and the SAM. Could also let the SAM be polymorphic.

PS: We'll require import language.sam instead of -Xexperimental, as soon as the SIP is ready and there are more tests.

adriaanm added some commits Oct 4, 2013

Clarify findMembers, add reverse engineered docs
When looking for deferred members, it only makes sense
to retry when deferred members aren't excluded.
Single Abstract Method support: synthesis helpers
`synthesizeSAMFunction` will be used to expand the following tree:

```
{ (p1: T1, ..., pN: TN) => body } : S
```

to:

```
{
 def apply$body(p1: T1, ..., pN: TN): T = body

 new S { def apply(p1: T1, ..., pN: TN): T = apply$body(p1,..., pN) }
}
```

The expansion assumes `S` (the expected type) defines a single abstract method
(let's call that method `apply` for simplicity).

1. If 'T' is not fully defined, it is inferred by type checking
`def apply$body` without a result type before type checking the block.
The method's inferred result type is used instead of T`.
[See test/files/pos/sammy_poly.scala]

2. To more easily enforce S's members are not in scope in `body`, that tree
goes to the `apply$body` method that's outside the anonymous subclass of S.
(The separate `apply$body` method simplifies the implementation of 1&2.)

3. The following restrictions apply to S:
  1. Its primary constructor (if any) must be public, no-args, not overloaded.
  2. S must have exactly one abstract member, its SAM
  3. SAM must take exactly one argument list
  4. SAM must be monomorphic

We may later relax these requirements to allow an implicit argument list,
both on the constructor and the SAM. Could also let the SAM be polymorphic.
Single Abstract Method support: synthesize SAMs
Under `-Xexperimental`, `typedFunction` invokes `synthesizeSAMFunction`
when the expected type for the function literal (`pt`) is not the built-in
`FunctionN` type of the expected arity, but `pt` does have a SAM
with the expected number of arguments.

PS: We'll require `import language.sam` instead of `-Xexperimental`,
as soon as the SIP is ready and there are more tests.
@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Oct 4, 2013

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Oct 4, 2013

PS: I also stress tested sammy

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Oct 5, 2013

Any chance of you dropping restrictions (3) and (4) ... especially (4).

@paulp

This comment has been minimized.

Copy link
Contributor

paulp commented Oct 5, 2013

The line following (3) and (4) says "We may later relax these requirements to allow an implicit argument list, both on the constructor and the SAM. Could also let the SAM be polymorphic." The facts that the restrictions exist and that the questions were answered preemptively both serve to suggest that the restriction won't be dropped in the immediate future, which is what you're presumably asking if you got that far, though it sounds more like you didn't see that line.

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Oct 5, 2013

@paulp I didn't see that line ;-)

@retronym

This comment has been minimized.

should be a devWarning

This comment has been minimized.

Copy link
Owner Author

adriaanm replied Oct 7, 2013

Oops.

@retronym

This comment has been minimized.

We should test this with a Java 8 interface with a one truly deferred method and others with defaults. I think that will lead to s/isDeferred/isDeferredNotDeafault.

We can even test that scenario on Java 6 with a test similar to test/files/run/t7398.scala.

This comment has been minimized.

Copy link
Owner Author

adriaanm replied Oct 7, 2013

Aha! Great catch. Looking into that.

@retronym

This comment has been minimized.

Pattern match?

def isMonomorphicMonoParamList(sym: Symbol) = sym.typeParams.isEmpty && sym.info.paramSectionCount == 1
deferredMembers match {
  case sym :: Nil if isMonomorphicMonoParamList(sym) => sym
  case _ => NoSymbol
}

This comment has been minimized.

Copy link
Owner Author

adriaanm replied Oct 7, 2013

I didn't want to turn the Scope returned by membersBasedOnFlags into a List.

@retronym

This comment has been minimized.

I think it's impossible for ctor.info.paramSectionCount to be 0, all constructors have at least one parameter section. The use of <= 0 implies otherwise.

This comment has been minimized.

Copy link
Owner Author

adriaanm replied Oct 7, 2013

The test is for <= 1, actually.

@retronym

This comment has been minimized.

Duplicated with synthesizePartialFunction.

This comment has been minimized.

Copy link
Owner Author

adriaanm replied Oct 8, 2013

McPastington has been driven into the hills for this battle.

@retronym

This comment has been minimized.

Why ANON_FUN_NAME?

This comment has been minimized.

Copy link
Owner Author

adriaanm replied Oct 7, 2013

See your "duplicated" comment below. What would be a good name? Something fresh based on sam.owner.name?

This comment has been minimized.

Copy link

retronym replied Oct 8, 2013

I think it should just be tpnme.ANON_CLASS_NAME for consistency with new T {}.

The class is scoped in a block so freshness isn't required.

We could consider making those names across the board; I've considered using the parent class name like $anon$Iterator.

For reference, Java 8 opts for:

% cat sandbox/Test.java
public class Test {
    public static void test(Functional f) {
        System.out.println(f.getClass());
    }

    public static void main(String... args) {
        test(x -> x);
    }
}

interface Functional { public int apply(int x); }

% $JAVA8_HOME/bin/javac -d sandbox/ sandbox/Test.java && $JAVA8_HOME/bin/java -classpath sandbox Test
class Test$$Lambda$1

This comment has been minimized.

Copy link

retronym replied Oct 8, 2013

That said, you could make an argument for $anonfun: the name is based on the syntactic form of the code, rather than what it is translated to. I'm not too bothered about this one so long as we make a conscious choice rather than leave it to Cutty McPastington :)

This comment has been minimized.

Copy link
Owner Author

adriaanm replied Oct 8, 2013

I guess I was intuitively going for the translation that's as close to regular Functions as possible.
IDE support (e.g., stepping through closures) should fall out for free, assuming it's based on the name of the anonymous class.

@retronym

This comment has been minimized.

SYNTHETIC?

This comment has been minimized.

Copy link
Owner Author

adriaanm replied Oct 7, 2013

Yep.

@retronym

This comment has been minimized.

I was worried that could lose track singleton types here. But the following test works:

scala> val s: String = ""; trait T { def apply(x: s.type): s.type }
s: String = ""
defined trait T

scala> ((x => x): T)(s)
res5: s.type = ""

This comment has been minimized.

Copy link
Owner Author

adriaanm replied Oct 7, 2013

Added test.

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Oct 7, 2013

Repeated params should probably be ruled out early, currently we hit a type error:

scala> trait T { def ap(as: String*): Any }; (xs => ()): T
<console>:10: error: type mismatch;
 found   : Seq[String]
 required: String
        (xs => ()): T
            ^
@retronym

This comment has been minimized.

Copy link
Member

retronym commented Oct 7, 2013

By name looks to work, here are a few test cases:

scala> trait T { def ap(as: => String): Any }; ((x => ()): T).ap(???)
defined trait T
res9: Any = ()

scala> trait T { def ap(as: => String): Any }; ((x => x): T).ap(???)
scala.NotImplementedError: an implementation is missing
  at scala.Predef$.$qmark$qmark$qmark(Predef.scala:229)
Nil,
List(fun.vparams.map(_.duplicate)), // must duplicate as we're also using them for `samDef`
TypeTree(samDefTp) setPos sampos.focus,
fun.body)

This comment has been minimized.

@retronym

retronym Oct 7, 2013

Member

We're dropping prefixes in the signature of the SAM.

% cat sandbox/test.scala
object Test {
  trait T { type U; def ap(a: String): U }; (xs => ???): T
}

% qbin/scalac -Xprint:typer -Xexperimental sandbox/test.scala
[[syntax trees at end of                     typer]] // test.scala
package <empty> {
  object Test extends scala.AnyRef {
    def <init>(): Test.type = {
      Test.super.<init>();
      ()
    };
    abstract trait T extends scala.AnyRef {
      type U;
      def ap(a: String): T.this.U
    };
    ({
      def ap$body(xs: String): Test.T#U = scala.this.Predef.???;
      @SerialVersionUID(0) final class $anonfun extends AnyRef with Test.T with Serializable {
        def <init>(): <$anon: Test.T> = {
          $anonfun.super.<init>();
          ()
        };
        final override def ap(xs: String): Test.T#U = ap$body(xs)
      };
      new $anonfun()
    }: Test.T)
  }
}

sandbox/test.scala:2: error: overriding method ap in trait T of type (a: String)this.U;
 method ap has incompatible type
  trait T { type U; def ap(a: String): U }; (xs => ???): T
                                                ^

Elsewhere, we call pt memberInfo sam which gives (a: String)Test.T#U. Instead, we would need: pt.typeSymbol.thisType memberInfo sam which gives (a: String)T.this.U. This in turn presents a challenge: we still need to substituteThis with the symbol of the derived class, which we're trying to avoid creating a symbol for manually.

This comment has been minimized.

@adriaanm

adriaanm Oct 8, 2013

Author Member

Yeah, this is a tricky problem. I hadn't thought of this instance, but a similar one applies when the result type depends on an argument value. The symbols for the params of the $body method will appear in the regular method's result type as we just copy it over.

This comment has been minimized.

@adriaanm

adriaanm Oct 8, 2013

Author Member

I can't think of a sensible implementation other than ??? for the lambda, though.

Don't pursue SAM translation after an arity mismatch.
Before this change:

    scala> trait T { def apply(a: Int): Int }
    defined trait T

    scala> ((x: Int, y: Int) => 0): T
    <console>:9: error: object creation impossible, since method apply in trait T of type (a: Int)Int is not defined
                  ((x: Int, y: Int) => 0): T
                                    ^
After the change, these cases report the same errors as they do
*without* -Xexperimental.
@retronym

This comment has been minimized.

Copy link
Member

retronym commented Oct 8, 2013

Here a tweak to avoid pursuing SAM translation when arities mismatch:

adriaanm#8

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Oct 8, 2013

Thanks! I'm working on implementing the rest of your feedback here: https://github.com/adriaanm/scala/tree/sammy-wip -- last TODO is the java8 test.

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Oct 8, 2013

One area for refinement after this PR is error messages. They usually suffer when typechecking cascades through a fallback.

We should endeavour to:

if (expectedTypeIsSam)
  val canSam == // arities and types line up, etc
  if (canSam) mkSam
  else withDiagnostic("Note: the function was not compatible with the SAM type ..") {
     mkFunction
  }
else
  mkFunction

This will be tricky, because mkFunction might work out fine, but we will be subsequently unable to adapt the function type to the almost-SAM. Perhaps we'll need a -Xlog-sams analagous to -Xlog-implicits to help diagnose why things didn't line up.

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Oct 9, 2013

bonus commit: adriaanm/scala@f02899e

adriaanm added some commits Oct 7, 2013

Single Abstract Method support: java8 test
Inspired by test/files/run/t7398.scala and sammy_poly.
Added some notes to original tests.

Elaborating on that note: we don't yet desugar `f(a)` to `f.sam(a)`,
like we do for regular functions: `f(a)` becomes `f.apply(a)`.

It seems pleasingly symmetrical and is easy to implement,
but not sure it's a good idea...
Simplify logic regarding missing parameter types
When we're typing the result of eta-expansion and fail
because of missing parameter types, we try again.
Whether we're in that case doesn't change between arguments,
so hoist that out of the loop.

Eventually, we should get rid of the retry and just immediately
type check the un-eta-expanded method call -- if possible...
@retronym

This comment has been minimized.

Copy link
Member

retronym commented Oct 9, 2013

LGTM. We can address the remaining issues in a followup.

As part of the review, I've donated a few more test cases: adriaanm#9

Perhaps we should consider a new facility to allow us to synthesize:

// (a, b) => body
class anon extends T {
  def foo(a: EmptyTree, b: EmptyTree) = magicScope {
    body
  }
}

in which magicScope hides foo and other other members of anon while typechecking body and where the EmptyTree-s for parameter types reuse the mechanism for method parameter type inference that backs -Yinfer-argument-types.

* Thus, findMember requiring DEFERRED flags yields deferred members,
* while `findMember(excludedFlags = 0, requiredFlags = 0).filter(_.isDeferred)` may not (if there's a corresponding concrete member)
*
* Requirements take precedence over exclusions, so requiring and excluding DEFERRED will yield a DEFERRED member (if there is one).

This comment has been minimized.

@gkossakowski

gkossakowski Oct 10, 2013

Member

After looking at the code I believe the predecessor of this implication is false. I think the way to find out is to write JUnit tests. Added to my personal TODO list.

@gkossakowski

This comment has been minimized.

Copy link
Member

gkossakowski commented Oct 13, 2013

Doesn't merge cleanly anymore. I'll fix the merge and open a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.