Skip to content

Precise apply for enum companion objects #9728

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

Closed
wants to merge 10 commits into from

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Sep 4, 2020

Here we explicitly provide the return type for apply, copy, and unapply for all case class definitions which means the factory methods for enum class cases are no longer widened.
Secondly when inferring types for an enum value, we widen to the base class, however if the bound of an inferred type is an enum then we do not widen, this allows us to define recursive enums such as Nat and preserve type information of nested values.

fixes #3935
fixes #6781

This doesn't really test with higher kinded types but I couldn't think of decent examples

@bishabosha bishabosha force-pushed the topic/enum-specialise branch from 8ef2a98 to 6fce4ca Compare September 4, 2020 15:38
@bishabosha bishabosha force-pushed the topic/enum-specialise branch from c3c09e3 to 3110a00 Compare September 9, 2020 12:32
@bishabosha
Copy link
Member Author

bishabosha commented Sep 9, 2020

here's one example of kind of thing that breaks with the new changes to widen to the parent class:

import OneCodes._

enum OneCodes:
  case JumpFromRegister(register: Int)
  case Jump(label: String)

def onecode[O](code: O)(getName: O => String, getAddr: O => Int): String =
  s"${getName(code)} ${getAddr(code)}"

def printASM(oneCodes: OneCodes): String = oneCodes match
  case jr: JumpFromRegister => onecode(jr)(_.enumLabel, _.register)
  case _                    => "<other codes>"

errors with:

11 |  case jr: JumpFromRegister => onecode(jr)(_.enumLabel, _.register)
   |                                                        ^^^^^^^^^^
   |                              value register is not a member of OneCodes

i.e we see in the inferred types

case jr @ _:OneCodes.JumpFromRegister => 
  onecode[OneCodes](jr)( // widening from `JumpFromRegister` to `OneCodes`
    {
      def $anonfun(_$1: OneCodes): String = _$1.enumLabel
      closure($anonfun)
    }

so should onecode remain parametrically polymorphic and we instead never widen types of bound patterns (assuming local variables can't escape to the outer scope),
or should onecode add a bound of O <: OneCodes so that it will infer JumpFromRegister, or should the pattern match just be refactored to use an extractor pattern instead of type pattern

For now I will fix the test tests/patmat/i7186.scala by explicitly passing the narrowed type parameter

what do you think @smarter?

@smarter
Copy link
Member

smarter commented Sep 9, 2020

Yeah this is the sort of issues that #9076 is attempting to fix, I wouldn't worry too much about it. In this particular example I think adding a <: OneCodes bound would make sense. What we should do however is bring attention to the widening behavior of enums in the documentation with an example (and show how it can be avoided using an explicit type ascription or an upper bound)

Comment on lines 366 to 367
if isSingleton(bound) || isEnum(bound) then inst
else dropSuperTraits(widenOr(widenEnum(widenSingle(inst))))
Copy link
Member

@smarter smarter Sep 9, 2020

Choose a reason for hiding this comment

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

I think that's still not quite right: if the upper bound is an enum, a singleton should still be widened unless it's the type of an enum case.

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mean remove singletons except the term ref of a singleton enum case?

Copy link
Member

Choose a reason for hiding this comment

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

yeah

Copy link
Member Author

Choose a reason for hiding this comment

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

this seems to get a lot trickier when unions of singletons are involved

Copy link
Member Author

Choose a reason for hiding this comment

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

new steps are widenSingle > widenOr > [widenEnumCase] > dropSuperTraits, where widenOr is intercepted so that singletons of module or enum value do not widen

@smarter
Copy link
Member

smarter commented Sep 10, 2020

Concerning your scodec change, I think instead I would write a few more vals with explicit types to make it clear what's going on:

    val nonReserved: Codec[Color] = mappedEnum(uint8, Color.Red -> 1, Color.Green -> 2, Color.Blue -> 3)
    val reserved: Codec[Color.Reserved] = uint8.as[Color.Reserved]
    val nonReservedRight: Codec[Right[Color.Reserved, Color]] = nonReserved.xmapc(Right.apply)(_.toOption.get)
    val reservedLeft: Codec[Left[Color.Reserved, Color]] = reserved.xmapc(Left.apply)(_.swap.toOption.get)
    val codec: Codec[Either[Color.Reserved, Color]] = choice(
      nonReservedRight.upcast,
      reservedLeft.upcast
    )

By giving an explicit result type to the xmapc calls, inference can figure out the rest, and crucially we now can clearly see the types on which we call upcast, this is important because this affects the behavior of upcast since it implicitly takes a ClassTag based on the type of the input, so this is more robust.

@bishabosha
Copy link
Member Author

@smarter that does not work, should it?:

[error] -- [E007] Type Mismatch Error: /Users/jamie/Workspace/dotty/community-build/community-projects/scodec/unitTests/src/test/scala/scodec/codecs/DiscriminatorCodecTest.scala:89:104 
[error] 89 |      val reservedLeft: Codec[Left[Color.Reserved, Color]] = reserved.xmapc(Left.apply)(_.swap.toOption.get)
[error]    |                                                                                        ^^^^^^^^^^^^^^^^^^^
[error]    |                                                Found:    Color
[error]    |                                                Required: Color.Reserved
[warn] one warning found
[error] one error found

@smarter
Copy link
Member

smarter commented Sep 11, 2020

Weird, I tried a similar example locally and it worked but there might be something different here, one solution is to be a bit more explicit and pass type parameters to Left.apply or xmapc.

@bishabosha
Copy link
Member Author

bishabosha commented Sep 14, 2020

more tricky:

import Nat._

enum Nat:
  case Zero
  case Succ[N <: Nat](n: N)

def quoteZero(using QuoteContext): Expr[Zero.type] =
  '{Zero} // <'{<Nat$#Zero:Nat>}:((quoted.QuoteContext) ?=> quoted.Expr[Nat.Zero.type])>

the above fails -Ycheck:all because Ident(Zero) is a Nat, not Nat.Zero.type

if you do not try to construct a precisely typed quote it does not work when you try to do an inline match on the result of using quotes to construct a Nat, e.g.

inline def toIntInline(inline nat: Nat): Int = inline nat match
  case Zero    => 0
  case Succ(n) => toIntInline(n) + 1

transparent inline def toNatMacro(inline int: Int): Nat = ${ Macros.toNatImpl('int) }

object Macros:
  import quoted._

  def toNatImpl(int: Expr[Int])(using QuoteContext): Expr[Nat] =

    def inner(int: Int, acc: Expr[Nat]): Expr[Nat] = int match
      case 0 => acc
      case n => inner(n - 1, '{Succ($acc)})

    val Const(i) = int
    require(i >= 0)
    inner(i, '{Zero})

assert(toIntInline(toNatMacro(3)) == 3)

@smarter smarter closed this Sep 14, 2020
@smarter smarter reopened this Sep 14, 2020
@smarter
Copy link
Member

smarter commented Sep 14, 2020

Does the same issue exist if you try to do Expr[0] ?

@bishabosha
Copy link
Member Author

bishabosha commented Sep 14, 2020

@smarter no that looks fine:

def natZero(using x$1: quoted.QuoteContext): quoted.Expr[(0 : Int)] = 
        <
          <<'{<0:(0 : Int)>}:((quoted.QuoteContext) ?=> quoted.Expr[(0 : Int)])>
            .
          apply:((using x$0: quoted.QuoteContext): quoted.Expr[(0 : Int)])>
        (<x$1:quoted.QuoteContext>):quoted.Expr[(0 : Int)]>

but I see the same thing with typing Ident outside of quotes, but I think that's an issue with the Refined printer not printing singletons maybe, the result of typedIdent has singleton type

@bishabosha
Copy link
Member Author

@bishabosha
Copy link
Member Author

bishabosha commented Sep 14, 2020

@smarter so it seems here that even though the bound of N in List[N] is <: Nat it still widens because B in List.:: has empty bounds, I don't know if we expect the precise type given that ls narrows the bounds of its arguments

def ls[N <: Nat](ls: List[N]): ls.type = ls

ls(Succ(Zero)::Nil)
// ls[Nat]({
//   val elem$1: Nat.Succ[Nat.Zero.type] = 
//     Nat$#Succ.apply[Nat.Zero.type](Nat$#Zero)
//   Nil.::[Nat](elem$1)
// })

@smarter
Copy link
Member

smarter commented Sep 14, 2020

Interesting, based on the printed tree I guess this is specific to right-associative operators as they get expanded into a val and an expression, and the val doesn't have an expected type?

@smarter
Copy link
Member

smarter commented Sep 14, 2020

The same sort of issue can probably be reproduced on master with a singleton bound or a union bound

@bishabosha
Copy link
Member Author

its the same with Options:

def ls[N <: Nat](ls: Option[N]): ls.type
ls(new Some(Succ(Zero)))
// ls[Nat](new Some[Nat](Nat$#Succ.apply[Nat.Zero.type](Nat$#Zero)))

@bishabosha
Copy link
Member Author

bishabosha commented Sep 14, 2020

if I do it with singleton bound it does work:

scala> def ls[N <: Singleton](ls: Option[N]): ls.type = ls
def ls[N <: Singleton](ls: Option[N]): ls.type

scala> ls(new Some(1))
val res0: Some[1] = Some(1)

scala> val foo = Option("hello")
val foo: Option[String] = Some(hello)

scala> ls(new Some(foo))                                                                          
val res1: Some[foo.type] = Some(Some(hello))

so maybe something else deals with these bounds :/

@bishabosha
Copy link
Member Author

bishabosha commented Sep 14, 2020

@smarter it works if I change isEnum in ConstraintHandling to check <:< scala.Enum, but I want to make sure a type parameter is a subtype of an unknown concrete enum class with the enum flag, do you know a good way to do that? I assume this is the sort of question that can't be answered efficiently - do we need a new kind of constraint?

@bishabosha
Copy link
Member Author

After offline discussion, this will be reopened as a new PR with some changes

@bishabosha bishabosha closed this Sep 28, 2020
@bishabosha bishabosha deleted the topic/enum-specialise branch September 10, 2021 14:18
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.

Cannot reduce inline match with an enum-based Nat Enum expansion does not work for singleton type parameters
2 participants