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

New RedundantBraces is too aggressive #1147

Open
olafurpg opened this Issue Apr 23, 2018 · 4 comments

Comments

1 participant
@olafurpg
Member

olafurpg commented Apr 23, 2018

This template is a guideline, not a strict requirement.

  • Version: 1.5.0
  • Integration: cli
  • Configuration:
rewrite.rules = [
  RedundantBraces
]

Steps

Given code like this:

      case FormatToken(open @ LeftParen(), _, _) if {
            val isSuperfluous = isSuperfluousParenthesis(open, leftOwner)
            leftOwner match {
              case _: Term.If | _: Term.While | _: Term.For | _: Term.ForYield
                  if !isSuperfluous =>
                true
              case _ => false
            }
          } =>

When I run scalafmt like this:

scalafmt

Problem

Scalafmt formats code like this:

>
...
[warn] Error in /Users/ollie/dev/scalafmt/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Router.scala: <input>:1036: error: illegal start of simple expression
val isSuperfluous = isSuperfluousParenthesis(open, leftOwner)
            ^

Expectation

I expect the code to format since it has no parse errors.

Workaround

rewrite.redundantBraces.generalExpressions = false

Notes

Regression since #1122

@olafurpg

This comment has been minimized.

Member

olafurpg commented Apr 24, 2018

I don't know if this is intended behavior or not, but I was surprised to see

-    while (curr.left.start < end.start && curr != prev) {
-      if (matches(curr.left)) {
-        result += curr.left
-        goToMatching()
-      } else {
-        prev = curr
-        curr = next(curr)
-      }
+    while (curr.left.start < end.start && curr != prev) if (matches(curr.left)) {
+      result += curr.left
+      goToMatching()
+    } else {
+      prev = curr
+      curr = next(curr)
     }

Question if curly braces should be kept around while loops and if statements without an else 🤔

@olafurpg

This comment has been minimized.

Member

olafurpg commented Apr 24, 2018

Infix operators are a tough one as well,

-            (opensConfigStyle(formatToken) || {
-              forceConfigStyle(leftOwner) && !styleMap.forcedBinPack(leftOwner)
-            }) =>
+            (opensConfigStyle(formatToken) ||
+              forceConfigStyle(leftOwner) && !styleMap.forcedBinPack(leftOwner)) =>

It's technically redundant but operator precedence and associativity is difficult sometimes so guiding braces may help readability

@olafurpg

This comment has been minimized.

Member

olafurpg commented Apr 26, 2018

Opened #1157 to disable generalExpressions by default until this is fixed to unblock v1.5.1

@olafurpg

This comment has been minimized.

Member

olafurpg commented Apr 26, 2018

I took a stab at fixing this in https://github.com/olafurpg/scalafmt/compare/general-expression?expand=1 but there are some complications to distinguish between case bodies and case guards.

@japgolly would you be interested in taking a stab at this?

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