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

SIP-23 Implementation of literal types #5310

Merged
merged 25 commits into from
Jan 24, 2018

Conversation

milessabin
Copy link
Contributor

@milessabin milessabin commented Jul 29, 2016

Implements literal types. Enabled by -Yliteral-types or -Xexperimental.

This is a continuation of work by @folone and @adriaanm. The work on @adriaanm's branch was the starting point and all the TODO items on the corresponding PR have been completed.

  • Literals can now appear in type position, designating the corresponding singleton type.
  • Widening/narrowing behaviour is consistent with 2.11.x and between 2.12.x with the flag enabled and disabled. The use of final on a val definition to explicitly request an unwidened type to be inferred is preserved. Type parameter inference remains the same unless the inference of a singleton type is explicitly requested via a <: Singleton bound.
  • A scala.ValueOf[T] type class and corresponding scala.Predef.valueOf[T] operator has been added yielding the unique value of types with a single inhabitant.
  • isInstanceOf/asInstanceOf tests/conversions are implemented via equality/identity tests for singleton types.
  • Support for scala.Symbol literal types has been added.
  • The spec markdown files have been updated.
  • Many tests for all of the above have been added.
  • All (par)tests pass.
  • With -Yliteral-types enabled on all tests there are 15 test result changes, all of which are expected. See below for details.
  • Types reported in error messages are widened where appropriate, matching behaviour with the flag disabled (eg. we report 'Found String expected Int' instead of 'Found "foo" expected Int').
  • The public macro API is unchanged relative to 2.11.x. In contrast to @adriaanm's branch there are now non-API LiteralConstantType and FoldableConstantType subtypes of the API type ConstantType.
  • shapeless is likely to be the library most directly affected by the above changes and, with only minor 2.11.x source-compatible changes, it compiles and passes all tests with -Yliteral-types both enabled and disabled.

Related bugs fixed in this PR

  • SI-1273 Singleton type has wrong bounds
  • SI-5103 Singleton types not inferred in all places they should be
  • SI-8323 Duplicate method name & signature with singleton type parameters over constant types
  • SI-8564 Methods with ConstantType results get the inhabitant of ConstantType as their body

Test result changes with -Yliteral-types enabled for all tests

New warnings due to Symbol literals becoming pure:

  • test/files/instrumented/indy-symbol-literal.scala

Error messages changed due to more precise types being reported:

  • test/files/neg/t8323.scala
  • test/files/neg/names-defaults-neg.scala

Pos tests which correctly become neg with -Yliteral-types

  • test/files/pos/unchecked-a.scala

Neg tests which correctly become pos with -Yliteral-types:

  • test/files/neg/any-vs-anyref.scala
  • test/files/neg/not-possible-cause.scala
  • test/files/neg/t6263.scala
  • test/files/neg/t900.scala

Error messages changed correctly:

  • test/files/neg/t8463.scala
  • test/files/run/repl-colon-type.scala

Compiler/REPL output changed due to more precise types being reported:

  • test/files/neg/logImplicits.scala
  • test/files/run/analyzerPlugins.scala
  • test/files/run/delambdafy_uncurry_byname_inline.scala

Compiler/REPL output changed due to Symbols becoming first-class literals:

  • test/files/run/t6549.scala

Run tests which correctly fail due to improved inference

  • test/files/run/StubErrorHK.scala
  • test/files/run/repl-kind.scala

Unavoidable failure due to the enablement of the feature:

  • test/files/run/settings-parse.scala

// valueOf -----------------------------------------------------------

/** Retrieve the single value of a type with a unique inhabitant. */
@inline def valueOf[T](implicit vt: ValueOf[T]): T {} = vt.value
Copy link
Member

Choose a reason for hiding this comment

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

The signature should be def valueOf[T]: T {}. Avoid exposing the implicit parameter. (Have a look at classOf for how it is done.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment on ValueOf below. It's the type class which is important, this def very much less so.

Copy link
Member

Choose a reason for hiding this comment

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

The style of the signature is not related to its importance. It should not be defined this way, regardless of importance.

Copy link
Member

Choose a reason for hiding this comment

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

Have a look at resolveClassTag to see how it's possible to resolve an implicit for your call without having to specify that your method takes one (and therefore allowing people to accidentally or maliciously pass one explicitly).

Choose a reason for hiding this comment

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

@soc What's wrong with this signature? stdlib have a lot of methods with implicit parameters. Isn't it the same as def classTag[T](implicit ctag: ClassTag[T]) = ctag?

Copy link
Member

@soc soc Aug 18, 2016

Choose a reason for hiding this comment

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

In my book magicless also wins, except in the most important circumstances.
A method that will be available to every single developer by default, on every IDE auto-complete, is one of those important circumstances. It needs to consistent. It's the standard library, not some random third-party library people can opt out of.

After years of doing the deprecation/removal dance, valueOf with ValueTag is one of the things that would get fixed or removed in short order.

Copy link
Member

Choose a reason for hiding this comment

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

I think perhaps it would be appropriate to add a comment to classOf, explaining (or trying to explain) why it don't have the obvious implementation.

Copy link
Member

Choose a reason for hiding this comment

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

I can have a look at my new classOf[T] == classTag[T].runtimeClass implementation, I'm pretty sure I added some comment there.

Copy link
Contributor

Choose a reason for hiding this comment

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

T {} is a little confusion because it does not respect .widen:

@ val t = typeOf[0 {}] 
t: Type = RefinedType(List(FoldableConstantType(Constant(0))), SynchronizedOps())
@ val w = t.widen 
w: Type = RefinedType(List(FoldableConstantType(Constant(0))), SynchronizedOps())
@ println(t eq w) 
true

Copy link
Contributor

Choose a reason for hiding this comment

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

How about encode it as Id[0], which is less magic.

@ type Id[+A] = A 
defined type Id
@ val dontWidenMe: Id[0] = 0 
dontWidenMe: Id[0] = 0
@ Some(dontWidenMe) 
res72: Some[Id[0]] = Some(0)

@xeno-by
Copy link
Member

xeno-by commented Jul 29, 2016

Why is the macro API left unchanged? What if metaprogrammers want to explicitly control the flavor of constant types?

Also, could you document the differences between LiteralConstantType and FoldableConstantType? At the moment, these are classes with no comments or documentation.

@milessabin
Copy link
Contributor Author

@xeno-by four reasons,

  • an API change can't be enabled via a feature flag
  • source compatibility with 2.11.x
  • the decision about whether to fold or not is one for the compiler, not a macro author
  • given that the 2.11/12 macro API is deprecated in favour of Scala.meta it seems inadvisable to change it unless absolutely necessary.

* Instances of `ValueOf[T]` are provided implicitly for all eligible types. Typically
* an instance would be required where a runtime value corresponding to a type level
* computation is needed. For example, we might define a type of list which encodes its
* length in its type, in which case the lenght of the list can be computed as a
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: lenght

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted.

@milessabin
Copy link
Contributor Author

I think those failures are spurious ...

@milessabin
Copy link
Contributor Author

/rebuild

@propensive
Copy link
Contributor

Just found a couple of minor things from early experiments:

  • foo.isInstanceOf[Singleton] will always return true. Additionally, you get a spurious "fruitless type test" warning if you check foo.isInstanceOf[String with Singleton].
  • interestingly, in the REPL, val x: "foo" = "foo"'s return type is printed as "foo", whereas final val x = "foo"'s is printed as String("foo"). Is there a deliberate distinction?

@milessabin Would you like me to continue reporting issues on this PR, or is there somewhere better?

else if (in.token == INTERPOLATIONID)
if (in.token == SYMBOLLIT) {
if(settings.YliteralTypes)
finish(Symbol(in.strVal.intern()))
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to represent these with a distinct type from Symbol to avoid a memory leak in a resident compiler (Symbol-s are stored in a hash map).

Copy link
Member

Choose a reason for hiding this comment

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

I forgot that it is actually a weak map, so we can keep things as they are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@retronym
Copy link
Member

Could you add @adriaanm and @folone as co-authors in the commit comment? I think the convention in Git is:

 Co-authored-by: Some One <some.one@example.foo>

@retronym retronym added this to the 2.12.1 milestone Jul 29, 2016
@milessabin
Copy link
Contributor Author

@propensive yes, please do report issues here.

@milessabin
Copy link
Contributor Author

@retronym re. Co-authored-by ... yes, of course.

spec/03-types.md Outdated
value which [conforms](06-expressions.html#expression-typing) to
`scala.AnyRef`, the type denotes the set of values consisting of `null` and the
value denoted by $p$ (i.e., the value $v$ for which `v eq p`). Where the path
does not conform to `scala.AnyRef` the type denotes the set consiting of only
Copy link
Contributor

Choose a reason for hiding this comment

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

s/consiting/consisting/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted.

@milessabin milessabin force-pushed the topic/sip-23-redux branch 2 times, most recently from 07341a0 to bb0ac9b Compare August 10, 2016 13:01
@jvican
Copy link
Member

jvican commented Aug 10, 2016

SIP-23 has been reviewed in today's SIP meeting. The assigned reviewer is @adriaanm, which will provide actionable feedback shortly.

@milessabin
Copy link
Contributor Author

milessabin commented Aug 10, 2016

@propensive apropos your observation about the different rendering of types in the REPL: the distinction is between inferred constant types (String("foo")) vs. explicit literal types ("foo"). I kept the representation of inferred constant types the same for backwards compatibility: very many neg tests would have different output if the representation changed. OTOH, where types have been provided explicitly, having a representation which is also syntactically valid under the new rules seems desirable.

@alexeyr
Copy link
Contributor

alexeyr commented Aug 11, 2016

Type parameter inference remains the same unless the inference of a singleton type is explicitly requested via a <: Singleton bound.

Does this apply to non-literal singleton types as well? It would be quite useful in some cases I've had. This change should also be documented in 6.26.4.

@dwijnand
Copy link
Member

JFYI this is green now @milessabin.

@milessabin
Copy link
Contributor Author

Community build here: https://scala-ci.typesafe.com/view/scala-2.13.x/job/scala-2.13.x-integrate-community-build/889/.

Although it looks like the community build hasn't be green for a while ... or am I misreading the Jenkins dashboard?

@SethTisue
Copy link
Member

yeah the 2.13 community build is kind of a shambles right now. a bunch of projects are green, but many aren't. so it can give some assurance, but far less assurance, currently, than the 2.12 community build gives.

I haven't prioritized it because as soon as the new collections land in 2.13, I expect everything to break anyway. the laborious process of getting everything green again will begin in earnest at that point.

@lrytz
Copy link
Member

lrytz commented Jan 19, 2018

compared to the previous run, there seem to be 3 additional failures. Theoretically they could be due to changes in the respective repos. To investigate.

scala-continuations: FAILED (RuntimeException: )

[scala-continuations] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.9/project-builds/scala-continuations-c9ed3b11fe80d175b140dc7c083132fd534a1de8/library/src/test/scala/scala/tools/selectivecps/TestSuite.scala:215: cannot compute type for CPS-transformed function result
[scala-continuations] [error]     test { shift { k: (Int => String) => 9 }; { test3(0); 2 } }
[scala-continuations] [error]                  ^

scala-parallel-collections: FAILED (RuntimeException: Tests unsuccessful)

[scala-parallel-collections] [error] Test scala.SerializationStabilityTest.testAll failed: java.io.InvalidClassException: scala.collection.immutable.HashMap$SerializationProxy; local class incompatible: stream classdesc serialVersionUID = 2, local class serialVersionUID = 3693899892909726219, took 0.023 sec

https://github.com/scala/scala-parallel-collections/blob/master/junit/src/test/scala/scala/SerializationStabilityTest.scala#L91

slick: FAILED (RuntimeException: )

[slick] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.9/project-builds/slick-95bd2107fe1d80c30d5fba7d421dc31395b01f96/slick/src/main/scala/slick/relational/RelationalProfile.scala:82: pattern type is incompatible with expected type;
[slick] [error]  found   : slick.relational.ProductResultConverter[_(in value $anonfun),(some other)_(in value $anonfun)]
[slick] [error]  required: slick.relational.ResultConverter[slick.relational.ResultConverterDomain,Any]
[slick] [error] Note: _ <: slick.relational.ResultConverterDomain, but trait ResultConverter is invariant in type M.
[slick] [error] You may wish to define M as +M instead. (SLS 4.5)
[slick] [error] Note: _ <: Any, but trait ResultConverter is invariant in type T.
[slick] [error] You may wish to define T as +T instead. (SLS 4.5)
[slick] [error]       case tm @ TypeMappingResultConverter(_: ProductResultConverter[_, _], _, _) =>
[slick] [error]                                               ^

@milessabin
Copy link
Contributor Author

The scala-continuations one is probably due to the introduction of the new subtypes of ConstantType ... should be an easy fix. The parallel collections problem looks unrelated. I'm guessing the Slick failure is a type inference change, probably also fairly easily fixed.

@soronpo
Copy link

soronpo commented Jan 19, 2018

I don't know how difficult is the backport to 2.12, but if you do it for TLS anyways, then you can use it as dummy PR to 2.12, just to test the community build.

@Atry
Copy link
Contributor

Atry commented Jan 22, 2018

There is a comment in https://github.com/milessabin/scala/blob/352a0c8c4eccbca1c0c98de784c2ed87cc5b7fe9/src/library/scala/Predef.scala#L203

@inline def implicitly[T](implicit e: T) = e // for summoning implicit values from the nether world -- TODO: when dependent method types are on by default, give this result type e.type, so that inliner has better chance of knowing which method to inline in calls like implicitly[MatchingStrategy[Option]].zero

I guess it's the time now to make implicitly have shapeless.the semantics.

@adriaanm
Copy link
Contributor

adriaanm commented Jan 22, 2018 via email

@Atry
Copy link
Contributor

Atry commented Jan 22, 2018

@adriaanm I meant 2.13 milessabin#6

@soronpo
Copy link

soronpo commented Jan 22, 2018

We’re not accepting new features in 2.12 anymore, even under a flag.

Was this in reply to me @adriaanm ? I was just suggesting how to test for community-build regressions, since the 2.13 build is currently failing.

@lrytz
Copy link
Member

lrytz commented Jan 22, 2018

You could also use the freeze script pick the last green run of the 2.13.x community build and then run it locally / scala/community-build#58 / scala/community-build#408 / scala/community-build#667.

@milessabin
Copy link
Contributor Author

@lrytz I'm not quite sure what you're proposing ... do you mean rebase this PR onto the commit corresponding to the last green community build and then run against that?

TBQH, I think it would probably be quicker and easier to checkout and try building the three newly failing projects.

@lrytz
Copy link
Member

lrytz commented Jan 22, 2018

Ah, my assumption is that the community build is not broken due to 2.13.x being broken, but because the community projects moved ahead with incompatible changes - that's the more common case anyway. So if you take the last green run and freeze all community projects at the SHAs they had at this point, the build will probably also pass with current 2.13.x (or the baseline of this PR).

@lrytz
Copy link
Member

lrytz commented Jan 22, 2018

Instead of running locally, you can push the frozen SHAs to a branch of your scala/community-builds fork, and then specify that when starting the community build (https://scala-ci.typesafe.com/view/scala-2.13.x/job/scala-2.13.x-integrate-community-build/build?delay=0sec)

@SethTisue
Copy link
Member

SethTisue commented Jan 23, 2018

Ah, my assumption is that the community build is not broken due to 2.13.x being broken, but because the community projects moved ahead with incompatible changes

My understanding is that the main reason the 2.13 community build is currently broken is that we had inadvertently released 2.13.0-M1 and 2.13.0-M2 with -Xsource:2.12 enabled by default. But we've made incompatible language changes since 2.12, so when that switch was flipped (by #6092), a number of projects broke. iirc it had especially (but perhaps not only) to do with eta expansion.

There are two paths forward for fixing it:

  • attempt the fixes ourselves
  • go ahead and release M3 anyway and let the project maintainers change their code themselves

we sometimes go the fix-it-ourselves route, but in this case several of the broken projects are pretty big/complicated (e.g. scalaz and shapeless), so I think it might be better to just cross our fingers and ship M3 without strong assurance from the community build.

Copy link

@Jasmeet107 Jasmeet107 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@adriaanm
Copy link
Contributor

Let's deal with the community build in the next weeks. As Seth said, this isn't the first PR to affect the community build in 2.13.x.

@adriaanm adriaanm merged commit 8ad55a4 into scala:2.13.x Jan 24, 2018
@adriaanm
Copy link
Contributor

🎉

@soronpo
Copy link

soronpo commented Jan 24, 2018

Wow! Congratulation @milessabin for threading this needle!

@johnynek
Copy link
Contributor

Woo! Thanks @milessabin and all reviewers and contributors to this effort.

@milessabin
Copy link
Contributor Author

Thanks for helping me getting this across the line @adriaanm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet