Do not eta-expand 0-arg methods. Improve eta-expansion for method values. #6475

adriaanm · 2018-03-27T21:30:22Z

Insert () before considering eta-expansion. A small step towards Scala 3, where you will have to apply () explicitly to Scala-defined methods that define an argument list.

Also clean up method value expansion: do it directly, instead of attaching a note for later and adapting again, which fixes scala/bug#8299.

szeiger · 2018-04-06T12:54:00Z

I think this goes in the right direction and it helps with the overloaded methods in the new collection library that required changes in the compiler codebase. Without -Xsource:2.14:

[error] /Users/szeiger/code/scala/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala:558: missing argument list for method mapArgInfo
[error] Unapplied methods are only converted to functions when a function type is expected.
[error] You can make this conversion explicit by writing `mapArgInfo _` or `mapArgInfo(_)` instead of `mapArgInfo`.
[error]       val argInfos = originalCallsite.argInfos.flatMap(mapArgInfo)
[error]                                                        ^
[error] /Users/szeiger/code/scala/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala:577: missing argument list for method mapArgInfo
[error] Unapplied methods are only converted to functions when a function type is expected.
[error] You can make this conversion explicit by writing `mapArgInfo _` or `mapArgInfo(_)` instead of `mapArgInfo`.
[error]       val capturedArgInfos = originalClosureInit.capturedArgInfos.flatMap(mapArgInfo)
[error]                                                                           ^
[error] /Users/szeiger/code/scala/src/compiler/scala/tools/reflect/ToolBoxFactory.scala:230: missing argument list for method makeParam
[error] Unapplied methods are only converted to functions when a function type is expected.
[error] You can make this conversion explicit by writing `makeParam _` or `makeParam(_)` instead of `makeParam`.
[error]           meth setInfo MethodType(freeTerms.map(makeParam).toList, AnyTpe)
[error]                                                 ^

These calls now compile with -Xsource:2.14 because the methods are monomorphic.

There's still the problem of the dual meaning of "expected type" in overload resolution. When we know that we're typechecking a lambda we can set it to a lub of the parameter types (i.e. a glb of the function type) but for typechecking other constructs that would be wrong (you need the opposite variance). While the more aggressive expansion will attempt to typecheck a method reference as a lambda it doesn't know about the parameter types that are necessary to do that.

This limitation causes a regressions due to the new collection library in cases like this:

    def iden[T](x: T): T = x
    val v = scala.collection.immutable.BitSet(1,2,3).map(iden)
    val vt: scala.collection.immutable.BitSet = v

These are the ones where an explicit _ is not sufficient as a work-around, you have to write map(x => iden(x)) to make overload resultion supply the correct parameter types that allow typing of the polymorphic lambda.

adriaanm · 2018-04-06T13:45:39Z

The dependence on syntactic function literals goes away in my overload_proto branch, btw. Let’s discuss in a chat next week.

…

On Fri, Apr 6, 2018 at 14:54 Stefan Zeiger ***@***.***> wrote: I think this goes in the right direction and it helps with the overloaded methods in the new collection library that required changes in the compiler codebase. Without -Xsource:2.14: [error] /Users/szeiger/code/scala/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala:558: missing argument list for method mapArgInfo [error] Unapplied methods are only converted to functions when a function type is expected. [error] You can make this conversion explicit by writing `mapArgInfo _` or `mapArgInfo(_)` instead of `mapArgInfo`. [error] val argInfos = originalCallsite.argInfos.flatMap(mapArgInfo) [error] ^ [error] /Users/szeiger/code/scala/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala:577: missing argument list for method mapArgInfo [error] Unapplied methods are only converted to functions when a function type is expected. [error] You can make this conversion explicit by writing `mapArgInfo _` or `mapArgInfo(_)` instead of `mapArgInfo`. [error] val capturedArgInfos = originalClosureInit.capturedArgInfos.flatMap(mapArgInfo) [error] ^ [error] /Users/szeiger/code/scala/src/compiler/scala/tools/reflect/ToolBoxFactory.scala:230: missing argument list for method makeParam [error] Unapplied methods are only converted to functions when a function type is expected. [error] You can make this conversion explicit by writing `makeParam _` or `makeParam(_)` instead of `makeParam`. [error] meth setInfo MethodType(freeTerms.map(makeParam).toList, AnyTpe) [error] ^ These calls now compile with -Xsource:2.14 because the methods are monomorphic. There's still the problem of the dual meaning of "expected type" in overload resolution. When we know that we're typechecking a lambda we can set it to a lub of the parameter types (i.e. a glb of the function type) but for typechecking other constructs that would be wrong (you need the opposite variance). While the more aggressive expansion will attempt to typecheck a method reference as a lambda it doesn't know about the parameter types that are necessary to do that. This limitation causes a regressions due to the new collection library in cases like this: def iden[T](x: T): T = x val v = scala.collection.immutable.BitSet(1,2,3).map(iden) val vt: scala.collection.immutable.BitSet = v These are the ones where an explicit _ is not sufficient as a work-around, you have to write map(x => iden(x)) to make overload resultion supply the correct parameter types that allow typing of the polymorphic lambda. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6475 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFjy4_dXOvy_h35zWi2RnjFRykUyztJks5tl2VqgaJpZM4S9q6f> .

lrytz · 2018-04-09T08:47:16Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

-              typed(tree0, mode, pt)
-          }
+        // (4.2) apply to empty argument list
+        else if (mt.params.isEmpty && (settings.isScala213 || !isFunctionType(pt))) emptyApplication


isScala213 is true for 2.13+, so should it be !isScala214 here?

The intent is to go the other way: anything < 2.13 needs to check the expected type before inserting the empty apply. From 2.13, we’ll always insert them. I’ll add a comment to clarify

szeiger · 2018-05-03T12:49:02Z

@adriaanm Should we merge this for M4 or hold off until you're done with the additional changes in overload_proto?

adriaanm · 2018-05-03T12:58:03Z

I would like to merge it. Lukas hasn't officially signed off on it yet, though. Maybe @retronym can take a look?

adriaanm · 2018-05-09T09:47:12Z

ping @lrytz

SethTisue · 2018-05-09T17:06:17Z

fixes scala/scala-dev#391

SethTisue · 2018-05-09T17:07:02Z

@adriaanm do you want a community build run on this...?

adriaanm · 2018-05-09T17:43:48Z

Hadn't thought about it, but I guess it wouldn't hurt :-)

Also eta-expand method values directly, rather than going through an adapt, which then needs to remember that an _ followed the method value

We'll use the expected type when we type the eta-expansion that was requested. We know `m` is expected to be a method, since only methods can be followed by `_` to request eta-expansion. So, have the expected type reflect that. Since the original expected type won't be compatible with the method type that FUNmode gives us (method values are not first class), we have to defer using the given expected type until we eta-expanded, which results in a function, which is first class. When typing under FUNmode under pt=* we can let implicit conversion do its thing, before we wrap this in a function. See scala/bug#8299.

lrytz

LGTM. Push--f'd to your branch, added some tests (diff https://gist.github.com/lrytz/82e301f22610ce7d1b604a069db8b2fc)

adriaanm · 2018-05-10T13:47:32Z

Thanks!

SethTisue · 2018-05-10T17:38:00Z

(I'll keep an eye out for related breakage in future community build runs.)

jrudolph · 2019-01-16T13:22:11Z

Is there any more documentation about why this change was done? Is it for implementation reasons? Or is it a temporary thing needed for an incremental improvement to saner defaults in the upcoming versions?

I see that it is somehow related to 0-arg methods still being allowed to be called without parentheses (why?). Why not change that behavior and keep eta-expansion intact? It's an incompatible change anyway.

While trying to migrate code to 2.13 (in https://github.com/akka/akka-http/pull/2298/files#diff-10c929125531820c5fea3db482903b3eR41) we now have the situation where for

def f0(): Int = 23
def f1(a: Int): Int = 42
def g(f0: () => Int, f1: Int => Int): Int = ???

in 2.12 we have

g(f0, f1)

while for 2.13 a valid replacement is

g(() => f0, f1)

which seems odd. First, it's now inconsistent with the 1-arg case and also the f0 application still doesn't require parentheses.

Of course, it's not the only possible replacement. g(f0 _, f1) might still work but it seems method values are also being deprecated in the future?

adriaanm · 2019-01-16T13:57:37Z

Our assumption is that relying on automatic insertion of () is more common than eta-expansion of a 0-arg method, and thus changing the latter affects fewer users than the former.

adriaanm · 2019-01-16T14:01:34Z

oh, wait a minute, looks like I forgot about

If the expected type is of the form () => T, we eta expand.

(from scala/scala3#2570 (comment))

So, your example should actually work.

jrudolph · 2019-01-16T14:20:46Z

Hmm, isn't it the same as https://github.com/scala/scala/pull/6475/files#diff-97f418d5b560997cbbea454143f963cfR9 ? Or should that only happen under -Xsource:2.14?

Welcome to Scala 2.13.0-M5 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_181).
Type in expressions for evaluation. Or try :help.

scala> def f0(): Int = 42
f0: ()Int

scala> def g(f0: () => Int) = ???
g: (f0: () => Int)Nothing

scala> g(f0)
         ^
       error: type mismatch;
        found   : Int
        required: () => Int

adriaanm · 2019-01-16T14:22:07Z

I was confused (well, we did go back and forth on this a few times), but the latest decision was to allow eta expansion if the expected type demands it. I'll create a follow up PR today

jrudolph · 2019-01-16T14:29:43Z

I was confused (well, we did go back and forth on this a few times), but the latest decision was to allow eta expansion if the expected type demands it. I'll create a follow up PR today

But is it still possible given the spec update to do Empty Application before eta expansion? It seems the M5 behavior is at least following the spec (even if that's not what I would like).

adriaanm · 2019-01-16T14:35:49Z

Hmm. 🤔 Now that I've refreshed my recollection of the motivation (see especially scala/bug#9178), I wonder whether a type-driven exception is a good idea. For dotty alignment, we'd have to do it, but the puzzlers are puzzling.

scala/bug#7187 deprecates eta-expansion of zero-arg method values (since 2.12; was never introduced for SAM types).
The next step is to also deprecate insertion of () (except for java-defined methods), as dotty already requires explicitly writing them.
Once explicit application to () is required, we can more aggressively eta-expand method references, even if they are 0-arity

jrudolph · 2019-01-16T14:41:29Z

Yep, this issue (like so many others) seems to have connections and inter-dependencies with so many other things it's quite hard to keep track of motivations and current progress...

adriaanm · 2019-01-16T15:39:53Z

I just checked the status quo in dotty (https://github.com/dotty-staging/dotty/blob/master/compiler/src/dotty/tools/dotc/typer/Typer.scala#L2491), and it looks like 0-ary eta expansion is performed regardless of expected type (well, there's a warning when the SAM type is not annotated as FunctionalInterface). I just happened to be in the neighbourhood yesterday but missed this. /cc @odersky @smarter

adriaanm · 2019-01-16T15:46:03Z

Since this issue is spread out over so many tickets, here's the motivation for deprecating eta-expansion in this case:

def mkStub(): () => String = () => ""
val stub: () => Any = mkStub

println(stub()) // <function0>

adriaanm · 2019-01-16T15:48:09Z

I think you could just as well argue that it doesn't make sense to define mkStub with an empty argument list, since it's pure in the sense that it doesn't mutate any potentially aliased objects.

Since you did define mkStub to take an argument list, you should also supply it on (intended) application.

adriaanm · 2019-01-16T16:30:43Z

@jrudolph do you have a real world example in akka or something? In the abstract, it's hard to decide.

dwijnand · 2019-01-16T16:53:29Z

https://github.com/akka/akka-http/pull/2298/files#diff-10c929125531820c5fea3db482903b3eR41 ?

jrudolph · 2019-01-17T09:33:20Z

@jrudolph do you have a real world example in akka or something? In the abstract, it's hard to decide.

Yes, one instance is in https://github.com/akka/akka-http/pull/2298/files#diff-10c929125531820c5fea3db482903b3eR41. But I think it's hard to discuss concretely because every single case will be quite insignificant when there's a simple workaround like here. I just stumbled over it because in that example we have both Function1 and Function0 next to each other and it shows how the new behavior isn't consistent any more.

It was

def encodeChunk(bytes: ByteString): ByteString = ...
def finish(): ByteString = ...

StreamUtils.byteStringTransformer(encodeChunk, finish)

Now it has to be

StreamUtils.byteStringTransformer(encodeChunk, () => finish())

Since this issue is spread out over so many tickets, here's the motivation for deprecating eta-expansion in this case:
def mkStub(): () => String = () => ""
val stub: () => Any = mkStub

println(stub()) // <function0>

In my opinion, this example works as intended. Empty application seems much more like a convenience feature than eta-expansion (but your mileage may vary) so I'd wish for eta-expansion to take precedence. You could go further and say why would you create a 0-arg method at all in Scala if not to mean it to be side-effecting? In that case empty application seems downright dangerous 🔥

adriaanm · 2019-01-17T09:44:37Z

I tend to agree. I think our (future) rule for auto-application should be even stricter than it is in dotty right now. You only get a free () when invoking a Java-defined method, otherwise, if the method is defined with an argument list, you must supply it. A Scala method overriding a Java method also gets an exemption: it may drop the empty argument list defined in Java (because you can't express no-argument list in Java), and then any (statically known) application of that argument-list-free method of course needn't supply the empty argument list.

I think we should deprecate along these lines in 2.14, and enforce in 3.0.

jrudolph · 2019-01-17T09:48:47Z

👍 sounds like a good plan for me.

I think the conflict will then still remains (and a different solution might be chosen) for calling Java methods or creating SAMs of Java interfaces like the one shown in scala/bug#9178.

adriaanm · 2019-01-17T13:10:33Z

See #7660

SethTisue · 2019-01-17T17:58:39Z

I'm not sure if I grasp the whole picture here, but this part for sure:

You only get a free () when invoking a Java-defined method

is 💯, the way it is now has been bugging me for a decade

scala-jenkins added this to the 2.13.0-M4 milestone Mar 27, 2018

adriaanm changed the title ~~wip: cleanup eta expansion~~ wip: cleanup eta expansion [ci: last-only] Mar 28, 2018

adriaanm force-pushed the eta_cleanup branch from 88548ad to 75886ed Compare March 28, 2018 16:11

adriaanm force-pushed the eta_cleanup branch 2 times, most recently from d34eff5 to 0fe3b36 Compare April 5, 2018 21:50

adriaanm changed the title ~~wip: cleanup eta expansion [ci: last-only]~~ Insert () before eta-expansion, clean up Apr 5, 2018

lrytz reviewed Apr 9, 2018

View reviewed changes

eed3si9n mentioned this pull request Apr 10, 2018

Audit -Xfuture for 2.13 scala/scala-dev#471

Closed

5 tasks

adriaanm mentioned this pull request Apr 10, 2018

Drop -Y flags that change language semantics #6505

Merged

adriaanm force-pushed the eta_cleanup branch 2 times, most recently from 3e53360 to 0ebdc2d Compare April 11, 2018 15:17

adriaanm mentioned this pull request Apr 26, 2018

SAM types don't work for zero parameter methods scala/bug#10555

Closed

adriaanm force-pushed the eta_cleanup branch 2 times, most recently from 9a4bd27 to 4b0ef47 Compare April 26, 2018 13:28

adriaanm changed the title ~~Insert () before eta-expansion, clean up~~ Do not eta-expand 0-arg methods. Improve eta-expansion for method values. Apr 26, 2018

adriaanm force-pushed the eta_cleanup branch from 4b0ef47 to 44b3aa4 Compare May 8, 2018 13:50

SethTisue mentioned this pull request May 9, 2018

disable 0-ary eta expansion scala/scala-dev#391

Closed

adriaanm added 2 commits May 10, 2018 15:02

Insert () before eta under -Xsource:2.13

33a56a3

Also eta-expand method values directly, rather than going through an adapt, which then needs to remember that an _ followed the method value

lrytz force-pushed the eta_cleanup branch from 44b3aa4 to 99ff973 Compare May 10, 2018 13:04

lrytz approved these changes May 10, 2018

View reviewed changes

adriaanm merged commit 8951701 into scala:2.13.x May 10, 2018

SethTisue added the release-notes worth highlighting in next release notes label May 14, 2018

MasseGuillaume mentioned this pull request Aug 2, 2018

Rewrite for eta-expension scala/scala-collection-compat#131

Open

adriaanm mentioned this pull request Jan 8, 2019

eta expansion should not precede empty application scala/bug#7187

Closed

adriaanm added a commit to adriaanm/scala that referenced this pull request Jan 8, 2019

Spec update for scala#6475

1b47f24

adriaanm mentioned this pull request Jan 8, 2019

Spec update: empty application precedes eta expansion #7614

Merged

adriaanm mentioned this pull request Jan 16, 2019

Fix #4364: Try SAM type when no candidates are found scala/scala3#5717

Merged

SethTisue mentioned this pull request Jan 16, 2019

bring back eta-expansion if expected type is Function0 scala/scala-dev#575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not eta-expand 0-arg methods. Improve eta-expansion for method values. #6475

Do not eta-expand 0-arg methods. Improve eta-expansion for method values. #6475

adriaanm commented Mar 27, 2018 •

edited

Loading

szeiger commented Apr 6, 2018

adriaanm commented Apr 6, 2018 via email

lrytz Apr 9, 2018

adriaanm Apr 9, 2018

szeiger commented May 3, 2018

adriaanm commented May 3, 2018

adriaanm commented May 9, 2018

SethTisue commented May 9, 2018

SethTisue commented May 9, 2018

adriaanm commented May 9, 2018

lrytz left a comment •

edited

Loading

adriaanm commented May 10, 2018

SethTisue commented May 10, 2018

jrudolph commented Jan 16, 2019

adriaanm commented Jan 16, 2019

adriaanm commented Jan 16, 2019 •

edited

Loading

jrudolph commented Jan 16, 2019

adriaanm commented Jan 16, 2019

jrudolph commented Jan 16, 2019

adriaanm commented Jan 16, 2019 •

edited

Loading

jrudolph commented Jan 16, 2019

adriaanm commented Jan 16, 2019

adriaanm commented Jan 16, 2019 •

edited

Loading

adriaanm commented Jan 16, 2019

adriaanm commented Jan 16, 2019

dwijnand commented Jan 16, 2019

jrudolph commented Jan 17, 2019

adriaanm commented Jan 17, 2019

jrudolph commented Jan 17, 2019

adriaanm commented Jan 17, 2019

SethTisue commented Jan 17, 2019 •

edited

Loading

Do not eta-expand 0-arg methods. Improve eta-expansion for method values. #6475

Do not eta-expand 0-arg methods. Improve eta-expansion for method values. #6475

Conversation

adriaanm commented Mar 27, 2018 • edited Loading

szeiger commented Apr 6, 2018

adriaanm commented Apr 6, 2018 via email

lrytz Apr 9, 2018

Choose a reason for hiding this comment

adriaanm Apr 9, 2018

Choose a reason for hiding this comment

szeiger commented May 3, 2018

adriaanm commented May 3, 2018

adriaanm commented May 9, 2018

SethTisue commented May 9, 2018

SethTisue commented May 9, 2018

adriaanm commented May 9, 2018

lrytz left a comment • edited Loading

Choose a reason for hiding this comment

adriaanm commented May 10, 2018

SethTisue commented May 10, 2018

jrudolph commented Jan 16, 2019

adriaanm commented Jan 16, 2019

adriaanm commented Jan 16, 2019 • edited Loading

jrudolph commented Jan 16, 2019

adriaanm commented Jan 16, 2019

jrudolph commented Jan 16, 2019

adriaanm commented Jan 16, 2019 • edited Loading

jrudolph commented Jan 16, 2019

adriaanm commented Jan 16, 2019

adriaanm commented Jan 16, 2019 • edited Loading

adriaanm commented Jan 16, 2019

adriaanm commented Jan 16, 2019

dwijnand commented Jan 16, 2019

jrudolph commented Jan 17, 2019

adriaanm commented Jan 17, 2019

jrudolph commented Jan 17, 2019

adriaanm commented Jan 17, 2019

SethTisue commented Jan 17, 2019 • edited Loading

adriaanm commented Mar 27, 2018 •

edited

Loading

lrytz left a comment •

edited

Loading

adriaanm commented Jan 16, 2019 •

edited

Loading

adriaanm commented Jan 16, 2019 •

edited

Loading

adriaanm commented Jan 16, 2019 •

edited

Loading

SethTisue commented Jan 17, 2019 •

edited

Loading