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

Add support for multiple trailing implicit parameter lists. #5108

Closed
wants to merge 1 commit into from

Conversation

milessabin
Copy link
Contributor

@milessabin milessabin commented Apr 20, 2016

Astonishingly this is a simple syntactic restriction. On removing the restriction in the parser all call sites are interpreted exactly as one would hope. I considered enforcing the presence of the implicit keyword in each follow-on parameter block, but that seems to only add syntactic noise.

This change is binary compatible in both directions, and all previously legal programs have the same meaning.

The benefit is that in many circumstances the Aux pattern becomes unnecessary.

@scala-jenkins scala-jenkins added this to the 2.12.0-M5 milestone Apr 20, 2016
@sjrd
Copy link
Member

sjrd commented Apr 20, 2016

I considered enforcing the presence of the implicit keyword in each follow-on parameter block, but that seems to only add syntactic noise.

I don't think it's noise. It should be enforced, I think. The implicit keyword is definitely inside the parentheses, so it's completely non-obvious that it silently propagates to all subsequent parameter lists.

@milessabin
Copy link
Contributor Author

@sjrd I think to a certain extent that depends on how you interpret the keyword now. I've gotten used to the idea that implicit flags that all subsequent parameters are implicit. I've also seen people write (implicit a: A, implicit b: B, ...) because they think of the modifier as being attached to the parameter rather than the parameter list.

This certainly isn't something I'd want to dig my heels in over though. I really just wanted to get the smallest possible change in first to get initial feedback.

@milessabin
Copy link
Contributor Author

See also SI-4718, this explanation of the Aux pattern and this gist.

@SethTisue
Copy link
Member

This would need an accompanying spec update.

@milessabin
Copy link
Contributor Author

Happy to add that.

@kmizu
Copy link
Contributor

kmizu commented Apr 20, 2016

I'd like to know the reason why the syntactic was added in past times. I think that it's too earlier to merge this PR without knowing the reason. Of course, the big reason may not exist.

@kmizu
Copy link
Contributor

kmizu commented Apr 20, 2016

Although I find there is the same restriction in the oldest commit in GitHub, no reason of the restriction is written:

6b1d01b#diff-e059ce64777235990044f1624042e5d0

@cvogt
Copy link
Member

cvogt commented Apr 20, 2016

Agreed that the position of implicit keyword for argument lists is unintuitive with regard to it's scope already today. I personally remember the exact confusion Miles describes about accidentally repeating it way back when I started. So since it is non-intuitive already, maybe less noise is the better choice?

I know Martin considered this feature before. Not sure what the hold back was. Maybe just worry about adding more features?

@odersky
Copy link
Contributor

odersky commented Apr 20, 2016

Since it's a spec change it will require a SIP. Happy to discuss it, of course.

At first glance I am not quite sure what the use case is. I mean I see the use case for mixing implicit parameter sections with non-implicit ones, but that's not what the PR covers. We also should study possible interactions with implicit function types.

@Ichoran
Copy link
Contributor

Ichoran commented Apr 20, 2016

I would think that the use case is to have an implicit that you often want to replace manually first, and then have the others filled in automatically. I run into this all the time, actually, and end up getting around it with default parameters which is strictly less powerful, and also kind of surprising (in that it takes some thought to convince oneself that an implicit in scope should be chosen before a default).

@kailuowang
Copy link

kailuowang commented Apr 20, 2016

@odersky, @milessabin and other people can probably speak about it better than me, but I think the main use case is to support implicit parameters whose type is (or constructed from) dependent type on previous implicit parameters.
Without this fix it's not possible to do

trait A { type T } 
def f(implicit a: A, b: a.T)

with this fix you can do

def f(implicit a: A)(b: a.T) 

where both a and b can be implicitly found.

@adelbertc
Copy link

I've found myself hitting this multiple times for the same reasons as @Ichoran and @kailuowang

@SethTisue
Copy link
Member

SethTisue commented Apr 20, 2016

the main use case is to support implicit parameters whose type is (or constructed from) dependent type on previous implicit parameters

has the possibility been explored of allowing this within a single implicit parameter list, rather than requiring going to multiple parameter lists?

@cvogt
Copy link
Member

cvogt commented Apr 20, 2016

@SethTisue type inference flows in any direction within arguments lists but left to right between them, so I don't think that's an option without major changes in how type inference works and making implicit argument lists work differently from ordinary ones. Allowing multiple implicit argument lists seems more realistic in my eyes.

@propensive
Copy link
Contributor

@cvogt That's not actually true of implicit parameter lists, though: they're evaluated parameter-by-parameter.

@cvogt
Copy link
Member

cvogt commented Apr 20, 2016

@propensive you are right. Implicit resolution is left to right. So I guess @SethTisue's suggestion could work. I'd prefer multiple lists now over one but maybe never though ;).

@raulraja
Copy link

+1 as others have already mentioned, lifting this restriction would simplify and reduce the boilerplate in many cases where we are trying to resolve dependent types from existing implicit evidences in scope in our code.

@alexy
Copy link

alexy commented Apr 21, 2016

Supportive of this, and a SIP is needed. Perhaps we can leverage the Scala Center to prioritize SIPs like these to fast-track them where the community has laid the groundwork and the case is clear, to act in general, and then iterate on the specifics (i.e. @SethTisue's idea or the original multiple lists).

@odersky
Copy link
Contributor

odersky commented Apr 21, 2016

Agreed, dependent typing of implicits is an important use case. There's also the other related one, where we would like to make a normal parameter type dependent on an implicit parameter. This would require a more far-reaching change, but would be a more thorough solution.

Btw, I am surprised that simply allowing multiple implicits seems to work. There are several places in the compiler where we skip over a possible implicit section to get at the result type. Either these all take already multiple implicits into account, or we have another sign that compilers are really hard to test completely.

@milessabin
Copy link
Contributor Author

milessabin commented Apr 21, 2016

Allowing a non-implicit parameter to depend on an implicit parameter implies that we want to support implicit and non-implicit parameter lists interspersed. I think that would be highly desirable in the longer term, especially if this is something that Dotty is actively considering.

That is a somewhat larger change than I'm proposing here however, and instead I suggest that we limit the change now, but make it easy to evolve in the Dotty direction later.

The tricky part with interspersing implicit and explicit parameter lists is knowing which is which at call sites when implicit argument lists can be provided explicitly. The way that this is done in languages like Agda and Idris is by using a different bracket style for providing implicit arguments explicitly, and I believe that this is being considered for Dotty as well.

I think that would be a change too far for Scala in the very short term. However, if we stick with implicit parameter lists only appearing at the end, and also follow @sjrd's advice that each implicit parameter list must be flagged with the implicit keyword, then it should be possible to relax that constraint later, allowing mixed implicit and non-implicit parameter lists, without too much difficulty.

@ghik
Copy link
Contributor

ghik commented Apr 21, 2016

If we consider allowing interspersing of explicit and implicit parameter lists then I think it would make sense to also allow interspersing them with type parameter lists (SI-4719 could be fixed as part of this work). This would provide much better control over type inference.

I totally support the idea of separate, unambiguous syntax for explicit passing of implicit params, e.g.

someFuture.map(someFunction)(implicit someExecutionContext)

With this we could also get rid of problems like this one (third example)

@milessabin
Copy link
Contributor Author

@ghik agreed that that would be highly desirable. However ... let's not let the best be the enemy of the good. So long as what's proposed here can evolve smoothly into what you're suggesting we don't need everything, all at once. Baby steps ... we can leave the giant leaps to Dotty.

@mgttlinger
Copy link

SIP PR for these changes: scala/docs.scala-lang#520

@adriaanm
Copy link
Contributor

Thank you, @milessabin! This an interesting and very exciting feature to explore, but we cannot change 2.12 this late in the development cycle, and not without a SIP that has gone through the due diligence process. I do support experimenting with different designs for this in 2.12.x, if we can put it behind a flag without overly complicating the code or slowing down the compiler (seems very doable).

Most importantly, as hard as it is to add new features, it's even harder to remove them. I don't want us to rush into this only to have to rethink and break existing code in 2.13 when refinements become apparent. We really need to give this the consideration it deserves before we settle on the design.

To summarize some of the vectors that span the design space:

  • one implicit argument list with relaxed rules for dependencies between arguments or multiple lists, preserving scoping of arguments?
  • if we do multiple implicit argument lists, should we rethink the discrepancy between explicit and implicit argument lists in terms of left-to-right inference?
  • are all argument lists following the first implicit one implicitly implicit, or do we allow interspersing or must the keyword be explicit without interspersing allowed
  • should implicit value arguments be treated uniformly with implicit type arguments, with [] denoting "typically" provided by compiler and () lists being supplied by programmers, and with both [] and () being allowed multiple lists, named arguments, default ones,... (this is my personal preference)

PS: I share @odersky's surprise that this "just works". I'm certain this will turn out to be more involved.

@milessabin
Copy link
Contributor Author

milessabin commented Apr 22, 2016

I would be happy to put this behind a flag so that we can experiment with it in the 2.12 cycle.

@retronym
Copy link
Member

retronym commented Apr 23, 2016

I guess this works because of the idiom:

   case mt @ MethodType(params, restpe) =>
          if (mt.isImplicit)
            loop(restpe, pt)

Used in Implicits#{checkCompatibility}, Infer#normalize,

I have highlighted all uses of MethodType#isImplicit with a (currently empty) comment in 2.12.x...retronym:review/5108 . We need to review them to try to find any assumptions that will be broken by multiple implicit arg lists.

@retronym
Copy link
Member

@milessabin I've gone through the list of usages and added comments in retronym#20

I only identified one or two more places that need adaptation.

@milessabin
Copy link
Contributor Author

@retronym thanks for that! I'll work through those and update the PR with the result and a flag ASAP.

@milessabin
Copy link
Contributor Author

This change is now hidden behind -Ymulti-implicit-params.

I've also incorporated @retronym's review comments.

@adriaanm adriaanm self-assigned this Apr 26, 2016
@djspiewak
Copy link
Member

This is an awesome change, but I do echo @adriaanm's concerns about this landing in 2.12. Especially since it's just a parser change, I'm leery of even hiding it behind a flag, since only the declaration site must be compiled with the flag in order to introduce foreign semantics at the call site. This is undeniably useful for frameworks like shapeless, but it also makes this a very dangerous sort of thing to "flag out".

I would vote for starting the SIP process and getting this into 2.13 as a priority. It makes a huge difference in any sort of type level programming, opening up a whole slew of things which are nearly (or actually) impossible right now.

@soc
Copy link
Member

soc commented Apr 28, 2016

While I share the concerns, if people are positive about this change, I'd prefer to have it in 2.12 as well as back-ported to 2.11.

If things don't work out in the end it's way less painful to have a deprecated Y flag, than having a successful Y flag but forcing library authors to deal with the mess of supporting 2.11-2.12-2.13 where some versions have the flag and some don't.

(Same with SI-2717)

@adriaanm adriaanm modified the milestones: 2.12.0-M5, WIP May 23, 2016
@adriaanm
Copy link
Contributor

For the concerns listed above, I don't feel comfortable merging this in M5, so I've adjusted the milestone.

@milessabin
Copy link
Contributor Author

Understood. Can you say what you mean by "adjusted the milestone"? Does that mean "maybe later in the 2.12.x cycle"?

Also, if there's more time, would you be open to a more ambitious change (still hidden behind a flag), supporting multiple interspersed implicit parameter blocks via an Agda/Idris/Dotty like syntax change?

@adriaanm
Copy link
Contributor

Yes to both interpretations. Later in 2.12.x with more discussion, experimentation and ambition :-)

@som-snytt
Copy link
Contributor

som-snytt commented Jun 4, 2016

I was reminded of a related spec clarification at

https://issues.scala-lang.org/browse/SI-9494

where implicit in class params is less obvious.

...where he said: "What does 'override implicit' mean?" is a good ice breaker for a job interview.

@SethTisue
Copy link
Member

closing for inactivity. we can always reopen if activity resumes

@SethTisue SethTisue closed this Feb 24, 2018
@milessabin
Copy link
Contributor Author

Yup, I'll try again and aim a little higher.

@fommil
Copy link
Contributor

fommil commented Feb 27, 2018

@milessabin if there is any help you need over the coming weeks? I am sponsoring Vladimir for 2 more weeks to work on scalafix but he will probably finish early, so I could ask him to help you out, instead (I'd really like multiple implicits). He's not familiar with the codebase though.

@propensive
Copy link
Contributor

Or even just lifting the restriction on referring to earlier implicit values from within the same block would be good.

@milessabin
Copy link
Contributor Author

@propensive I think that the left to right rule is worth keeping.

@milessabin
Copy link
Contributor Author

@fommil if you or Vladimir have cycles to spare I'd point them at scala/scala-dev#474.

@fommil
Copy link
Contributor

fommil commented Mar 1, 2018

@milessabin yeah, I'd really love that one too... but without a well defined spec there isn't much Vladimir can do and it looks like 474 is heading for a protracted debate. There is plenty in scalafix to keep him busy that he stands a much better chance of completing 😄

@propensive
Copy link
Contributor

@milessabin I think you're misunderstanding what I'm suggesting. I want to be be able to write,

def foo[A](implicit x: Foo[A], y: Bar[x.Out]) = ...

but agree it would be unreasonable to support,

def foo[A](implicit y: Bar[x.Out], x: Foo[A]) = ...

So there should be no change to the left-to-right evaluation semantics.

@fommil
Copy link
Contributor

fommil commented Mar 1, 2018

Isn't this what people have been calling "auto currying"?

@propensive
Copy link
Contributor

Not to my knowledge, though I can believe it.

@lrytz
Copy link
Member

lrytz commented Mar 1, 2018

def foo[A](implicit x: Foo[A], y: Bar[x.Out]) = ...

This would be useful in general (not only on implicit parameter lists), and also for default arguments. Being discussed here scala/docs.scala-lang#653 / #5641

@propensive
Copy link
Contributor

@lrytz Interesting! I'll have to spend some time reading the discussions. My feeling was that calculating LUBs across types with dependencies between them would be a nonstarter, though while not soluble in the general case, there are certainly many cases which obviously are. Though it seems like a big change.

@SethTisue SethTisue removed this from the WIP milestone Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet