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

Fix #6190: eta-expand companion object if functions are expected #7207

Open
wants to merge 9 commits into
base: master
from

Conversation

@liufengyun
Copy link
Contributor

liufengyun commented Sep 12, 2019

Fix #6190: eta-expand companion object if functions are expected

As suggested by @odersky in #6190, we stop generating function types as parents for companion objects of case class. To compensate, we insert C.apply and eta-expand if the expected type is a function type.

Clarification:

In the following code, .tupled cannot be omitted --- auto tupling does not work here.

case class Foo(key: Int, value: String)

val a = Map(1 -> "a", 2 -> "b").map(Foo.apply.tupled)
@sjrd

This comment has been minimized.

Copy link
Member

sjrd commented Sep 14, 2019

I believe this is good migration story for dropping the automatic function type parent.

However, I'd like this to only be a migration story, and not encourage new code to use that "feature" for any kind of object with an apply method. So I would suggest that the compiler emits a warning when it eta-expands an object.

The reason is that

  • Either we also do this for any other term, not just objects, and it will be very confusing because that would essentially become a silent implicit conversion that we cannot get rid of, and that is not even type-based!
  • Or we have a weird inconsistency between terms that happen to refer to objects and ones that do not. Which is not much better.

So I would recommend: do it only for objects, and warn when you do it that it is just a migration thing, and that they should either eta-expand themselves or write .apply themselves.

@liufengyun

This comment has been minimized.

Copy link
Contributor Author

liufengyun commented Sep 14, 2019

@sjrd The insertion only happens for the companion object of a case class. And a warning is issued as suggested.

@liufengyun liufengyun force-pushed the dotty-staging:fix-6190 branch from 1b04fb6 to e629143 Sep 15, 2019
@liufengyun liufengyun requested a review from odersky Sep 20, 2019
@odersky

This comment has been minimized.

Copy link
Contributor

odersky commented Sep 30, 2019

While we are at it, should we drop the auto-generated apply method as well? It's no longer needed since we can now construct instances without new anyway. This step would remove some major complication about suppressing auto-generated apply methods that clash with user-defined ones.

@liufengyun

This comment has been minimized.

Copy link
Contributor Author

liufengyun commented Sep 30, 2019

While we are at it, should we drop the auto-generated apply method as well?

It's a nice idea. The only concern is migration. From the experience with community build, there is a significant amount of code using companion objects as function values for map. If we can compensate for that, then migration is not a problem.

@odersky

This comment has been minimized.

Copy link
Contributor

odersky commented Oct 26, 2019

Looking at the extensive changes this needs in the community build I am a bit nervous to do this now. The breakage might be too extensive, in particular in connection with tupled. Is there a way to apply automatic source code rewriting to mitigate this?

@liufengyun

This comment has been minimized.

Copy link
Contributor Author

liufengyun commented Oct 28, 2019

I think rewriting is possible to be implemented in scalafix.

Meanwhile, I think it would be better to postpone changes in this PR until 3.1, so that it will not create more hassles in the migration.

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