Skip to content

Minimal enums with precise apply methods #9922

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

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Sep 30, 2020

This is a reworking of #9728 with the complete semantics detailed below:

  • All case class companions generate copy, apply and unapply with explicit result types, including when they are an enum. This result type is a fully applied reference to the case class.

  • In type inference, after widening singleton types and union types, check the upper bound. If the upper bound is a class enum case, keep the type, else replace top-level reference to a class enum case with a reference to its parent enum type.

Included in the tests are some demonstrations of reducing enums with quoted pattern match, and some type inference checks

Non Goals in this PR:

  • make recursive enums such as type level Nat work with inline match

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.

I would like to evaluate the more restricted scheme where we decide locally when we type the enum apply method whether we should use the narrow or the wide type.

def isSingleton(tp: Type): Boolean = tp match
case WildcardType(optBounds) => optBounds.exists && isSingleton(optBounds.bounds.hi)
case _ => isSubTypeWhenFrozen(tp, defn.SingletonType)

def isEnumCase(tp: Type): Boolean = tp match
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to extract the commonalities of isEnumCase and isSingleton into a method

def widenWildcard(tp: Type) = tp match
  case WildcardType(optBounds) => optBounds
  case _ => tp

@@ -1146,6 +1146,11 @@ object Types {
case _ => this
}

/** if this type is a reference to a class case of an enum, replace it by its first parent */
final def widenEnumCase(using Context): Type = this match
case tp: (TypeRef | AppliedType) if tp.classSymbol.isAllOf(EnumCase) => tp.parents.head
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the restriction to TypeRefs and AppliedTypes? What should happen with TypeVars, annotated types, or aliases?
A more pervasive alternative would be:

if tp.classSymbol.isAllOf(EnumCase) then tp.parents.head else tp

val wideInst =
if isSingleton(bound) then inst
else dropSuperTraits(widenOr(widenSingle(inst)))
else
val lub = widenOr(widenSingle(inst))
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nicer if widenEnum simply returns the argument type if it is not an enum case. Then we could just use

dropSuperTraits(widenEnum(widenOr(widenSingle(inst))))

here

@@ -1399,8 +1399,8 @@ object Scanners {

object IndentWidth {
private inline val MaxCached = 40
private val spaces = Array.tabulate(MaxCached + 1)(new Run(' ', _))
private val tabs = Array.tabulate(MaxCached + 1)(new Run('\t', _))
private val spaces = Array.tabulate[Run](MaxCached + 1)(new Run(' ', _)) // TODO: remove new after bootstrap
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, this gives me pause. Is this really what we want? I find it surprising and unnatural that one needs to add [Run] here. /cc @smarter

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative design would be more local: Instead of widening enum cases everywhere where they take part in inference we only decide at the application itself. I.e. new Run(...) is always a Run and will stay that way. Run(...) is of type IndentWidth unless its expected type is Run. We should evaluate which scheme is preferable. My tendency is to prefer the alternative.

Copy link
Member

Choose a reason for hiding this comment

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

If you want an Array of Run you can write explicitly val spaces: Array[Run] = Array.tabulate(...)(Run(...)), that sounds OK to me and not much longer than adding a new.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. I mean the worrying thing is that the Run type is in a sense provisional, and can be revoked at each type inference step. That's analogous to union and singleton types of course. But I would argue that the behavior in these cases can also be suprising and it's a necessary tradeoff. Only for enum the tradeoff does not look to be so necessary or natural.

appliedEnumRef: Tree)(using Context): (Tree, List[DefDef]) = {

def extractType(t: Tree): Tree = t match {
case Apply(t1, _) => extractType(t1)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice that we can simplify desugaring in a significant way.

@odersky odersky assigned bishabosha and smarter and unassigned odersky and bishabosha Oct 1, 2020
odersky added a commit to dotty-staging/dotty that referenced this pull request Oct 1, 2020
Keeps many elements from scala#9922 but the modality where we do the
widening is different. The new rule is as follows:

In an application of a compiler-generated apply or copy method of an enum case,
widen its type to the underlying supertype of the enum case by means of a type
ascription, unless the expected type is an enum case itself.
odersky added a commit to dotty-staging/dotty that referenced this pull request Oct 1, 2020
Keeps many elements from scala#9922 but the modality where we do the
widening is different. The new rule is as follows:

In an application of a compiler-generated apply or copy method of an enum case,
widen its type to the underlying supertype of the enum case by means of a type
ascription, unless the expected type is an enum case itself.
odersky added a commit to dotty-staging/dotty that referenced this pull request Oct 2, 2020
Keeps many elements from scala#9922 but the modality where we do the
widening is different. The new rule is as follows:

In an application of a compiler-generated apply or copy method of an enum case,
widen its type to the underlying supertype of the enum case by means of a type
ascription, unless the expected type is an enum case itself.
@bishabosha
Copy link
Member Author

superseded by #9932

@bishabosha bishabosha closed this Oct 2, 2020
@bishabosha bishabosha deleted the topic/enum-precise-apply-methods branch September 10, 2021 14:17
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.

3 participants