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

Remove redundant braces from if-else and case expressions #1122

Merged
merged 14 commits into from Mar 18, 2018

Conversation

2 participants
@japgolly
Contributor

japgolly commented Mar 13, 2018

No description provided.

@olafurpg

Very cool! Thank you for this contribution.

Only blocking comment is the if (true) 1 case. If you are motivated to generalize the implementation for any Term.Block I think that might be a nice cleanup, but it's not necessary.

private def patchExpr(term: Term, inBlock: Boolean)
(implicit builder: PatchBuilder, ctx: RewriteCtx): Unit = {
val open = term.tokens.head

This comment has been minimized.

@olafurpg

olafurpg Mar 14, 2018

Member

It's better to use headOption to avoid crashing on if (true) print("") (without else) because

ollie-scalafix-scopes@ q"if (true) 1"
res6: Term.If = Term.If(Lit.Boolean(true), Lit.Int(1), ())

ollie-scalafix-scopes@ "if (true) 1".parse[Term].get.asInstanceOf[Term.If].elsep.tokens.isEmpty
res7: Boolean = true

This comment has been minimized.

@olafurpg

olafurpg Mar 14, 2018

Member

Starting with

term match {
  case block: Term.Block =>
    ...
  case _ =>
}

would be an OK way to keep .head/.last

This comment has been minimized.

@olafurpg

olafurpg Mar 15, 2018

Member

Actually looks like some Term.Block can also have empty tokens

ollie-scalafix-scopes@ "case x =>".parse[Case].get.asInstanceOf[Case].body
res10: Term = Term.Block(List())

ollie-scalafix-scopes@ "case x =>".parse[Case].get.asInstanceOf[Case].body.tokens
res11: Tokens = Tokens()

This comment has been minimized.

@japgolly

japgolly Mar 16, 2018

Contributor

Oh my, what a silly mistake 😆 I'll check for emptyness. And I like the way you're quickly testing things out in the console, fantastic idea, I'll adopt that. Thanks :)

builder += TokenPatch.Remove(open)
builder += TokenPatch.Remove(close)
case x: Term.If if settings.ifElseClauses =>

This comment has been minimized.

@olafurpg

olafurpg Mar 14, 2018

Member

I'm wondering if we can generalize the implementation to handle all single statement Term.Blocks in one go. What happens if we do something like the following psuedo code?

case b: Term.Block if b.stats.lengthCompare(1) == 0 =>
  for {
    open <- b.tokens.headOption
    if open.is[Token.LeftBrace]
    close <- b.tokens.headOption
    if close.is[Token.RightBrace]
  } yield List(remove(open), remove(left), removeTrailingLF(...))

That could handle if + else + case clauses. If there's a wish for more detailed configuration we could do it against b.parent.get.productPrefix which returns for example "Term.If" or "Case"

This comment has been minimized.

@japgolly

japgolly Mar 16, 2018

Contributor

That appeals to me much more conceptually but why wasn't it done that way before? The fact that it was only done for a few small cases suggests to me that a holistic approach didn't work... Is that not so? I'd be interested to know the history.

This comment has been minimized.

@japgolly

japgolly Mar 17, 2018

Contributor

Bah, I'm going down this path regardless. It's looking very promising! :D

@olafurpg

This comment has been minimized.

Member

olafurpg commented Mar 14, 2018

to fix the formatting check you can run ./scalafmt from the project directory. The other test failure in 2.12 is not spurious, I think it's related to if (true) 1.

@japgolly

This comment has been minimized.

Contributor

japgolly commented Mar 16, 2018

Hi @olafurpg. Firstly, thank you for the awesome feedback. This is my first time working with scala{fmt,meta} and appreciate all the help I can get :)

@japgolly

This comment has been minimized.

Contributor

japgolly commented Mar 17, 2018

@olafurpg Please take another look. I think this is much better now

olafurpg added some commits Mar 17, 2018

Integrate syntactic groups with RedundantBraces.
Previously, RedundantBraces would strip braces that were used to
influenece precedence.
```
// Before
{1 + 2} * 3
// After
1 + 2 * 3
```
This is a bug since the second program has a different meaning than the
first program.
@olafurpg

This was a nice cleanup! Thanks for the refactoring.

One last comment, can we add the new behavior where all term blocks are removed behind a configuration flag? Current RedundantBraces users should be able to upgrade while keeping the old behavior.

case _: Term.Function | _: Defn => false
case _ => true
}
exactlyOneStatement && blockSizeIsOk(b) && innerOk && !isProcedureSyntax(

This comment has been minimized.

@olafurpg

olafurpg Mar 17, 2018

Member

nitpick, I never cracked the code on infix operators so typically it's best to line break after && manually 😉 http://scalameta.org/scalafmt/#Infixapplications

private def retainSingleStatBlock(b: Term.Block): Boolean =
b.parent.exists {
case parentIf: Term.If =>
// if (a) { if (b) c } else d

This comment has been minimized.

@olafurpg

olafurpg Mar 17, 2018

Member

Good catch! I did not think of that

This comment has been minimized.

@olafurpg

olafurpg Mar 18, 2018

Member

Turns out this was a bug in scalacenter/scala-syntax#30 😄

insideIfThen && parentIfHasAnElse && blockIsIfWithoutElse
case _ =>
// No other (known) special cases

This comment has been minimized.

@olafurpg

olafurpg Mar 17, 2018

Member

When I think of it, I'm afraid special cases include whenever blocks are used to influence precedence

- {1 + 2} * 3
+ 1 + 2 * 3
- case x if { x match { case 1 => true } }
+ case x if x match { case 1 => true }

It's possible to detect these cases, but it requires a bit of machinery.

This comment has been minimized.

@olafurpg

olafurpg Mar 17, 2018

Member

I took the liberty to integrate those syntactic groups to ensure redundant braces does not remove blocks that are required to influence precedence and associativity.

@olafurpg

This comment has been minimized.

Member

olafurpg commented Mar 18, 2018

Sidenote, in Scalafmt v2 (#917) I want to move to a tree printer where transformations like this should be possible to express as simply as

tree.transform {
  case Term.Block(stat :: Nil) => stat
}

with automatic handling of precedence and other tricky cases. This would help make the scalafix and scalafmt rewrite API so much nicer to deal with, we're working towards this in https://github.com/scalacenter/scala-syntax

@japgolly

This comment has been minimized.

Contributor

japgolly commented Mar 18, 2018

Fantastic changes! I didn't think about op precedence at all and I'm very glad you had a solution up your sleeve.

I've added settings to allow the retention of the old behaviour. Back to you @olafurpg

case parent =>
val side = parent match {
case t: Term.ApplyInfix
if t.args.lengthCompare(1) == 0 && (t.args.head eq b) =>

This comment has been minimized.

@japgolly

japgolly Mar 18, 2018

Contributor

Note @olafurpg , I've changed this from case Term.ApplyInfix(_, _, _, arg :: Nil) if arg eq b => to

  1. be a little more runtime-efficient by bypassing unappys
  2. this now works as expected if args isn't a List
@olafurpg

LGTM 👍

Thanks a lot for iterating on the feedback @japgolly !

@olafurpg olafurpg merged commit 8f1c4ba into scalameta:master Mar 18, 2018

1 check passed

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

@olafurpg olafurpg added this to the v1.5.0 milestone Mar 18, 2018

@olafurpg

This comment has been minimized.

Member

olafurpg commented Mar 18, 2018

This fix should already be available as a pre-release http://scalameta.org/scalafmt/#Pre-release I updated the milestone deadline for v1.5.0 to April 16th https://github.com/scalameta/scalafmt/milestones The IntelliJ plugin is a pain to publish so it's only updated for stable releases.

@japgolly

This comment has been minimized.

Contributor

japgolly commented Mar 19, 2018

Awesome and thanks a lot for the help @olafurpg!

@olafurpg

This comment has been minimized.

Member

olafurpg commented Apr 23, 2018

v1.5 is out now! http://scalameta.org/scalafmt/#1.5.0

I ran the rewrite on the scalafmt repo and hit a bug where a non-redundant curly brace was removed. I opened a ticket #1147 would you be interested in taking a look @japgolly ?

I suspect it might be caused by the SyntacticGroups implementation, so I'm happy to look into it as well.

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