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

SI-7187 deprecate eta-expansion of zero-arg method values #5327

Merged
merged 1 commit into from Aug 11, 2016

Conversation

Projects
None yet
5 participants
@lrytz
Member

lrytz commented Aug 10, 2016

For backwards compatiblity with 2.11, we already
don't adapt a zero-arg method value to a SAM.
In 2.13, we won't do any eta-expansion for zero-arg method values,
but we should deprecate first.

Re-submission of #5309

JIRA: https://issues.scala-lang.org/browse/SI-7187

SI-7187 deprecate eta-expansion of zero-arg method values
For backwards compatiblity with 2.11, we already
don't adapt a zero-arg method value to a SAM.
In 2.13, we won't do any eta-expansion for zero-arg method values,
but we should deprecate first.
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 10, 2016

Member

Thanks, @lrytz! 🍻

Member

adriaanm commented Aug 10, 2016

Thanks, @lrytz! 🍻

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 10, 2016

Member

LGTM -- let's come back in 2.12.x and disable 0-ary eta expansion under -Xsource:2.13

Member

adriaanm commented Aug 10, 2016

LGTM -- let's come back in 2.12.x and disable 0-ary eta expansion under -Xsource:2.13

@lrytz lrytz merged commit 910d2db into scala:2.12.x Aug 11, 2016

5 checks passed

cla @lrytz signed the Scala CLA. Thanks!
Details
integrate-ide [2812] SUCCESS. Took 2 s.
Details
validate-main [3169] SUCCESS. Took 75 min.
Details
validate-publish-core [3110] SUCCESS. Took 19 min.
Details
validate-test [2741] SUCCESS. Took 55 min.
Details
@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Aug 16, 2016

Member

is this release-notable, or a bit too obscure for that?

Member

SethTisue commented Aug 16, 2016

is this release-notable, or a bit too obscure for that?

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Aug 17, 2016

Member

i think we could add it. it's not that adding this one causes the release notes to overflow, it's more that they are already so long that it doesn't matter anymore :)

Member

lrytz commented Aug 17, 2016

i think we could add it. it's not that adding this one causes the release notes to overflow, it's more that they are already so long that it doesn't matter anymore :)

@lrytz lrytz added the release-notes label Aug 17, 2016

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Sep 1, 2016

Member

I ran into this when upgrading play-twirl in the community build. The warning asks me, "Did you intend to write foo()?" But that's the wrong suggestion; what I want in my case is foo _. Perhaps the message should offer both alternatives?

Member

SethTisue commented Sep 1, 2016

I ran into this when upgrading play-twirl in the community build. The warning asks me, "Did you intend to write foo()?" But that's the wrong suggestion; what I want in my case is foo _. Perhaps the message should offer both alternatives?

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Sep 1, 2016

Member

also, should this PR have included a spec update (to 6.26.2)?

Member

SethTisue commented Sep 1, 2016

also, should this PR have included a spec update (to 6.26.2)?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Sep 2, 2016

Member

What should we update about the spec? We don't usually include notes on deprecation there, but maybe we should? Regarding the error message, the assumption in swapping the order of eta-expansion and empty-arg-list-application is that the latter is far more common than the former. Thus, the message shouldn't have to mention the former (if it's relevant enough to mention, maybe we shouldn't change the behavior?)

Member

adriaanm commented Sep 2, 2016

What should we update about the spec? We don't usually include notes on deprecation there, but maybe we should? Regarding the error message, the assumption in swapping the order of eta-expansion and empty-arg-list-application is that the latter is far more common than the former. Thus, the message shouldn't have to mention the former (if it's relevant enough to mention, maybe we shouldn't change the behavior?)

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Sep 2, 2016

Member

6.26.2 says eta expansion comes before empty application. it's a chain of "Otherwise," clauses, and empty application is last in the chain. I have not followed this whole thing in detail, but I thought the whole point of SI-7187 was to change the order in 6.26.2.

Member

SethTisue commented Sep 2, 2016

6.26.2 says eta expansion comes before empty application. it's a chain of "Otherwise," clauses, and empty application is last in the chain. I have not followed this whole thing in detail, but I thought the whole point of SI-7187 was to change the order in 6.26.2.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Sep 2, 2016

Member

You understand correctly, except that the actual change is deferred until
2.13 :-) for now we're just warning
On Fri, Sep 2, 2016 at 20:03 Seth Tisue notifications@github.com wrote:

6.26.2 says eta expansion comes before empty application. it's a chain of
"Otherwise," clauses, and empty application is last in the chain. I have
not followed this whole thing in detail, but my understanding is that the
whole point of SI-7187 was to change the order in 6.26.2.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#5327 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFjy1zqm6W07YPxSorNnMXa1SuG0cQlks5qmGTogaJpZM4JhEGX
.

Member

adriaanm commented Sep 2, 2016

You understand correctly, except that the actual change is deferred until
2.13 :-) for now we're just warning
On Fri, Sep 2, 2016 at 20:03 Seth Tisue notifications@github.com wrote:

6.26.2 says eta expansion comes before empty application. it's a chain of
"Otherwise," clauses, and empty application is last in the chain. I have
not followed this whole thing in detail, but my understanding is that the
whole point of SI-7187 was to change the order in 6.26.2.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#5327 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFjy1zqm6W07YPxSorNnMXa1SuG0cQlks5qmGTogaJpZM4JhEGX
.

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Sep 2, 2016

Member

ah. so SI-7187 shouldn't have been closed in JIRA, then?

Member

SethTisue commented Sep 2, 2016

ah. so SI-7187 shouldn't have been closed in JIRA, then?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Sep 6, 2016

Member

Yep, reopened.

Member

adriaanm commented Sep 6, 2016

Yep, reopened.

@adriaanm adriaanm modified the milestone: 2.12.0-RC1 Oct 29, 2016

@adriaanm adriaanm added the 2.12 label Oct 29, 2016

@lrytz lrytz referenced this pull request Nov 1, 2016

Closed

notes on possible 2.12 release notes improvements #202

12 of 16 tasks complete
@adamvoss

This comment has been minimized.

Show comment
Hide comment
@adamvoss

adamvoss Nov 21, 2016

Given this, shouldn't I be getting a deprecation warning for the following code on Scala 2.12.0?

package object mypackage {
  def render() = {}
  val handler: Function0[Any] = render _
}

I see with -Xprint:typer this becomes {(() => package.this.render())}

I arrived here by way of #4822 (comment) in trying to understand why the code above is fine but using eta-expansion with other SAM-convertible types gives a compiler error due to type mismatch (scala-js/scala-js#2656). I see the release note

Note that SAM conversion only applies to lambda expressions, not to arbitrary expressions with Scala FunctionN types

However, I would think that irrespective of this item and the note:

Eta-expansion (conversion of a method to a function value) of zero-args methods has been deprecated, as this can lead to surprising behavior

adamvoss commented Nov 21, 2016

Given this, shouldn't I be getting a deprecation warning for the following code on Scala 2.12.0?

package object mypackage {
  def render() = {}
  val handler: Function0[Any] = render _
}

I see with -Xprint:typer this becomes {(() => package.this.render())}

I arrived here by way of #4822 (comment) in trying to understand why the code above is fine but using eta-expansion with other SAM-convertible types gives a compiler error due to type mismatch (scala-js/scala-js#2656). I see the release note

Note that SAM conversion only applies to lambda expressions, not to arbitrary expressions with Scala FunctionN types

However, I would think that irrespective of this item and the note:

Eta-expansion (conversion of a method to a function value) of zero-args methods has been deprecated, as this can lead to surprising behavior

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 29, 2016

Member

No, render _ is a method value, not eta-expansion -- that would simply be render. Eta expansion refers to the equivalence between an expression of function type that's not a function literal, and the function literal that applies that expression to the expected arguments. For the 1-argument case, eta-expansion turns e into x => e(x).

Member

adriaanm commented Nov 29, 2016

No, render _ is a method value, not eta-expansion -- that would simply be render. Eta expansion refers to the equivalence between an expression of function type that's not a function literal, and the function literal that applies that expression to the expected arguments. For the 1-argument case, eta-expansion turns e into x => e(x).

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 29, 2016

Member

To elaborate a bit, because this is pretty confusing in the spec. The intent of the warning is to act only on implicit eta-expansion, not the explicit kind that is triggered by method value syntax.

Method values are specified here: http://www.scala-lang.org/files/archive/spec/2.12/06-expressions.html#method-values:

The expression e _ is well-formed if e is of method type or if e is a call-by-name parameter. If e is a method with parameters, e _ represents e converted to a function type by eta expansion.

Note that this means that e _ cannot have a method type, and is thus not subject to eta-expansion (it is explicit notation for triggering eta-expansion).

On eta-expansion:

6.26.5 Eta Expansion

Eta-expansion converts an expression of method type to an equivalent expression of function type.

Member

adriaanm commented Nov 29, 2016

To elaborate a bit, because this is pretty confusing in the spec. The intent of the warning is to act only on implicit eta-expansion, not the explicit kind that is triggered by method value syntax.

Method values are specified here: http://www.scala-lang.org/files/archive/spec/2.12/06-expressions.html#method-values:

The expression e _ is well-formed if e is of method type or if e is a call-by-name parameter. If e is a method with parameters, e _ represents e converted to a function type by eta expansion.

Note that this means that e _ cannot have a method type, and is thus not subject to eta-expansion (it is explicit notation for triggering eta-expansion).

On eta-expansion:

6.26.5 Eta Expansion

Eta-expansion converts an expression of method type to an equivalent expression of function type.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 29, 2016

Member

Personally, I would prefer to deprecate method value syntax in favor of writing out the function literal. Underscore already means too many things.

Member

adriaanm commented Nov 29, 2016

Personally, I would prefer to deprecate method value syntax in favor of writing out the function literal. Underscore already means too many things.

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Nov 29, 2016

Member

Personally, I would prefer to deprecate method value syntax in favor of writing out the function literal. Underscore already means too many things

that's now scala/scala-dev#273

Member

SethTisue commented Nov 29, 2016

Personally, I would prefer to deprecate method value syntax in favor of writing out the function literal. Underscore already means too many things

that's now scala/scala-dev#273

retronym added a commit to retronym/scala that referenced this pull request Jul 8, 2017

Fix regression with eta expansion of implicit method
In scala#5327, a change was made to typedEta to accept an
original (ie, pre-typechecked) tree to be used in a
fallback path.

However, the caller provided an original tree larger
than the actual tree being typechecked.

This commit just passes the part of the orig tree that
corresponds to the tree we're eta expanding, rather than
the entire `Typed(methodValue, functionPt)` tree.

That avoids an infinite loop in typechecking the erroneous
code in the test case.

adriaanm added a commit to retronym/scala that referenced this pull request Sep 18, 2017

Fix regression with eta expansion of implicit method
In scala#5327, a change was made to typedEta to accept an
original (ie, pre-typechecked) tree to be used in a
fallback path.

However, the caller provided an original tree larger
than the actual tree being typechecked.

This commit just passes the part of the orig tree that
corresponds to the tree we're eta expanding, rather than
the entire `Typed(methodValue, functionPt)` tree.

That avoids an infinite loop in typechecking the erroneous
code in the test case.

adriaanm added a commit to retronym/scala that referenced this pull request Sep 20, 2017

Fix regression with eta expansion of implicit method
In scala#5327, a change was made to typedEta to accept an
original (ie, pre-typechecked) tree to be used in a
fallback path.

However, the caller provided an original tree larger
than the actual tree being typechecked.

This commit just passes the part of the orig tree that
corresponds to the tree we're eta expanding, rather than
the entire `Typed(methodValue, functionPt)` tree.

That avoids an infinite loop in typechecking the erroneous
code in the test case.

@eed3si9n eed3si9n referenced this pull request Feb 13, 2018

Open

Audit `-Xfuture` for 2.13 #471

5 of 5 tasks complete

eed3si9n added a commit to eed3si9n/scala that referenced this pull request Apr 8, 2018

Remove eta-expansion of zero-arg method values
Fixes scala/bug#7187

scala#5327 deprecates eta-expansion of zero-arg method values. This removes it in 2.13.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment