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

Introduce TypeRep to replace Manifest #31

Open
TiarkRompf opened this issue Sep 26, 2012 · 24 comments
Open

Introduce TypeRep to replace Manifest #31

TiarkRompf opened this issue Sep 26, 2012 · 24 comments
Labels

Comments

@TiarkRompf
Copy link
Owner

Some DSLs like Scalan (https://github.com/scalan/Scalan-v2) need to attach domain specific information to types. In general it seems logical to abstract over the type representation, too, not just the terms (Rep[T]). The default would be TypeRep[T] = TypeTag[T].

@julienrf
Copy link
Contributor

Where would the TypeRep[T] = TypeTag[T] definition live? In BaseExp?

@TiarkRompf
Copy link
Owner Author

We could put it directly in BaseExp, but maybe it will work better to use a different trait that is mixed in on a higher level. After all we also need to consider the case when we want to use something else than TypeTag[T], and it would be nice if we could use BaseExp as is.

@vjovanov
Copy link
Collaborator

It seems that this feature can not be implemented without macros. Here is the main blocker:

def list_new[A:TypeRep](xs: Seq[Rep[A]]): Rep[List[A]] = ListNew(xs)

results in "could not find implicit value for evidence parameter of type scala.lms.TypeRep[List[A]] ".

Apparently the compiler here finds the manifest of a type List[A] if it has a manifest for T in scope. We can not reproduce this without compiler support (e.g. macros). I think it is not worth the effort.

Simple TypeTag version is ready for a pull request.

@julienrf
Copy link
Contributor

Maybe you need to help the compiler? Sometimes the type inference mechanism is not good when we use type aliases, what if you add the following?

trait TypeRepBase {
  type TypeRep[A]
}

trait TypeRepTypeTag extends TypeRepBase {
  type TypeRep[A] = TypeTag[A]
  implicit def typeTag2TypeRep[A : TypeTag]: TypeRep[A] = implicitly[TypeTag[A]]
}

@vjovanov
Copy link
Collaborator

Then you get a StackOverflow since typeTag2TypeRep has the highest priority.

@sstucki
Copy link
Collaborator

sstucki commented Apr 26, 2013

I'm confused. Can't we use this

type TypeRep[A] = TypeTag[A]
implicit def typeTag2TypeRep[A](implicit ev: TypeTag[A]): TypeRep[A] = ev

@vjovanov
Copy link
Collaborator

This is a diverging implicit.

@vjovanov
Copy link
Collaborator

And if you use the wrapper class then you get the error that I presented in the first comment.

@sstucki
Copy link
Collaborator

sstucki commented Apr 26, 2013

What does the wrapper class look like? implicit class TypeRep[+A](tt: TypeTag[A])?

@vjovanov
Copy link
Collaborator

It is not covariant, as the TypeTag is not covariant.
And I use a method:

implicit def typeRepFromManifest[T](implicit mf: Manifest[T]): TypeRep[T] = TypeRepExp(mf)

Instead of the implicit class.

Now I am looking how does the compiler get an implicit Manifest[List[T]] when it has an implicit Manifest[T].

@sstucki
Copy link
Collaborator

sstucki commented Apr 26, 2013

What about changing the definition of list_new to

def list_new[A](xs: Seq[Rep[A]])(implicit ev: TypeRep[List[A]]): Rep[List[A]] = ListNew(xs)

so the TypeRep[List[A]] gets passed along? And you'd need to change the ListNew constructor too, so maybe it's a bit cumbersome.

@vjovanov
Copy link
Collaborator

As well as 111 other places in LMS :)

@sstucki
Copy link
Collaborator

sstucki commented Apr 26, 2013

Well yea, but short of changing the compiler, this is the only way, right? At least if you want to allow TypeRep[A] to be an actual class (so you can add additional info to it) rather than an alias for TypeTag[A]. How would you solve the problem using Macros?

@TiarkRompf
Copy link
Owner Author

how about defining

implicit def listTypeRep[A:TypeRep]: TypeRep[List[A]] 

in ListOps?

@vjovanov
Copy link
Collaborator

In case of type TypeRep[T] Macros can override the priorities of
implicits so they could find the appropriate implicit in the scope.

In case of wrapper class, they could introduce the wrapped TypeTag as an
implict, spawn an appropriate TypeTag for X[T], and finally wrap it
again into the TypeTag.

@vjovanov
Copy link
Collaborator

@TiarkRompf This would require adding these implicits in many places for different types. We could make one for all higher kinds but how would the implementation of this method go?

@vjovanov
Copy link
Collaborator

Do we have a use case for this feature that would justify overheads in complexity that applies to compilation times, designers, and users?

@TiarkRompf
Copy link
Owner Author

Thinking about it, it seems to me that having DSL designers state explicitly "this is a type that can be lifted" may be a good thing. The main benefit is enabling polytypic programming: the TypeRep abstraction is a safe mechanism to define that e.g. a List[(A,B)] is globally represented as a (FastList[A], FastList[B]). It will also enable transformers that change types in such a way. Compared to the current Manifest situation, i hope that we can reduce complexity for users somewhat. For example, we can have better error messages when no implicit TypeRep is found (using implicitNotFound).

@vjovanov
Copy link
Collaborator

I have investigated what happens. In the malicious case, mentioned in the first comment, the compiler produces trees that do run-time reflection to create a new TypeTag[List[T]] based on the manifest of T. We could reproduce this, but then we are replicating parts of the compiler. Our modification would also need to copy all the additional data from the TypeTag[T] to the TypeTag[List[T]].

This logic should also include all of the possible cases: TypeTag[(T, U)], TypeTag[List[List[T]]], TypeTag[List[(T, U]] etc.

@vjovanov
Copy link
Collaborator

vjovanov commented May 1, 2013

vjovanov/virtualization-lms-core@TiarkRompf:develop-0.4...wip/typerep

Tests do not compile as I would need to resolve 60 more type errors there. Also there are some differences in behavior. For example, vjovanov@93dbee0.

If everyone is happy with this we need to fix the remaining tests and merge immediately. This pull request required a noticeable amount of work and it would be a shame to be wasted.

@TiarkRompf
Copy link
Owner Author

Awesome, this looks good to me. I think we should go ahead and merge it into develop-0.4 and then fix the tests. I can take care of the tests if you want.

@TiarkRompf
Copy link
Owner Author

Some minor comments:

  • should we rename TypeRepExp to TypeExp in analogy with Rep/Exp, and move it from the Base world to BaseExp?
  • I think it would also make sense to restrict the interface of trait TypeRep somewhat. DSL implementors can always pattern match to obtain a TypeExp, and for users the structure of a TypeRep shouldn't matter.
  • in Effects.scala, there are checks typeRep[T] == manifest[Any]: should the rhs be typeRep[Any]?

@vjovanov
Copy link
Collaborator

vjovanov commented May 1, 2013

Agreed. I thought that I found all manifest[.?] but it seems some are missed. I will fix those.

@vjovanov
Copy link
Collaborator

vjovanov commented May 1, 2013

Something strange happened. Github says that I am up to date (vjovanov/virtualization-lms-core@TiarkRompf:develop-0.4...wip/typerep). But when I rebase there are conflicts. I am fixing those.

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

No branches or pull requests

4 participants