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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Apr 14, 2018
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2a04ef3
Treat Defn.Type `with` chains the same as self types.
olafurpg Mar 27, 2018
1873e71
Add harder #1125 tests
iantabolt Apr 11, 2018
3d06212
Better tests for #1125 and rename SelfAnnotation=>WithChain
iantabolt Apr 11, 2018
d9a481c
Open up with formats to all Defn/Decl; add cats test for #1133
iantabolt Apr 12, 2018
b60a53d
Move tests from Type.stat to TypeWith.stat
iantabolt Apr 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Router.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import org.scalafmt.internal.ExpiresOn.Right
import org.scalafmt.internal.Length.Num
import org.scalafmt.internal.Length.StateColumn
import org.scalafmt.internal.Policy.NoPolicy
import org.scalafmt.util.`:parent:`
import org.scalafmt.util.Delim
import org.scalafmt.util.CtorModifier
import org.scalafmt.util.InfixApplication
Expand All @@ -33,7 +32,7 @@ import org.scalafmt.util.Literal
import org.scalafmt.util.LoggerOps
import org.scalafmt.util.Modifier
import org.scalafmt.util.RightParenOrBracket
import org.scalafmt.util.SelfAnnotation
import org.scalafmt.util.WithChain
import org.scalafmt.util.SomeInterpolate
import org.scalafmt.util.TokenOps
import org.scalafmt.util.TreeOps
Expand Down Expand Up @@ -1021,7 +1020,7 @@ class Router(formatOps: FormatOps) {
Split(Space, 0),
Split(Newline, 1).withPolicy(policy)
)
case t @ SelfAnnotation(top) =>
case t @ WithChain(top) =>
Copy link
Contributor Author

@iantabolt iantabolt Apr 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

This name is good 👍

val isFirstWith = !t.lhs.is[Type.With]
if (isFirstWith) {
val chain = withChain(top)
Expand Down
8 changes: 7 additions & 1 deletion scalafmt-core/shared/src/main/scala/org/scalafmt/util/TreeExtractors.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package org.scalafmt.util

import scala.collection.immutable.Seq
import scala.meta.Decl
import scala.meta.Defn
import scala.meta.Name
import scala.meta.Pat
import scala.meta.Template
Expand Down Expand Up @@ -37,11 +39,15 @@ object `:parent:` {
}
}

object SelfAnnotation {
object WithChain {
def unapply(t: Type.With): Option[Type.With] =
TreeOps.topTypeWith(t) match {
// self types
case (top: Type.With) `:parent:` (_: Term.Param) `:parent:` (_: Template) =>
Some(top)
// val/def/var/type definitions or declarations
case (top: Type.With) `:parent:` (_: Defn | _: Decl) =>
Some(top)
case _ => None
}
}
10 changes: 10 additions & 0 deletions scalafmt-tests/src/test/resources/default/TypeWith.stat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,13 @@
annotationOwnerLookUp: ScLiteral => Option[
PsiAnnotationOwner with PsiElement]): Option[PsiAnnotationOwner] = 1
}
<<< #1133
implicit val catsStdInstancesForOption
: Traverse[Option] with MonadError[Option, Unit] with Alternative[Option]
with CommutativeMonad[Option] with CoflatMap[Option] = ???
>>>
implicit val catsStdInstancesForOption: Traverse[Option]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes no problem (I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM 👍. I rolled it back

with MonadError[Option, Unit]
with Alternative[Option]
with CommutativeMonad[Option]
with CoflatMap[Option] = ???
26 changes: 26 additions & 0 deletions scalafmt-tests/src/test/resources/unit/Type.stat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,29 @@ type Row =
type a = b_! # D
>>>
type a = b_! #D
<<< with-chain types #1125
type RequiredServices = Bag with
SomeOtherService.RequiredServices
with HasDynamicConfig with HasThrottles
with HasThrowableNotifier
>>>
type RequiredServices = Bag
with SomeOtherService.RequiredServices
with HasDynamicConfig
with HasThrottles
with HasThrowableNotifier
<<< abstract def #1125
trait NeedsServices {
Copy link
Contributor Author

@iantabolt iantabolt Apr 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

def services: Bag with
SomeOtherService.RequiredServices
with HasDynamicConfig with HasThrottles
with HasThrowableNotifier
}
>>>
trait NeedsServices {
def services: Bag
with SomeOtherService.RequiredServices
with HasDynamicConfig
with HasThrottles
with HasThrowableNotifier
}