-
Notifications
You must be signed in to change notification settings - Fork 46
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 RuntimeConstraint #175
Conversation
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.
The POC is ok to me. You'll just need to do the same changes in modules' RefinedTypeOps
extension (like cats
and zio
).
case IronType[a, c] => RuntimeConstraint.Impl[a, c, T] | ||
|
||
object RuntimeConstraint: | ||
final class Impl[A, C, T](_test: A => Boolean, val message: String): |
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.
Why does it need to have a third T
type parameter?
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 type parameter is required for opaque types so one can do something alongside what's below. It seems that the unnecessary type param was C
in this case
object One {
opaque type Foo = Int :| GreaterEqual[10]
object Foo extends RefinedTypeOpsImpl[Int, GreaterEqual[10], Foo]
}
object Two {
import One.Foo
def doSomething(a: Int)(using RuntimeConstraint[Int, Foo]): Foo = ???
}
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 don't see why it should have a different shape than Constraint[A, C]
:
final class RuntimeConstraint[A, C](_test: A => Boolean, val message: String):
inline def test(value: A): Boolean = _test(value)
trait RefinedTypeOpsImpl[A, C, T](using rtcAuto: RuntimeConstraint.AutoDerived[A, C]):
???
For notes about AutoDerived
, see my other reply below.
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 initial thinking was so that we could use it for opaque types outside of the opaque type companion object. However, thinking about it a bit more now I don't think that makes too much sense. I changed it as per your suggestion
final class AutoDerived[A, T](val inner: RuntimeConstraint[A, T]) | ||
object AutoDerived: | ||
inline given [A, C](using rtc: RuntimeConstraint[A, A :| C]): AutoDerived[A, A :| C] = | ||
new AutoDerived[A, A :| C](rtc) | ||
|
||
inline given [A, C](using inline c: Constraint[A, C], ng: NotGiven[RuntimeConstraint[A, A :| C]]): AutoDerived[A, A :| C] = | ||
new AutoDerived[A, A :| C](RuntimeConstraint.derived[A, C]) |
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.
One of the things that was bugging me was having auto-derivation of RuntimeConstraint
from a Constraint
enabled by default. This is fairly dangerous as methods requiring a RuntimeConstraint
would auto-derive one if one wasn't in scope, which can lead to increased compilation time and generated code size.
With this, a user / method / etc can define whether they want an explicit or implicit given to be provided. For newtypes, this is generated by default
Example:
// will raise a compile-time error if one isn't explicitely defined (newtypes are OK since we create one for them in RefinedTypeOpsImpl)
def foo(value: Int)(using RuntimeConstraint[Int, Int :| Greater[10]]) = ???
// Will prioritise an explicitly defined RuntimeConstraint and auto-derive one if it doesn't exist
def foo(value: Int)(using RuntimeConstraint.AutoDerived[Int, Int :| Greater[10]]) = ???
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 still don't understand why RuntimeConstraint
does not have the same type parameters than Constraint
. Then, it would look like:
def foo(value: Int)(using RuntimeConstraint[Int, Greater[10]]) = ???
def foo(value: Int)(using RuntimeConstraint.AutoDerived[Int, Greater[10]]) = ???
Also, I'm not sure about the usefulness of AutoDerived
. It would leak an implementation detail (how the instance is generated) and I'm not sure about the real impact of auto deriving automatically on bytecode size and compilation times.
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.
After sleeping on it, I think you're right. I reverted these changes
trait RefinedTypeOpsImpl[A, C, T]: | ||
|
||
trait RefinedTypeOpsImpl[A, C, T](using rtcAuto: RuntimeConstraint.AutoDerived[A, A :| C]): | ||
given rtc: RuntimeConstraint[A, T] = rtcAuto.inner.asInstanceOf[RuntimeConstraint[A, T]] |
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 don't think we need such given instance. If someone want to get the constraint bound to a newtype, it can do it as follow:
inline def foo[T](using mirror: RefinedTypeOps.Mirror[T], constraint: Constraint[mirror.BaseType, mirror.ConstraintType] /*Or runtime constraint*/) = ???
But we can get rtc
as a public val
to allow using it in RefinedTypeOps
extensions like in Cats or ZIO
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 made it a val
but also created a protected given RuntimeConstraint[A, C]
. My reasoning behind having the protected given
is so that it can be used in the companion object of opaque types when creating codecs etc
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.
Miss clicked on "approve". I'm changing it to "request changes"
Looks good to me. Can you add some Scaladoc to |
Done. Feel free to merge in if you're happy with it! |
POC for #174