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

Direct representation of higher-kinded types #1343

Merged
merged 92 commits into from
Jul 15, 2016

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jun 29, 2016

Rebased version of #1337 with commits squashed so that there are less than 100 commits in the PR.

This is (hopefully the last) attempt to model higher-kinded types in dotty. This time it's done directly, having specific types for type lambdas and higher-kinded applications.

This PR is based on #1282 but undoes most of it. Nevertheless I thought it would be good to have a trace of the history how we arrived here.

@odersky
Copy link
Contributor Author

odersky commented Jun 30, 2016

There are two areas that should be investigated further:

  • [] should we bring back named type parameters?
  • [] can we represent existential types as hk types applied to type bounds?

But the commit as is is ready for review.

@@ -1641,6 +1641,7 @@ object SymDenotations {
*/
def isCachable(tp: Type): Boolean = tp match {
case _: TypeErasure.ErasedValueType => false
case tp: TypeRef if tp.symbol.isClass => true
Copy link
Member

Choose a reason for hiding this comment

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

down to 22secs from 60secs with this change.
Now, need to do something with compareAliasedRefined to get back to 2.5secs.

@odersky
Copy link
Contributor Author

odersky commented Jul 6, 2016

There are some loose ends with support for "existential types", which can be encoded using a combination of hk applications and wildcards. E.g.

T { forSome type A <: U }

can be encoded as

 ([A] -> T)[_ <: U]

Subtyping and some other basic functionality works over these types, but other things crash the
compiler or lead to unintuitive results. That's no surprise as we do not really know what the right
rules for these types are. Bring existential types back? Would that not get us back into the same
intractable problems we have with scalac? On the other hand, if we want to forbid such types, the
question is how to do this in a clean way that's not too restrictive. The cleanest way would be
to not allow wildcards in hk type applications, period. But that would invalidate some patterns the
collections library, where we write definitions such as

type Coll = CC[_]

with CC a higher-kinded abstract type or type param.

@DarkDimius
Copy link
Member

Was playing with how much does removing compareAliasedRefined affect linker.
Replacing it by false reduces analysis time from 12 seconds down to 2.5. Yheartbeat is also able to discover this: number of member selections goes from 33'045'060 down to 955'174

@odersky
Copy link
Contributor Author

odersky commented Jul 7, 2016

The PR is completed now as far as I am concerned. I mean to come back to named parameters, which would also affect compareAliasedDefined. But I think this is better done in a separate PR. For the moment the status of named type parameters is "in limbo".

* or -1 if no such parameter exists.
/** Is entry associated with `pt` removable? This is the case if
* all type parameters of the entry are associated with type variables
* which have its `inst` fields set.
Copy link
Member

Choose a reason for hiding this comment

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

their inst fields

Use (cached) superType where possible.
Beta-reduce only if `Config.simplifyApplications` is true.
Turning off beta-reduction revealed two problems which are
also fixed in this commit:

1. Bad treatement of higher-kinded argyments in cyclicity checking
2. Wrong variance for higher-kinded arguments in TypeAccumulator
@odersky
Copy link
Contributor Author

odersky commented Jul 13, 2016

As far as I can see we should now have feature parity with Scala 2.x with SI-2712.

else if (tparam.paramVariance == 0) variance = 0
val pvariance = tparam.paramVariance
if (pvariance < 0) variance = -variance
else if (pvariance == 0) variance = 0
Copy link
Member

Choose a reason for hiding this comment

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

variance *= tparam.paramVariance instead of the if/else ? (Same in the case below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great minds think alike :-) See next commit.

@odersky
Copy link
Contributor Author

odersky commented Jul 14, 2016

We decided to wait with several other PRs until this is in, so I want to merge by the end of the day, unless something important comes up.

lazy val typeParams: List[LambdaParam] =
paramNames.indices.toList.map(new LambdaParam(this, _))

def derivedTypeLambda(paramNames: List[TypeName] = paramNames, paramBounds: List[TypeBounds] = paramBounds, resType: Type)(implicit ctx: Context): Type =
Copy link
Member

@smarter smarter Jul 14, 2016

Choose a reason for hiding this comment

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

Every other derivedFoo method returns a Foo, but this one may return a TypeAlias or a TypeBounds instead of a TypeLambda, why is that? Could a different name be chosen to avoid confusion?

@smarter
Copy link
Member

smarter commented Jul 14, 2016

More bound checks are needed, this shouldn't compile:

class Foo[A]
class Bar[B]

object Test {
  type Alias[F[X] <: Foo[X]] = F[Int]
  val x: Alias[Bar] = new Bar[Int] // Bar[X] is not a subtype of Foo[X]
}

@@ -850,6 +879,12 @@ object Types {
case _ => this
}

/** If this is a TypeAlias type, its alias, otherwise this type itself */
final def followTypeAlias(implicit ctx: Context): Type = this match {
Copy link
Member

Choose a reason for hiding this comment

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

This method is never called

@smarter
Copy link
Member

smarter commented Jul 14, 2016

Also tests/pos-scala2/t2994.scala fails to compile in scalac but compiles in dotty:

tests/pos-scala2/t2994.scala:23: error: type s takes type parameters
    trait curry[n[_, _], s[_]] { type f[z <: NAT] = n[s, z] }
                                                      ^

I think we should follow scalac here and enforce kind-correctness, for reference, the change to curry happened in 7f72143 in dotty

Previous logic could only handle classes as constructors.
Also, address other reviewers comments.
@smarter
Copy link
Member

smarter commented Jul 15, 2016

Better, but I can still break it :)

class Foo[A]
class Bar[B]

object Test {
  type Alias[F[X] <: Foo[X]] = F[Int]

  def foo[M[_[_]], A[_]]: M[A] = null.asInstanceOf[M[A]]

  val x = foo[Alias, Bar]
}

scalac checks the bounds in the type of x and sees the issue:

error: type arguments [Bar] do not conform to type Alias's type parameter bounds [F[X] <: Foo[X]]
  val x = foo[Alias, Bar]
      ^

@smarter
Copy link
Member

smarter commented Jul 15, 2016

Interestingly, scalac will happily compile this code if I replace val x = foo[Alias, Bar] by just foo[Alias, Bar], not sure if this can be exploited to crash at runtime but it's still a bug.

Enable checking of bounds when comparing type lambdas. This invalidates
a pattern used in t2994 and potentially other code, where a bound [X] -> Any
is used as a template that is a legal supertype of all other bounds. The old
behavior is still available under language:Scala2.
@odersky
Copy link
Contributor Author

odersky commented Jul 15, 2016

@smarter Bounds check is tigthened now "at the source", in a different way from how it's done in Scala-2. The alternative would have been to open a loophole by not comparing bounds of type lambdas and then fixing it by checking all * types for bound correctness. But as you noted that would have to include all types computed by typer, not just types written by the programmer. I am scared this would be too expensive and that error messages would suffer.

scalac seems to check inferred types of declared values and methods, but not other types. I agree this is not enough.

@smarter
Copy link
Member

smarter commented Jul 15, 2016

Yes, I was skeptical about that loophole too, would be interesting to figure out how much code relies on it. A proper migration warning seems tricky indeed, but maybe we could have a flag in the constraint set var unsoundLambdaSubtyping = false that we set to true if the check ever fails, if a the end of a compilation run the flag is set, then issue a warning

@odersky
Copy link
Contributor Author

odersky commented Jul 15, 2016

Going to merge now to let other PRs proceed. Many thanks to @smarter for the thorough review!

@odersky odersky merged commit 409c6c3 into scala:master Jul 15, 2016
@odersky odersky deleted the change-hk-direct2 branch July 15, 2016 11:12
@DarkDimius
Copy link
Member

Hooray! 🌟 🎆 🎆 🎆 ⭐

@odersky
Copy link
Contributor Author

odersky commented Jul 15, 2016

On Fri, Jul 15, 2016 at 11:03 AM, Guillaume Martres <
notifications@github.com> wrote:

Yes, I was skeptical about that loophole too, would be interesting to
figure out how much code relies on it. A proper migration warning seems
tricky indeed, but maybe we could have a flag in the constraint set var
unsoundLambdaSubtyping = false that we set to true if the check ever
fails, if a the end of a compilation run the flag is set, then issue a
warning

That sounds like a good strategy. I am reading this only now after having
merged the PR. So we would need a separate PR for this.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#1343 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAwlVtRpAaFiEaNlPl2ws4tcoNVlfpAJks5qV0zxgaJpZM4JBcu4
.

Prof. Martin Odersky
LAMP/IC, EPFL

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.

None yet

5 participants