-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Replace runtime.Rich* implicits on primitives by direct extension methods. #23872
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
Conversation
...hods. It has always been weird that the public API of primitive types, enriched by `Predef` depends on the `Rich*` classes, which are in the not-really-public `runtime` package. Moreover, the design of those classes was overabstracted, with consequences on performance (unnecessary boxing) and quality of the API (including methods that do not make sense on some types). To mitigate that, the individual `Rich*` classes redefined some (but not all) of the methods, defeating the abstraction. We solve both issues with a simple solution: define all those methods as simple `extension` methods. We do this directly in the companion objects of the primitive types.
@sjrd you have beaten me to it 😄
The problem is that extension methods are not equivalent to implicit classes/definition extensions. Without solving https://contributors.scala-lang.org/t/relaxed-extension-methods-sip-54-are-not-relaxed-enough/6585/1 this change will break everywhere that has methods in the same name in scope.
The problem is that extension methods are not equivalent to implicit classes/definition extensions. Without solving https://contributors.scala-lang.org/t/relaxed-extension-methods-sip-54-are-not-relaxed-enough/6585/1 this change will break everywhere that has methods in the same name in scope.
I don't see how that problem appears when defining extensions for monomorphic types. You wouldn't redefine another extension of toHexString for Int (or if you do, it's on you), so you won't have a clash like that. With polymorphic types there are reasons to define separate extensions for Foo[Int] and Foo[String], but for a monomorphic type that's not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why this was done, but it feels weird to say that an extension method was deprecated since 2.12.15 😅
The problem is that extension methods are not equivalent to implicit classes/definition extensions. Without solving https://contributors.scala-lang.org/t/relaxed-extension-methods-sip-54-are-not-relaxed-enough/6585/1 this change will break everywhere that has methods in the same name in scope.
I don't see how that problem appears when defining extensions for monomorphic types. You wouldn't redefine another extension of
toHexStringforInt(or if you do, it's on you), so you won't have a clash like that. With polymorphic types there are reasons to define separate extensions forFoo[Int]andFoo[String], but for a monomorphic type that's not the case.
You are right. I thought the implementation used a typeclass approach + extension methods. My mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is part of the very little subset where everyone agrees on it :). Same goes for to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is strictly a refactoring. Adding infix anywhere should be done separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we view the PR this way, then okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(my personal preferences here)
It has always been weird that the public API of primitive types, enriched by
Predefdepends on theRich*classes, which are in the not-really-publicruntimepackage.Moreover, the design of those classes was overabstracted, with consequences on performance (unnecessary boxing) and quality of the API (including methods that do not make sense on some types). To mitigate that, the individual
Rich*classes redefined some (but not all) of the methods, defeating the abstraction.We solve both issues with a simple solution: define all those methods as simple
extensionmethods. We do this directly in the companion objects of the primitive types.This would be a required step before we do anything about #23824.