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 SIP 23 ValueOf #5647

Merged
merged 1 commit into from
Jan 6, 2019
Merged

Add SIP 23 ValueOf #5647

merged 1 commit into from
Jan 6, 2019

Conversation

milessabin
Copy link
Contributor

@milessabin milessabin commented Dec 19, 2018

Added an implementation of SIP 23 ValueOf and copied over the corresponding Scala 2 tests.

Fixes #2616.

Added an implementation of SIP 23 ValueOf and copied over the
corresponding Scala 2 tests.
case ConstantType(c: Constant) =>
success(Literal(c))
case TypeRef(_, sym) if sym == defn.UnitClass =>
success(Literal(Constant(())))
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't Unit handled by the ContantType case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps it should be, but currently Unit isn't classified as a singleton type in either Scala 2 or Dotty.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should certainly be, at least conceptually. Isn't Unit the singleton type par excellence?

@odersky
Copy link
Contributor

odersky commented Dec 19, 2018

There is already a constValue function in typelevel (the package that's going to be renamed to compiletime). It's used by the S constructor for "successor" both in pattern and in value position. I believe we should not have both a typeclass and this method, that would be unnecessary duplication. Here are the relevant stubs:

package object typelevel {
  inline def constValueOpt[T]: Option[T] = ???
  inline def constValue[T]: T = ???
  type S[X <: Int] <: Int
}

The main difference between constValueOpt and valueOf seems to be that constValueOpt does not force its argument type to be fully defined, but will instead return with None if that fails.

I don't have a strong opinion whether we should have a typeclass or a macro, but we should certainly not have both. So if we go for the typeclass, we should replace all occurrences of constValue with it. That would be a first test whether it's expressive enough. Unfortunately we do not have many tests with constValue or S, but there are some.

@odersky
Copy link
Contributor

odersky commented Dec 19, 2018

Specifically, the question would be: Can we formulate S in both patterns and expressions using ValueOf? If not, then our choices are:

  • have both S and ValueOf (unappealing if S needs constValue internally)
  • drop S and accept the loss of expressiveness
  • drop ValueOf typeclass, possibly rename constValue method to valueOf method.

One question for my understanding: Why does ValueOf have to be a typeclass? By comparison, ClassTag is a typeclass since we typically pass ClassTags implicitly, and the synthesized ClassTag is a "last resort" if don't have another one in context. Is there a similar argument for passing ValueOf implicitly instead for just computing it with a direct call?

@milessabin
Copy link
Contributor Author

ValueOf is the SIP 23 replacement for shapeless's Witness. Witness is widely used in libraries and user code, so ValueOf is important from a migration point of view. ValueOf is part of 2.13 and it's also part of Typelevel Scala ... there are several projects built with Typelevel Scala which use it.

Witness is implemented in shapeless via a set of macros, and part of the motivation for including ValueOf if SIP 23 was to allow those macros to be removed in versions of shapeless spanning Scala 2 and 3.

Typically Witness is used to propagate values implicitly through chains of polymorphic method calls in ways which are similar to ClassTag. I don't think constValue could be a substitute for that.

I'd be more than happy to see constValue and ValueOf aligned more closely, but I think it's very much needed in more or less the form that it appears in this PR.

@odersky
Copy link
Contributor

odersky commented Dec 19, 2018

OK, I see the case for ValueOf as a typeclass now. But there might be other ways to avoid the duplication.

  • The fallback instance for ValueOf could be defined in the library, based on constValue.
  • Or maybe we can define S and constValue in terms of ValueOf?

@smarter
Copy link
Member

smarter commented Dec 19, 2018

It's worth noting for this discussion that once we switch to using the 2.13 library we'll have to use their version of the ValueOf class, or at least a binary-compatible one.

@milessabin
Copy link
Contributor Author

I see now that the valueOf method does overlap with constValue. I think we might be able to combine them.

Could we have an intrinsified implementation of valueOf which uses the constValue strategy if possible, and if not falls back to summoning a contextual ValueOf instance?

If that produces the same bytecode in all the cases that constValue currently handles would you be happy to replace it with valueOf?

@odersky
Copy link
Contributor

odersky commented Dec 20, 2018

I see now that the valueOf method does overlap with constValue. I think we might be able to combine them.

That would be good, yes. I think valueOf as a name is fine for this.

Could we have an intrinsified implementation of valueOf which uses the constValue strategy if possible, and if not falls back to summoning a contextual ValueOf instance?

Should it not be the other way around? If there is a contextual ValueOf instance, pick that. If not there is a fallback instance that can be based on valueOf

If that produces the same bytecode in all the cases that constValue currently handles would you be happy to replace it with valueOf?

Sure. The biggest use case we have right now is Tuple.scala. It would be interesting to see whether we can use ValueOf there. On the other hand, since things are pretty much in flux now, we can also accept some duplication, as long as we plan to come back to try to eliminate it once our toolbox has stabilized.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

@milessabin I happy to have this go in now and we sort out the duplication afterwards. Alternatively, if you want to try to do some harmonization first let me know and I'll hold back with merging.

@milessabin
Copy link
Contributor Author

@odersky let's merge this now and deal with the duplication as part of firming up the content of the typelevel/compiletime package.

@odersky odersky merged commit 3a457fa into scala:master Jan 6, 2019
@biboudis biboudis added this to the 0.12 Tech Preview milestone Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants