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

Generalize with-chain wrapping for defs and type definitions #1132

Merged
merged 5 commits into from Apr 14, 2018

Conversation

3 participants
@iantabolt
Contributor

iantabolt commented Apr 11, 2018

This fixes #1125 to handle wrapping of "with-chains".

For example, a long chain of

Foo with Bar with Baz

can be wrapped as

Foo
  with Bar
  with Baz

This was already implemented for self-types (trait Xyz { self: Foo with Bar with Baz => ). This PR expands the behavior to handle type definitions (type MyType = Foo with Bar with Baz) and function return types (def xyz: Foo with Bar with Baz).

@@ -1021,7 +1021,7 @@ class Router(formatOps: FormatOps) {
Split(Space, 0),
Split(Newline, 1).withPolicy(policy)
)
case t @ SelfAnnotation(top) =>
case t @ WithChain(top) =>

This comment has been minimized.

@iantabolt

iantabolt Apr 11, 2018

Contributor

I don't love this name but I wanted to change it to something more general than SelfAnnotation. Any suggestions?

This comment has been minimized.

@olafurpg

olafurpg Apr 13, 2018

Member

This name is good 👍

@olafurpg

Thanks for this contribution! New name looks good, we should make this work for all result types.

case (top: Type.With) `:parent:` (_: Term.Param) `:parent:` (_: Template) =>
Some(top)
// type definitions
case (top: Type.With) `:parent:` (_: Defn.Type) =>

This comment has been minimized.

@olafurpg

olafurpg Apr 11, 2018

Member

I suspect this pattern can be extended to any Defn or Decl, that will cover val/var/def regardless if they're abstract or not.

This comment has been minimized.

@olafurpg

olafurpg Apr 11, 2018

Member

I'm not sure when we don't want this to match for Type.With, there are other tree nodes like Term.ApplyType where this is equally relevant.

This comment has been minimized.

@iantabolt

iantabolt Apr 11, 2018

Contributor

Yeah I agree for the most part.

If I open it up to match all Type.Withs then one test fails (with good reason IMO):

// Before:
{ def getAnnotationOwner(
       annotationOwnerLookUp: ScLiteral => Option[
         PsiAnnotationOwner with PsiElement]): Option[PsiAnnotationOwner] = 1 }

// After -- breaks in generic type:
{
  def getAnnotationOwner(
      annotationOwnerLookUp: ScLiteral => Option[PsiAnnotationOwner
        with PsiElement]): Option[PsiAnnotationOwner] = 1
}

// Expected:
{
  def getAnnotationOwner(
      annotationOwnerLookUp: ScLiteral => Option[
          PsiAnnotationOwner with PsiElement]): Option[PsiAnnotationOwner] = 1
}

From https://github.com/scalameta/scalafmt/blob/master/scalafmt-tests/src/test/resources/default/TypeWith.stat

This comment has been minimized.

@iantabolt

iantabolt Apr 11, 2018

Contributor

But even in that scenario, if the type were long enough we probably want a newline instead of violating the column limit. Maybe we can make this split a low priority or high cost etc?

@ceedubs

This comment has been minimized.

ceedubs commented Apr 12, 2018

This looks like a great change! I'm adding some examples here based on @olafurpg's request in #1133.

In Cats when we have code like:

  implicit val catsDataInstancesForNonEmptyList: SemigroupK[NonEmptyList]
    with Reducible[NonEmptyList] with Bimonad[NonEmptyList] with NonEmptyTraverse[NonEmptyList] = ...

  implicit val catsDataInstancesForNonEmptyVector: SemigroupK[NonEmptyVector]
    with Reducible[NonEmptyVector] with Bimonad[NonEmptyVector]
    with NonEmptyTraverse[NonEmptyVector] = ...

   implicit val catsStdInstancesForOption
    : Traverse[Option] with MonadError[Option, Unit] with Alternative[Option]
      with CommutativeMonad[Option] with CoflatMap[Option] = ...

After formatting it ends up looking like:

  implicit val catsDataInstancesForNonEmptyList: SemigroupK[NonEmptyList] with Reducible[
    NonEmptyList] with Bimonad[NonEmptyList] with NonEmptyTraverse[NonEmptyList] = ...

  implicit val catsDataInstancesForNonEmptyVector: SemigroupK[NonEmptyVector] with Reducible[
    NonEmptyVector] with Bimonad[NonEmptyVector] with NonEmptyTraverse[NonEmptyVector] = ...

   implicit val catsStdInstancesForOption
    : Traverse[Option] with MonadError[Option, Unit] with Alternative[Option] with CommutativeMonad[
      Option] with CoflatMap[Option] = ...

It would be nice to see the style changes exhibited in this PR to extend to the return types of vals like these.

@iantabolt

This comment has been minimized.

Contributor

iantabolt commented Apr 12, 2018

@ceedubs Thanks for the input! I'll add these as test cases.

I ran the examples through with this change and it ended up with

implicit val catsDataInstancesForNonEmptyList: SemigroupK[NonEmptyList]
  with Reducible[NonEmptyList]
  with Bimonad[NonEmptyList]
  with NonEmptyTraverse[NonEmptyList] = ???

implicit val catsDataInstancesForNonEmptyVector: SemigroupK[NonEmptyVector]
  with Reducible[NonEmptyVector]
  with Bimonad[NonEmptyVector]
  with NonEmptyTraverse[NonEmptyVector] = ???

implicit val catsStdInstancesForOption: Traverse[Option] with MonadError[Option, Unit]
  with Alternative[Option]
  with CommutativeMonad[Option]
  with CoflatMap[Option] = ???

Does that seem about right? I'm not sure why the last one didn't drop MonadError to the next line 🤔

Edit: Figured it out. I was running with { case With(top) => Some(top) } which doesn't actually enforce that it is the top with.

@ceedubs

This comment has been minimized.

ceedubs commented Apr 12, 2018

@iantabolt nice! On the last one, maybe it has to do with inlining until you hit the column limit? I'm not super familiar with the particulars of behavior. And does it depend on binPack.parentConstructors? Either way, I think that it's a really big improvement and I'd be happy with it.

with HasThrottles
with HasThrowableNotifier
<<< abstract def #1125
trait NeedsServices {

This comment has been minimized.

@iantabolt

iantabolt Apr 12, 2018

Contributor

This probably shouldn't be in Type.stat since there is no type. Maybe all these tests should be in TypeWith.stat?

This comment has been minimized.

@olafurpg

olafurpg Apr 13, 2018

Member

TypeWith.stat would be good, this is important enough to merit a separate suite.

@olafurpg

A couple more minor comments.

@@ -1021,7 +1021,7 @@ class Router(formatOps: FormatOps) {
Split(Space, 0),
Split(Newline, 1).withPolicy(policy)
)
case t @ SelfAnnotation(top) =>
case t @ WithChain(top) =>

This comment has been minimized.

@olafurpg

olafurpg Apr 13, 2018

Member

This name is good 👍

: Traverse[Option] with MonadError[Option, Unit] with Alternative[Option]
with CommutativeMonad[Option] with CoflatMap[Option] = ???
>>>
implicit val catsStdInstancesForOption: Traverse[Option]

This comment has been minimized.

@olafurpg

olafurpg Apr 13, 2018

Member

This may be more ambitious that you were planning for (so "no" is an acceptable answer), but while were fixing with chains, could we add a test case for?

new A with B with C with D ...

I've seen surprising behavior in these scenarios and I suspect fixing it likewise is a matter of adding another case to WithChain

This comment has been minimized.

@iantabolt

iantabolt Apr 13, 2018

Contributor

Yes no problem (I think)

This comment has been minimized.

@iantabolt

iantabolt Apr 13, 2018

Contributor

I might need some help with this. I don't think this falls into the Type.With case and instead hits the Template case.

My thought is to treat this like the KwExtends case above and do

       case tok @ FormatToken(_, right @ KwWith(), _) =>
         rightOwner match {
+          // Anonymous template (new A with B { ... })
+          case template: Template if template.parent.exists(_.is[Term.New]) =>
+            val lastToken = template.tokens.last
+            binPackParentConstructorSplits(
+              Set(template),
+              lastToken,
+              2
+            )
           case template: Template =>
             val hasSelfAnnotation = template.self.tokens.nonEmpty

This almost works but it indents after every with

new A
  with B
    with C
      with D

Any ideas?

This comment has been minimized.

@iantabolt

iantabolt Apr 13, 2018

Contributor

I pushed the non-working change mentioned above. I'm assuming the fix is something analogous to the isFirstWith logic but I'm not sure how to achieve it with the template

This comment has been minimized.

@olafurpg

olafurpg Apr 13, 2018

Member

Hmm, the changes seem unrelated enough to merit a separate PR. Your change already improves on the state-of-the-art so I propose you drop the last commit, we merge and move this case to another ticket.

I'll need to locally play around with this case to see how would be best to solve it.

This comment has been minimized.

@iantabolt

iantabolt Apr 13, 2018

Contributor

SGTM 👍. I rolled it back

with HasThrottles
with HasThrowableNotifier
<<< abstract def #1125
trait NeedsServices {

This comment has been minimized.

@olafurpg

olafurpg Apr 13, 2018

Member

TypeWith.stat would be good, this is important enough to merit a separate suite.

@olafurpg

BTW, you can see what type nodes are for a given syntax with the ammonite repl like this

@ import $ivy.`org.scalameta:testkit_2.12:1.7.0`, scala.meta._, testkit._
import $ivy.$                                 , scala.meta._, testkit._

@ q"new A with B with C"
res1: Term.New = Term.New(
  Template(
    List(),
    List(Ctor.Ref.Name("A"), Ctor.Ref.Name("B"), Ctor.Ref.Name("C")),
    Term.Param(List(), _, None, None),
    None
  )
)
@olafurpg

LGTM 👍 Thanks for this contribution @iantabolt !

@olafurpg olafurpg merged commit 906ccd9 into scalameta:master Apr 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@olafurpg

This comment has been minimized.

Member

olafurpg commented Apr 14, 2018

I'll be attending a conference for the next week, but I hope to have v1.5.0 out this April. Until then, you can already use the pre-releases to Bintray as documented on the website but note those don't include the IntelliJ plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment