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

Adding Dotty cross-build #65

Merged
merged 13 commits into from
Dec 16, 2021
Merged

Adding Dotty cross-build #65

merged 13 commits into from
Dec 16, 2021

Conversation

pk044
Copy link
Contributor

@pk044 pk044 commented Feb 7, 2021

No description provided.

@pk044 pk044 marked this pull request as draft February 7, 2021 01:41
case Value(a) => F.pure(Left(right(a)))
case Effect(fa) => F.fmap(fa.value)(a0 => Left(right(a0)))
case FlatMap(prev, f) => F.pure(Left(prev.flatMap(runFold(f))))
case Fold(v0, l0: (ADT1 => Sealed[F, A0, ADT]), r0) => F.pure(Left(Fold(v0, runFold(l0), runFold(r0))))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dotty couldn't infer the type for it - for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, shall we followup with Dotty team, just in case it's a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure - I can create an issue in their repo and see if it can be resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: it seems like it's not supposed to compile at all, and this type hint shouldn't be here :)

@marcin-rzeznicki I was wondering if I could ask for your assistance - the following compilation error might be caused by the fact that Sealed is covariant, while subclasses like FlatMap or Fold are invariant.

[error] -- [E007] Type Mismatch Error: /home/pk/Development/iterators/sealed-monad/src/main/scala/pl/iterators/sealedmonad/Sealed.scala:94:62 
[error] 94 |        case Fold(v0, l0, r0) => F.pure(Left(Fold(v0, runFold(l0), runFold(r0))))
[error]    |                                                              ^^
[error]    |Found:    (l0 : ADT$6 => pl.iterators.sealedmonad.Sealed[F, A$8, ADT$6])
[error]    |Required: ADT => pl.iterators.sealedmonad.Sealed[F, A0, ADT]
[error]    |
[error]    |where:    A$8   is a type in method step with bounds <: A0
[error]    |          ADT$6 is a type in method step with bounds <: ADT

This worked fine in Scala 2 because the equality constraints were inferred incorrectly by the compiler - in Scala 3 it'll fail because we are getting a subtype constraint here, while it should be an equality constraint.

Do you think it could be handled differently than removing covariance? If I'm not mistaken, that would require plenty of changes in user code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC we needed covariance for two things:

  • to guide inference in cases like def flatMap[B, ADT1 >: ADT](f: A => Sealed[F, B, ADT1]): Sealed[F, B, ADT1] = Sealed.FlatMap(this, f) so that it'd always coalesce to the common superclass,
  • and for things like final case class Effect[F[_], A](fa: Eval[F[A]]) extends Sealed[F, A, Nothing] to be subclasses

I think the former can be eliminated easily be moving all that cruft to functions e.g. similar to how EitherT does it. But the latter is worse. Maybe the trick with XPartiallyApplied would work?

But I have to admit I don't understand why it is wrong now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for the input! I'll tinker with it a bit and see what solution will work the best.

PS. There was a time when Sealed was invariant wrt to ADT - perhaps you could go back to the commit that changed it and, hehe, partially revert some bits

yeah, I saw it and was thinking of doing it - I have a feeling it'll be neccessary :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I could not help more. I think we baked this in very deeply - at that time it seemed like a great idea. BTW, why is it wrong now?

yeah, I saw it and was thinking of doing it - I have a feeling it'll be neccessary :(

Yeah, seems like 😞

Copy link
Contributor Author

@pk044 pk044 Feb 11, 2021

Choose a reason for hiding this comment

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

I think this gist describes it better than I do: https://gist.github.com/smarter/2e1c564c83bae58c65b4f3f041bfb15f#a-type-soudness-bug-in-pattern-matching

there's one 'slight change' though - there's no ClassCastException being thrown in the latest version of Dotty (in the snippet provided below), there's a compilation error instead - the same as in Sealed:

class C[+T]

case class D[S](_s: S) extends C[S] {
  var s: S = _s
}

object Test {
  def main(args: Array[String]): Unit = {
    val x = new D[String]("abc")
    val y: C[Object] = x

    y match {
      case d @ D(x) =>
        d.s = new Integer(1)
//Found:    Integer
//Required: S$1
//where:    S$1 is a type in method main with bounds <: Object
    }
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh - this is Scala for you. Well, as you can see, our example could not introduce any unsoudness (if you check the paper linked in the GIST, this is restricted refinement encoding - ) and all this stuff was inferred correctly (note that A0 is actually like existential here - in that we don't even care what it is). But this can be an useful hint. You can probably get rid of all pattern matching by introducing a helper method e.g. reduce(cont) and overriding it in each subclass (FlatMap and Fold steps are roughly the same in all the interesting cases - it's continuation that differs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcin-rzeznicki I tried implementing the Visitor pattern (at least that's what I think you suggested to implement), but at every attempt it seems like variance is always getting in the way: https://scastie.scala-lang.org/W20dZap8Suq7jxfqcnyrFQ

On another hand - I wonder how dangerous would it be to go from:

case Fold(v0, l0: (ADT1 => Sealed[F, A0, ADT]), r0) => F.pure(Left(Fold(v0, runFold(l0), runFold(r0))))

to:

case Fold(v0, l0, r0) => F.pure(Left(Fold(v0, runFold(l0.asInstanceOf[ADT => Sealed[F, A0, ADT]]), runFold(r0))))

since we don't care what A0 is - how much would it hurt to use type-casting against it? 😄

@pk044 pk044 marked this pull request as ready for review December 16, 2021 15:28
@pk044 pk044 merged commit 3ba8218 into master Dec 16, 2021
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.

3 participants