SIP-NN - Allow referring to other arguments in default parameters #653

pathikrit · 2017-01-11T06:31:20Z

No description provided.

…arameters

soronpo · 2017-01-11T11:05:45Z

Quick note - give the SIP a proper title (leave it as SIP-NN until it receives an official number)

jvican · 2017-01-11T18:27:52Z

Thanks for submitting this proposal @pathikrit! Will review in a bit. We're reviewing this at the end of this month, I'm preparing the SIP meeting in the next few days and will let you know the exact meeting date soon.

jvican · 2017-01-11T22:55:09Z

sips/pending/_posts/2017-01-11-refer-other-arguments-in-args.md

+The other more verbose alternative is by overloading:
+
+```scala
+def substring(s: String, start: Int = 0, end: Int = s.length): String


This is not working for me, I get <console>:12: error: not found: value s in the console.

Even if that worked you still can't have two methods with the same type signature.. This needs attention.

Indeed. The trick here is scalac's generation of synthetic methods for the default args.

Oops, a typo. Fixed.

jvican · 2017-01-11T22:55:33Z

sips/pending/_posts/2017-01-11-refer-other-arguments-in-args.md

+| Jan 12th 2017 | Initial Feedback |
+
+## Introduction
+Currently there is no way to refer to other arguments in the default parameters list:


Adding a motivation section that showcases why this is useful would be great!

Working on it...

jvican · 2017-01-11T22:56:32Z

sips/pending/_posts/2017-01-11-refer-other-arguments-in-args.md

+def substring(s: String, start: Int = 0)(end: Int = s.length): String
+```
+
+However, the above workaround is not always suitable in certain situations.


Why? Any examples or cases in which the previous transformation cannot be done? I get what you mean here, but explaining it a little bit more could be good for the upcoming review 😄.

jvican · 2017-01-11T23:01:46Z

sips/pending/_posts/2017-01-11-refer-other-arguments-in-args.md

+```
+
+#### Referring to type members:
+The default parameters should be able to refer to type members of other arguments e.g.:


This section seems orthogonal to the target of this PR (this issue was raised by @olafurpg). I think that the good approach here would be to create another SIP that keeps track of this feature, since I would expect the implementation of this to be different to the one requested. WDYT?

I don't know much about scalac internals to comment. Maybe it is very similar (then keep 1 SIP) and maybe it isn't (2 SIPs). Can someone more familiar with scalac comment?

I've been looking into the implementation details of this and I can confirm that they are orthogonal issues. Concretely, referring to type members is tricky. I think it can be done but the feature interaction is scary. Could someone confirm? Maybe @xeno-by || @sjrd?

I think they're similar in spirit, but I'm sure they're distinct in the implementation.

To avoid a situation where the committee pushes you back a month to go look into and document this, I would keep it in. At the very least they can ask you to split it out - you can then drop it or pursue it as a separate SIP.

jvican

Just a first pass on the document requesting some changes to make it clearer. I think the proposed features are great, thank you for the time to write this up!

xeno-by · 2017-01-12T17:08:53Z

sips/pending/_posts/2017-01-11-refer-other-arguments-in-args.md

+#### Multiple Implicit Parameters
+[Multiple implicit parameters](https://github.com/scala/scala.github.com/pull/520) should also be allowed to refer to one another (left to right):
+```scala
+def codec[A](data: A)(implicit encoder: Encoder[A])(implicit decoder: Decoder[A] = encoder.reverse) // Legal


Am I missing something, or this is not legal Scala syntax (multiple implicit parameter lists)?

I think that what @pathikrit means is that this change should also be valid for implicit parameter lists. That would marry well with this SIP.

See the link to the PR

xeno-by · 2017-01-12T17:10:13Z

sips/pending/_posts/2017-01-11-refer-other-arguments-in-args.md

+---
+layout: sip
+disqus: true
+title: SIP-NN - Allow referring to other arguments in default parameters


"Other arguments" or "previous arguments"?

Maybe previous I guess if other is not feasible (circle dependencies)?

I would actually write "Allow references to left arguments of the same list in default parameters" if @pathikrit agrees with my previous analysis that doing it for all the arguments is unfeasible. 😄

xeno-by · 2017-01-12T17:14:07Z

sips/pending/_posts/2017-01-11-refer-other-arguments-in-args.md

+AFAIK, there are no major languages where referring to parameters declared to the ***right*** is allowed.
+
+### Discussions
+[Scala Lang Forum](https://contributors.scala-lang.org/t/refer-to-previous-argument-in-default-argument-list/215/6)


I think it would be useful to have an implementation sketch to accompany this SIP. In my opinion, information about why this proposal hasn't been implemented yet and why it is feasible would help the potential discussion during one of the upcoming SIP meetings.

I've been looking into this today and will submit an implementation sketch very soon, along with a design document /cc @pathikrit

The following changes implement a more flexible encoding of default args that allows default implementation to depend on parameters defined on the left (either previous parameters lists or parameters in the same parameter list defined on the left). This encoding breaks binary compatibility so it targets 2.13. It consists of changes in the synthetic method generation of defaults and the call sites of these methods. The implementation of synthetic methods now depends on any parameter defined at the left of a default arg, even if it their body does not use these parameters. This is done so that changing the body of the default args does not change the signature and hence break binary compatibility. More explanation on this implementation can be found in the [SIP proposal](scala/docs.scala-lang#653). WIP: This commit still needs more work. This is what's missing: 1. Reusing the results of default args. Currently, invocations to default methods are inlined at the call-site and a lot of invocations can be reused. Declaring more than one default arg considerably increases bytecode size. My idea to tackle this issue is to save the results of the invocation in vals before the the call-site invocation. Example: ``` // definition def foo(a: Int, b: Int = a, c: Int = a + b): Int = a + b + c // invocation foo(1) ``` will modify the call-site like: ``` foo(1, foo$default$2(1), foo$default$3(1, foo$default$2(1))) ``` As you see, invocations to 1 and `foo$default$2` could be optimized: ``` val a$default = 1 val b$default = foo$default$2(a$default) val c$default = foo$default$3(a$default, b$default) foo(a$default, b$default, c$default) ``` This generates more bytecode for call-sites omitting default arguments, but avoids unnecessary calls that could cause bad performance (especially if more than 2 default args are defined in a method). 2. Generate more tests for the default arguments in class.

jvican · 2017-01-13T18:19:22Z

@pathikrit I've done some research into this proposal to shed some light into the way scalac encodes default args and how we can improve it according to what you suggest in this proposal. After some experimentation and an implementation prototype, I've decided to document my findings and explore different ways in which this feature can be implemented.

Current implementation

Let's see a simple example of a method with defaults args:

def foo(a: Int, b: Int = 2, c: Int = 3): Int = a + b + c

The way scalac encodes this feature is by generating synthetic methods for every
default argument (see Namers.scala). Namers will find the method definition
foo and convert it into the equivalent of:

def foo(a: Int, b: Int = 2, c: Int = 3): Int = a + b + c
def foo$default$2() = 2
def foo$default$3() = 3

Therefore, when it finds a call site like foo(1), NamesDefaults.scala will transform it into:

foo(1, foo$default$2(), foo$default$3())

As you see, the synthetic methods created don't take parameters. This is what
currently constraints dependencies between parameters defined in the same
parameter list.

What happens if we define the dependencies between curried parameters lists though?

def foo(a: Int)(b: Int = a, c: Int = a): Int = a + b + c
def foo$default$2(a: Int) = 2
def foo$default$3(a: Int) = 3

is transformed into:

foo(1, foo$default$2(1), foo$default$2(1))

In this case, scalac generates synthetic methods that take a as parameter, as it's the only parameter in previous parameter lists. This is done even if the default implementation of b and c does not depend on a. Otherwise, changing the default arguments' implementation will change the signature of the synthetic methods and break binary compatibility.

Note that these are simplified examples of default args. The current implementation takes care of more details, for example it copies type parameters of foo over the synthetic methods. This usually happens for both higher-kinded functions and constructors.

How can we implement the changes proposed in this proposal?

Exploring other encodings

First attempt

Our goal with the new encoding is to allow dependencies between parameters defined in the same parameter list, both on the right and on the left. The following method would be allowed:

// definition
def foo(a: Int, b: Int = c, c: Int = a): Int = a + b + c
// invocation
foo(a)

Let's see if we can find a way to implement it. First, we start with the same encoding used by dependencies in curried parameter lists and modify it to be more flexible. Let's create synthetic methods that define all the parameters that a given default argument could depend on. We'll get the following synthetic methods:

def foo$default$2(a: Int, c: Int) = c // default implementation of b
def foo$default$3(a: Int, b: Int) = a // default implementation of c

As b can theoretically depend on a and c, we need to express that potential dependency in the synthetic method definition, and the same for the synthetic method for c. However, this encoding produces a recursive dependency between b and c. To compute the default value of b, we need to invoke foo$default$2 with an a and c, but c needs to be computed as well and it recursively depends on b. Unfortunately, there is no way we can generate code for the call foo(a).

Second attempt

The previous encoding didn't work because of recursive dependencies in the default args. Why are we generating synthetic methods that could express all the dependencies of an argument? Let's just do it for the parameters that the default arguments actually depend on.

def foo$default$2(c: Int) = c // default implementation of b
def foo$default$3(a: Int) = a // default implementation of c

Now that we have removed the circular dependency, we can generate code for the call site:

foo(a, foo$default$2(foo$default$3(a)), foo$default$3(a))

However, this encoding is worse than we thought. We are encoding the parameter dependency in the signature of the synthetic methods, which means that if we change the implementation of b or c, we'll also change the generated binary signature. This will break users' binaries because the types for the default invocations won't match. This encoding, while feasible, it's too strict for developers to use as it will render unusable already compiled binaries.

Can we do better?

Third and last attempt

At this point, we can only constrain the scope of this proposal to make the default args implementation feasible. Another way to remove the circular dependency that we created in the first encoding attempt is to constrain the parameter dependencies. Instead of allowing any parameter dependency, we will only allow dependencies for:

parameters of previous parameter lists
parameters defined on the left of a default arg

Therefore, the method we started with is not valid anymore -- b cannot depend on c. Let's redefine foo:

// definition
def foo(a: Int, b: Int = a, c: Int = a + b): Int = a + b + c
// invocation
foo(a)

With our new limitation, these are the generated synthetic methods:

def foo$default$2(a: Int) = a // default implementation of b
def foo$default$3(a: Int, b: Int) = a + b // default implementation of c

A similar transformation applies to curried methods def foo(a: Int)(b: Int = a, c: Int = a +b).

Fortunately, our new scheme allow us to:

Keep binary compatibility no matter what the implementation of b or c is.
Allow implementations of default args to depend on parameters defined on the left.

I think that binary compatibility is utterly important for Scala. I suggest that we don't allow default implementations to depend on arguments defined both on the left and the right, only those on the left. This is closer to what we have right now and it strikes a good tradeoff between usability and power. This encoding will lift a common restriction of the Scala language and enforce a simple rule on library authors that can safely replace default args implementation without thinking in binary compatibility.

jvican · 2017-01-16T22:37:09Z

FYI @pathikrit: @sjrd will be the reviewer for this proposal. You can talk to him or me for any doubt you have along the process. He will be the one giving you feedback and leading the discussions in our next meeting.

soronpo · 2017-02-14T10:13:14Z

@pathikrit I suggest you checkout how this proposal looks on the site, and fix the annotations. I recommend that you build the site locally and view the HTML version of your proposal. The MD to HTML translation is different than what is viewed by an MD editor. See #689

First draft of SIP to allow referring to other arguments in default p…

e38ca6d

…arameters

jvican changed the title ~~SIP to allow referring to other arguments in default parameters~~ SIP-NN - Allow referring to other arguments in default parameters Jan 11, 2017

jvican added the Ongoing SIP label Jan 11, 2017

Update 2017-01-11-refer-other-arguments-in-args.md

d665502

jvican reviewed Jan 11, 2017

View reviewed changes

xeno-by reviewed Jan 12, 2017

View reviewed changes

jvican mentioned this pull request Jan 13, 2017

WIP: Add prototype improving default args encoding scala/scala#5641

Closed

jvican assigned sjrd Jan 16, 2017

Minor review suggestions

04ba8c3

jvican merged commit 3ec289f into scala:master Feb 13, 2017

jvican mentioned this pull request Apr 11, 2017

February SIP Meeting Minutes #762

Merged

lrytz mentioned this pull request Mar 1, 2018

Add support for multiple trailing implicit parameter lists. scala/scala#5108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SIP-NN - Allow referring to other arguments in default parameters #653

SIP-NN - Allow referring to other arguments in default parameters #653

pathikrit commented Jan 11, 2017

soronpo commented Jan 11, 2017

jvican commented Jan 11, 2017

jvican Jan 11, 2017

dwijnand Jan 12, 2017

jvican Jan 12, 2017

pathikrit Jan 16, 2017

jvican Jan 11, 2017 •

edited

Loading

pathikrit Jan 16, 2017

jvican Jan 11, 2017

pathikrit Jan 16, 2017

jvican Jan 11, 2017 •

edited

Loading

pathikrit Jan 16, 2017

jvican Jan 16, 2017

dwijnand Jan 16, 2017

jvican left a comment

xeno-by Jan 12, 2017

jvican Jan 12, 2017

pathikrit Jan 16, 2017

xeno-by Jan 12, 2017

pathikrit Jan 16, 2017

jvican Jan 16, 2017

xeno-by Jan 12, 2017

jvican Jan 12, 2017 •

edited

Loading

jvican commented Jan 13, 2017 •

edited

Loading

jvican commented Jan 16, 2017

soronpo commented Feb 14, 2017

SIP-NN - Allow referring to other arguments in default parameters #653

SIP-NN - Allow referring to other arguments in default parameters #653

Conversation

pathikrit commented Jan 11, 2017

soronpo commented Jan 11, 2017

jvican commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

jvican commented Jan 13, 2017 • edited Loading

Current implementation

Exploring other encodings

First attempt

Second attempt

Third and last attempt

jvican commented Jan 16, 2017

soronpo commented Feb 14, 2017

jvican Jan 11, 2017 •

edited

Loading

jvican Jan 11, 2017 •

edited

Loading

jvican Jan 12, 2017 •

edited

Loading

jvican commented Jan 13, 2017 •

edited

Loading