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 OpIntercept to allow custom types and operations #149

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

soronpo
Copy link
Collaborator

@soronpo soronpo commented Jun 20, 2020

See discussion on #140
For any operation/types that are not supported internally, the user can create an implicit for OpIntercept.Aux[Op, Out] to set the operation type "value".

See https://github.com/fthomas/singleton-ops/compare/master...soronpo:op_intercept?expand=1#diff-b17d5bc3066a42d6ad991f1b86cbc212 for examples.

cc @erikerlandson

@codecov
Copy link

codecov bot commented Jun 20, 2020

Codecov Report

Merging #149 into master will increase coverage by 0.21%.
The diff coverage is 90.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   91.47%   91.69%   +0.21%     
==========================================
  Files          10       10              
  Lines         692      710      +18     
  Branches       15       15              
==========================================
+ Hits          633      651      +18     
  Misses         59       59              
Impacted Files Coverage Δ
src/main/scala/singleton/ops/impl/Op.scala 100.00% <ø> (ø)
.../main/scala/singleton/ops/impl/GeneralMacros.scala 91.38% <90.62%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2150800...aa14d5e. Read the comment docs.

@soronpo
Copy link
Collaborator Author

soronpo commented Jun 20, 2020

@erikerlandson the branch is ready. Can you please check if this satisfies your usecase?

}
CalcCache.clearOpInterceptCalc()
cachedCalc match {
case Left(t : CalcUnknown) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is totally nitpicking, but IIUC, the convention for Either is that the Left is the "error" condition, isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I've rarely used it in this fashion, so forgot. I'll fix.

@erikerlandson
Copy link
Contributor

@soronpo this looks very cool - so you get the Out type by caching the result from "inner" macro calls invoked by the type checking, and then grabbing it out of the cache when it pops back up?

@soronpo
Copy link
Collaborator Author

soronpo commented Jun 21, 2020

@soronpo this looks very cool - so you get the Out type by caching the result from "inner" macro calls invoked by the type checking, and then grabbing it out of the cache when it pops back up?

Yes. Kinda dirty, but this will get the job done.

@soronpo
Copy link
Collaborator Author

soronpo commented Jun 21, 2020

Another cool thing is that recursive calls like Fibonacci are implicitly memoized due the caching mechanism of singleton-ops. But we are still very limited to due stack limitations. I should probably take the time to do tail-recursive refactoring as much as possible in the future.

@soronpo
Copy link
Collaborator Author

soronpo commented Jun 21, 2020

I pushed a change to the concept. I realized that the reason I had to use shapeless.the instead of implicitly was the reason I needed the caching in the first place. I also found a way to directly call the implicit instead of c.typecheck (c.inferImplicitValue`) and used it to get a more refined type. So there is no need for caching any more, except for passing the error message, which is done internally without the user knowing about it. See the updated examples. This is now much simpler.

@erikerlandson
Copy link
Contributor

erikerlandson commented Jun 21, 2020

interesting, I couldn't figure out how to persuade inferImplicitValue come back with an Out type I could access

Aha, I was trying to use itree and itree.tpe, not whatever itree.tpe.decls.head.info is

@erikerlandson
Copy link
Contributor

I will be very surprised if this doesn't work for Rational, but I'll try it out to make sure

@soronpo
Copy link
Collaborator Author

soronpo commented Jun 21, 2020

Thanks. Let me know. I've pushed the last update (hopefully). I added a value return. If the return value is a singleton (literal), the implicit function for OpIntercept doesn't require returning an instance. If the return value is a non-singleton, then for the value, we need an instance, of course. In any case the value field inside the returned operation is lazy, so if you don't need the value, you can leave ??? in any case.

@soronpo
Copy link
Collaborator Author

soronpo commented Jun 21, 2020

I will be very surprised if this doesn't work for Rational, but I'll try it out to make sure

It should be much more simple to implement. Make sure to remove the dedicated Add, Sub, etc. operations and use the build-in ids.

@soronpo
Copy link
Collaborator Author

soronpo commented Jun 22, 2020

There is a weird fail for Scala 2.11:

[error] /home/travis/build/fthomas/singleton-ops/src/test/scala/singleton/ops/OpInterceptSpec.scala:37:29: overriding value value in trait Op of type this.Out;
[error]  value value has incompatible type
[error]     val add2 = shapeless.the[Vec[W.`1`.T, W.`2`.T] + Vec[W.`3`.T, W.`8`.T]]

@erikerlandson
Copy link
Contributor

@soronpo I played with an implementation for Rational addition, and confirmed that OpIntercept is working like we wanted:

scala> val t = shapeless.the[Rational[1,3] + 2 + Rational[1,6]]
t: singleton.ops.impl.OpMacro[singleton.ops.impl.OpId.+,singleton.ops.rational.Rational[1,3] + 2,singleton.ops.rational.Rational[1,6],singleton.ops.NP]{type OutWide = singleton.ops.rational.Rational[this.Out,this.Out]; type Out = singleton.ops.rational.Rational[this.Out,this.Out]; final val isLiteral: Boolean(false)} = $anon$1@30b7f393

scala> t.value
res13: t.Out = singleton.ops.rational$$anon$2$$anon$3@7056fb08

scala> t.value.show
res14: String = Rational(5, 2)

One caveat: Getting the value in a useful form seems to require shapeless.the. For example, if I do the same thing with implicitly I think the types are correct but the value gets "erased" to Out:

scala> val t = implicitly[Rational[1,3] + 2 + Rational[1,6]]
t: singleton.ops.rational.Rational[1,3] + 2 + singleton.ops.rational.Rational[1,6] = $anon$1@d402a9

scala> t.value
res11: t.Out = singleton.ops.rational$$anon$2$$anon$3@4744a330

scala> t.value.show
               ^
       error: value show is not a member of t.Out

similar for using Id:

scala> val t = implicitly[Id[Rational[1,3] + 2 + Rational[1,6]]]
t: singleton.ops.Id[singleton.ops.rational.Rational[1,3] + 2 + singleton.ops.rational.Rational[1,6]] = $anon$1@2fb8109b

scala> t.value
res15: t.Out = singleton.ops.rational$$anon$2$$anon$3@52d74b49

scala> t.value.show
               ^
       error: value show is not a member of t.Out

I don't think this is a deal breaker, but if there is an easy way to get the functionality of shapeless.the via Id or some singleton-ops idiom, that would be nice. Maybe via OpCast ?

@soronpo
Copy link
Collaborator Author

soronpo commented Jun 22, 2020

I don't think this is a deal breaker, but if there is an easy way to get the functionality of shapeless.the via Id or some singleton-ops idiom, that would be nice. Maybe via OpCast ?

Yeah, I was thinking to write calcOp[T]. Maybe it's possible to workaround this somehow.

@soronpo
Copy link
Collaborator Author

soronpo commented Jun 24, 2020

I'm considering changing the implementation of the internal operations and how OpIntercept works for the value trees.
Currently, operations are defined as:
type SomeOp = OpMacro[N, A1, A2, A3]
I think of switching to:
type SomeOp = OpMacro[N, Args], where Args <: Tuple
Then OpIntercept can be changed to:

trait OpIntercept[Op[N, Args]] { 
  type Out
  def apply(args : Args) : Out
}

This way we can define any operations on the typelevel on any types and arguments, and also get a term-level composed function.

@erikerlandson
Copy link
Contributor

This way we can define any operations on the typelevel on any types and arguments, and also get a term-level composed function.

in this concept, is def apply(args : Args) : Out a replacement for val value: Out ?

@soronpo
Copy link
Collaborator Author

soronpo commented Jun 24, 2020

in this concept, is def apply(args : Args) : Out a replacement for val value: Out ?

Yes. A value is optional, and where needed then it would typically depend on the actual value arguments and not summoned from thin air.

@soronpo
Copy link
Collaborator Author

soronpo commented Jul 24, 2020

I've pushed my WIP, but it will take me while to complete due to priorities. The current status is that it is impossible to combine OpInterceptAux and OpGenAux, but I was able to remove OpConv. I started a change to support generic number of arguments. I was playing with various methods to support Rational. I played with the concept of creating a new LiteralNumeric[T, UB].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants