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

Consider changing layout of objects and traits in the types package #416

Closed
fthomas opened this issue Jan 22, 2018 · 4 comments
Closed

Consider changing layout of objects and traits in the types package #416

fthomas opened this issue Jan 22, 2018 · 4 comments

Comments

@fthomas
Copy link
Owner

fthomas commented Jan 22, 2018

From

object string extends StringTypes

trait StringTypes {
  type NonEmptyString = String Refined NonEmpty
  object NonEmptyString extends RefinedTypeOps[NonEmptyString, String]
}

to

object string {
  type NonEmptyString = String Refined NonEmpty
  object NonEmptyString extends RefinedTypeOps[NonEmptyString, String]
}

trait StringTypes {
  final type NonEmptyString = string.NonEmptyString
  final val NonEmptyString = string.NonEmptyString
}

The latter has the benefit that if one types NonEmptyString in a file that hasn't already imported that symbol, IntelliJ will be able to autocomplete the name and automatically import string.NonEmptyString if the name is completed. If the actual definition of NonEmptyString is in a trait, IntelliJ is able to autocomplete the name but won't import it since StringTypes.NonEmptyString cannot be imported.

I'm hesitant to make this change purely because of ergonomics of one IDE, so I'm still looking for other justifications for this change.

This layout is not binary compatible with the current layout and needs to be done in 0.9 or later. It seems to be source compatible though.

@fthomas
Copy link
Owner Author

fthomas commented Jan 22, 2018

Another advantage of the proposed layout is that case classes can be declared final without triggering this compile error: The outer reference in this type test cannot be checked at run time.. This does not (yet) affect the types package since it only contains type aliases and companion objects for now.

@fthomas
Copy link
Owner Author

fthomas commented Jan 23, 2018

Here are more advantages:

// current layout

scala> val x = all.NonEmptyString(".")
x: all.NonEmptyString = .

scala> val y = string.NonEmptyString(".")
y: string.NonEmptyString = .

scala> implicitly[all.NonEmptyString =:= string.NonEmptyString]
res10: all.NonEmptyString =:= string.NonEmptyString = <function1>

scala> implicitly[all.NonEmptyString.type =:= string.NonEmptyString.type]
<console>:36: error: Cannot prove that all.NonEmptyString.type =:= string.NonEmptyString.type.
       implicitly[all.NonEmptyString.type =:= string.NonEmptyString.type]
                 ^
// proposed layout

scala> val x = all.NonEmptyString(".")
x: string.NonEmptyString = .

scala> val y = string.NonEmptyString(".")
y: string.NonEmptyString = .

scala> implicitly[all.NonEmptyString =:= string.NonEmptyString]
res1: all.NonEmptyString =:= string.NonEmptyString = <function1>

scala> implicitly[all.NonEmptyString.type =:= string.NonEmptyString.type]
res2: all.NonEmptyString.type =:= string.NonEmptyString.type = <function1>

There are two differences:

  1. with the proposed layout the compiler uses the same string representation for NonEmptyString type independent of the path the companion is accessed
  2. the proposed layout creates less types: all.NonEmptyString.type and string.NonEmptyString.type are the same type (as it should be)

@kusamakura
Copy link
Contributor

I think it's worth it. It's cleaner and like you said, creates less instances.

@fthomas
Copy link
Owner Author

fthomas commented Jan 29, 2018

The proposed layout also matches how the standard library makes some types available in the scala package:

package object scala {
...
  type Vector[+A] = scala.collection.immutable.Vector[A]
  val Vector = scala.collection.immutable.Vector
...
}

I'm convinced. This will happen in 0.9.0.

fthomas added a commit that referenced this issue Feb 1, 2018
fthomas added a commit that referenced this issue Feb 1, 2018
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

No branches or pull requests

2 participants