From cd24e91e46633f590a8ec7d61aa1d96f7e520fb6 Mon Sep 17 00:00:00 2001 From: Albert Meltzer <7529386+kitbellew@users.noreply.github.com> Date: Mon, 17 Feb 2020 08:51:18 -0800 Subject: [PATCH 1/4] Add failing "unable to format due to bug" tests See #448, #1462, #1485, #1599 and #1717. --- .../src/test/resources/default/Lambda.stat | 37 ++++++++++++++++++ .../newlines/afterCurlyLambdaNever.stat | 20 ++++++++++ ...SingleArgParenLambdaParamsF_danglingT.stat | 39 +++++++++++++++++++ .../VerticalMultilineDefnSite.stat | 15 +++++++ 4 files changed, 111 insertions(+) diff --git a/scalafmt-tests/src/test/resources/default/Lambda.stat b/scalafmt-tests/src/test/resources/default/Lambda.stat index e145abc393..7385f59398 100644 --- a/scalafmt-tests/src/test/resources/default/Lambda.stat +++ b/scalafmt-tests/src/test/resources/default/Lambda.stat @@ -386,3 +386,40 @@ testAsync("Resource.make is equivalent to a partially applied bracket") { implic } } } +<<< #448 +object Foo { + code.collect { + case d: Defn.Def if (d match { + case _ => false + + + }) && + d=> + } +} +>>> +Unable to format file due to bug in scalafmt +<<< #1485 +style = IntelliJ +maxColumn = 120 +=== +class A { + def foo: B = + a.b() { c => + val d = c.add( + D[E]( + 1, { + case conditionNameShouldLongEnough => + foo1(objectNameShouldLongEnough, objectNameShouldLongEnough) + 0 + case conditionNameShouldLongEnough => + foo1(objectNameShouldLongEnough, objectNameShouldLongEnough) + 1 + } + ) + ) + } + .f("f") +} +>>> +Unable to format file due to bug in scalafmt diff --git a/scalafmt-tests/src/test/resources/newlines/afterCurlyLambdaNever.stat b/scalafmt-tests/src/test/resources/newlines/afterCurlyLambdaNever.stat index 8602f2866f..9b4168ec5c 100644 --- a/scalafmt-tests/src/test/resources/newlines/afterCurlyLambdaNever.stat +++ b/scalafmt-tests/src/test/resources/newlines/afterCurlyLambdaNever.stat @@ -37,3 +37,23 @@ def f = { def f = { something.call { x => g(x) } } +<<< #1717 +maxColumn = 100 +=== +class Bar { + def foo(request: Request): RpcResult[Response] = { + repository + .flatMap { campaigns => + request match { + case Nil => connection.pure(Right(toResponse(campaigns, Map.empty))) + case _ => + getProfileEmails(request.securityContext, profileIds).to[ConnectionIO].map { + profileEmails => + Right(toResponse(campaigns, profileEmails)) + } + } + }.transact(managerTransactor) + } +} +>>> +Unable to format file due to bug in scalafmt diff --git a/scalafmt-tests/src/test/resources/newlines/beforeConfigSingleArgParenLambdaParamsF_danglingT.stat b/scalafmt-tests/src/test/resources/newlines/beforeConfigSingleArgParenLambdaParamsF_danglingT.stat index 4d1f58e37d..338ae1e2d3 100644 --- a/scalafmt-tests/src/test/resources/newlines/beforeConfigSingleArgParenLambdaParamsF_danglingT.stat +++ b/scalafmt-tests/src/test/resources/newlines/beforeConfigSingleArgParenLambdaParamsF_danglingT.stat @@ -702,3 +702,42 @@ object a { _ => e ) } +<<< #1599 1: original, with partial func +object A { + def foo() = { + x.map(_.y( + abcd, + { case ((abcdefghij, aswbasdfaw), asfda) => aswdf}, + { case (abcdefghij, sadfasdass) => (asdfa.sadvfs(abcdefghij).get, asdfasdfasfdasda.asdfas(asdfasdaas).get) }, + foo + )) + } +} +>>> +Unable to format file due to bug in scalafmt +<<< #1599 2: modified, with partial func inside block +object A { + def foo() = { + x.map(_.y( + abcd, + {{ case ((abcdefghij, aswbasdfaw), asfda) => aswdf}}, + {{ case (abcdefghij, sadfasdass) => (asdfa.sadvfs(abcdefghij).get, asdfasdfasfdasda.asdfas(asdfasdaas).get) }}, + foo + )) + } +} +>>> +Unable to format file due to bug in scalafmt +<<< #1599 3: modified, with lambda +object A { + def foo() = { + x.map(_.y( + abcd, + { case_abcdefghij_aswbasdfaw_asfda => aswdf}, + { case_abcdefghij_sadfasdass => (asdfa.sadvfs(abcdefghij).get, asdfasdfasfdasda.asdfas(asdfasdaas).get) }, + foo + )) + } +} +>>> +Unable to format file due to bug in scalafmt diff --git a/scalafmt-tests/src/test/resources/vertical-multiline/VerticalMultilineDefnSite.stat b/scalafmt-tests/src/test/resources/vertical-multiline/VerticalMultilineDefnSite.stat index 0ebcc2895d..facc2571d7 100644 --- a/scalafmt-tests/src/test/resources/vertical-multiline/VerticalMultilineDefnSite.stat +++ b/scalafmt-tests/src/test/resources/vertical-multiline/VerticalMultilineDefnSite.stat @@ -14,3 +14,18 @@ def format_![T <: Tree]( )(implicit ev: Parse[T], ev2: EC ): String +<<< #1462 +maxColumn = 91 +=== +object Test { + + def create( + somethingInfo: foo.T = Something("my-something", Some("test-something"), "something", Some(Format("something", "something")), + Some(something.Something("something", "something", "something", "something"))) + ): Unit = { + w(foo) + } + +} +>>> +Unable to format file due to bug in scalafmt From 425cf1652e743da3fa6c082d4e2625001eb00f11 Mon Sep 17 00:00:00 2001 From: Albert Meltzer <7529386+kitbellew@users.noreply.github.com> Date: Fri, 21 Feb 2020 09:00:21 -0800 Subject: [PATCH 2/4] BestFirstSearch: make private, expose one method --- .../src/main/scala/org/scalafmt/Scalafmt.scala | 3 +-- .../org/scalafmt/internal/BestFirstSearch.scala | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/Scalafmt.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/Scalafmt.scala index f40fcf19c3..ef3b709537 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/Scalafmt.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/Scalafmt.scala @@ -69,8 +69,7 @@ object Scalafmt { val formatOps = new FormatOps(tree, style) runner.event(CreateFormatOps(formatOps)) val formatWriter = new FormatWriter(formatOps) - val search = new BestFirstSearch(formatOps, range, formatWriter) - val partial = search.getBestPath + val partial = BestFirstSearch(formatOps, range, formatWriter) val formattedString = formatWriter.mkString(partial.state) val correctedFormattedString = if ((style.lineEndings == preserve && isWindows) || diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala index 5e3da43431..f0df903f9d 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala @@ -18,7 +18,7 @@ import org.scalafmt.util.TreeOps /** * Implements best first search to find optimal formatting. */ -class BestFirstSearch( +private class BestFirstSearch private ( val formatOps: FormatOps, range: Set[Range], formatWriter: FormatWriter @@ -180,8 +180,6 @@ class BestFirstSearch( .foreach(Q.enqueue(_)) else if (escapeInPathologicalCases && visits(curr.depth) > maxVisitsPerToken) { - Q.clear() - best.clear() runner.event(CompleteFormat(explored, deepestYet)) throw SearchStateExploded( deepestYet, @@ -268,3 +266,14 @@ class BestFirstSearch( } case class SearchResult(state: State, reachedEOF: Boolean) + +object BestFirstSearch { + + def apply( + formatOps: FormatOps, + range: Set[Range], + formatWriter: FormatWriter + ): SearchResult = + new BestFirstSearch(formatOps, range, formatWriter).getBestPath + +} From c390889953ee410912bc8e46b66b60a03262c934 Mon Sep 17 00:00:00 2001 From: Albert Meltzer <7529386+kitbellew@users.noreply.github.com> Date: Fri, 21 Feb 2020 09:08:32 -0800 Subject: [PATCH 3/4] BestFirstSearch: null if internal search failed --- .../scalafmt/internal/BestFirstSearch.scala | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala index f0df903f9d..9b3f5213f5 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala @@ -105,12 +105,6 @@ private class BestFirstSearch private ( state.column << 8 | state.indentation } - def hasReachedEof(state: State, stop: Token): Boolean = - state.depth >= tokens.length || { - val token = tokens(state.depth) - token.left.start >= stop.start && token.left.end > token.left.start - } - val memo = mutable.Map.empty[(Int, StateHash), State] def shortestPathMemo(start: State, stop: Token, depth: Int, maxCost: Int)( @@ -122,7 +116,7 @@ private class BestFirstSearch private ( else { // Only update state if it reached stop. val nextState = shortestPath(start, stop, depth, maxCost) - if (tokens(nextState.depth).left != stop) None + if (null == nextState) None else { memo.update(key, nextState) Some(nextState) @@ -140,7 +134,7 @@ private class BestFirstSearch private ( maxCost: Int = Integer.MAX_VALUE ): State = { val Q = new mutable.PriorityQueue[State]() - var result = start + var result: State = null Q += start // TODO(olafur) this while loop is waaaaaaaaaaaaay tooo big. while (Q.nonEmpty) { @@ -153,11 +147,14 @@ private class BestFirstSearch private ( formatWriter.mkString(deepestYet), tokens(deepestYet.depth).left ) - if (hasReachedEof(curr, stop)) { + val splitToken = + if (curr.depth >= tokens.length) null else tokens(curr.depth) + if (null == splitToken || + splitToken.left.start >= stop.start && + splitToken.left.start < splitToken.left.end) { result = curr Q.clear() } else if (shouldEnterState(curr)) { - val splitToken = tokens(curr.depth) val style = styleMap.at(splitToken) if (curr.depth > deepestYet.depth) { deepestYet = curr @@ -214,7 +211,7 @@ private class BestFirstSearch private ( nextState.split.cost == 0 => val nextNextState = shortestPath(nextState, token, depth + 1, maxCost = 0) - if (hasReachedEof(nextNextState, token)) { + if (null != nextNextState) { optimalNotFound = false // logger.elem(split, splitToken, formatWriter.mkString(nextNextState.splits), tokens(nextNextState.splits.length)) Q.enqueue(nextNextState) @@ -239,7 +236,7 @@ private class BestFirstSearch private ( def getBestPath: SearchResult = { val state = shortestPath(State.start, tree.tokens.last) - if (state.depth == tokens.length) { + if (null != state) { runner.event(CompleteFormat(explored, state)) SearchResult(state, reachedEOF = true) } else { @@ -249,7 +246,6 @@ private class BestFirstSearch private ( deepestYet.policy.execute(Decision(tok, nextSplits)) val msg = s"""UNABLE TO FORMAT, |tok=$tok - |state.depth=${state.depth} |toks.length=${tokens.length} |deepestYet.length=${deepestYet.depth} |policies=${deepestYet.policy.policies} From b59dd4bedede3842397b842f11da931261649b1b Mon Sep 17 00:00:00 2001 From: Albert Meltzer <7529386+kitbellew@users.noreply.github.com> Date: Fri, 21 Feb 2020 00:34:34 -0800 Subject: [PATCH 4/4] BestFirstSearch: don't give up too soon Currently, the algorithm would clear the search priority queue when the top candidate points to a new statement. Sometimes this leads to losing a viable path and the dreaded "Unable to format due to bug" error. Rather than clearing the queue, let's maintain several generations of priority queues and create a new generation on a new statement, thus continuing to process subsequent states just like the old algorithm did but with the previously discarded states available as backup. Apply this modification to top-level invocations only, so that nested calls will still terminate early; thus the backup candidates are really processed at the very end of the existing logic. Fixes #448. Fixes #1462. Fixes #1485. Fixes #1599. Fixes #1717. --- .../scalafmt/internal/BestFirstSearch.scala | 35 +++++++++--- .../src/test/resources/default/Lambda.stat | 26 ++++++++- .../newlines/afterCurlyLambdaNever.stat | 16 +++++- ...SingleArgParenLambdaParamsF_danglingT.stat | 56 ++++++++++++++++++- .../VerticalMultilineDefnSite.stat | 12 +++- 5 files changed, 130 insertions(+), 15 deletions(-) diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala index 9b3f5213f5..f28320ae69 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala @@ -133,11 +133,18 @@ private class BestFirstSearch private ( depth: Int = 0, maxCost: Int = Integer.MAX_VALUE ): State = { - val Q = new mutable.PriorityQueue[State]() - var result: State = null + def newGeneration = new mutable.PriorityQueue[State]() + var Q = newGeneration + var generations: List[mutable.PriorityQueue[State]] = Nil + def addGeneration = + if (Q.nonEmpty) { + generations = Q :: generations + Q = newGeneration + } Q += start + // TODO(olafur) this while loop is waaaaaaaaaaaaay tooo big. - while (Q.nonEmpty) { + while (true) { val curr = Q.dequeue() explored += 1 runner.event(Explored(explored, depth, Q.size)) @@ -152,9 +159,10 @@ private class BestFirstSearch private ( if (null == splitToken || splitToken.left.start >= stop.start && splitToken.left.start < splitToken.left.end) { - result = curr - Q.clear() - } else if (shouldEnterState(curr)) { + return curr + } + + if (shouldEnterState(curr)) { val style = styleMap.at(splitToken) if (curr.depth > deepestYet.depth) { deepestYet = curr @@ -168,7 +176,8 @@ private class BestFirstSearch private ( dequeueOnNewStatements && dequeueSpots.contains(tokenHash) && (depth > 0 || !isInsideNoOptZone(splitToken))) - Q.clear() + if (depth == 0) addGeneration + else Q.clear() } val blockClose = shouldRecurseOnBlock(splitToken, stop, style) @@ -230,8 +239,18 @@ private class BestFirstSearch private ( } } } + + if (Q.isEmpty) { + if (generations.isEmpty) + return null + + Q = generations.head + generations = generations.tail + } } - result + + // unreachable + null } def getBestPath: SearchResult = { diff --git a/scalafmt-tests/src/test/resources/default/Lambda.stat b/scalafmt-tests/src/test/resources/default/Lambda.stat index 7385f59398..4f055d03ee 100644 --- a/scalafmt-tests/src/test/resources/default/Lambda.stat +++ b/scalafmt-tests/src/test/resources/default/Lambda.stat @@ -398,7 +398,16 @@ object Foo { } } >>> -Unable to format file due to bug in scalafmt +object Foo { + code.collect { + case d: Defn.Def + if (d match { + case _ => false + + }) && + d => + } +} <<< #1485 style = IntelliJ maxColumn = 120 @@ -422,4 +431,17 @@ class A { .f("f") } >>> -Unable to format file due to bug in scalafmt +class A { + def foo: B = + a.b() { c => + val d = c.add(D[E](1, { + case conditionNameShouldLongEnough => + foo1(objectNameShouldLongEnough, objectNameShouldLongEnough) + 0 + case conditionNameShouldLongEnough => + foo1(objectNameShouldLongEnough, objectNameShouldLongEnough) + 1 + })) + } + .f("f") +} diff --git a/scalafmt-tests/src/test/resources/newlines/afterCurlyLambdaNever.stat b/scalafmt-tests/src/test/resources/newlines/afterCurlyLambdaNever.stat index 9b4168ec5c..5fcf5e844f 100644 --- a/scalafmt-tests/src/test/resources/newlines/afterCurlyLambdaNever.stat +++ b/scalafmt-tests/src/test/resources/newlines/afterCurlyLambdaNever.stat @@ -56,4 +56,18 @@ class Bar { } } >>> -Unable to format file due to bug in scalafmt +class Bar { + def foo(request: Request): RpcResult[Response] = { + repository + .flatMap { campaigns => + request match { + case Nil => connection.pure(Right(toResponse(campaigns, Map.empty))) + case _ => + getProfileEmails(request.securityContext, profileIds).to[ConnectionIO].map { + profileEmails => Right(toResponse(campaigns, profileEmails)) + } + } + } + .transact(managerTransactor) + } +} diff --git a/scalafmt-tests/src/test/resources/newlines/beforeConfigSingleArgParenLambdaParamsF_danglingT.stat b/scalafmt-tests/src/test/resources/newlines/beforeConfigSingleArgParenLambdaParamsF_danglingT.stat index 338ae1e2d3..7ba3021c6a 100644 --- a/scalafmt-tests/src/test/resources/newlines/beforeConfigSingleArgParenLambdaParamsF_danglingT.stat +++ b/scalafmt-tests/src/test/resources/newlines/beforeConfigSingleArgParenLambdaParamsF_danglingT.stat @@ -714,7 +714,23 @@ object A { } } >>> -Unable to format file due to bug in scalafmt +object A { + def foo() = { + x.map( + _.y( + abcd, + { case ((abcdefghij, aswbasdfaw), asfda) => aswdf }, { + case (abcdefghij, sadfasdass) => + ( + asdfa.sadvfs(abcdefghij).get, + asdfasdfasfdasda.asdfas(asdfasdaas).get + ) + }, + foo + ) + ) + } +} <<< #1599 2: modified, with partial func inside block object A { def foo() = { @@ -727,7 +743,25 @@ object A { } } >>> -Unable to format file due to bug in scalafmt +object A { + def foo() = { + x.map( + _.y( + abcd, + { { case ((abcdefghij, aswbasdfaw), asfda) => aswdf } }, { + { + case (abcdefghij, sadfasdass) => + ( + asdfa.sadvfs(abcdefghij).get, + asdfasdfasfdasda.asdfas(asdfasdaas).get + ) + } + }, + foo + ) + ) + } +} <<< #1599 3: modified, with lambda object A { def foo() = { @@ -740,4 +774,20 @@ object A { } } >>> -Unable to format file due to bug in scalafmt +object A { + def foo() = { + x.map( + _.y( + abcd, + { case_abcdefghij_aswbasdfaw_asfda => aswdf }, { + case_abcdefghij_sadfasdass => + ( + asdfa.sadvfs(abcdefghij).get, + asdfasdfasfdasda.asdfas(asdfasdaas).get + ) + }, + foo + ) + ) + } +} diff --git a/scalafmt-tests/src/test/resources/vertical-multiline/VerticalMultilineDefnSite.stat b/scalafmt-tests/src/test/resources/vertical-multiline/VerticalMultilineDefnSite.stat index facc2571d7..39d1140f28 100644 --- a/scalafmt-tests/src/test/resources/vertical-multiline/VerticalMultilineDefnSite.stat +++ b/scalafmt-tests/src/test/resources/vertical-multiline/VerticalMultilineDefnSite.stat @@ -28,4 +28,14 @@ object Test { } >>> -Unable to format file due to bug in scalafmt +object Test { + + def create( + somethingInfo: foo.T = Something("my-something", Some("test-something"), "something", + Some(Format("something", "something")), Some(something.Something("something", + "something", "something", "something"))) + ): Unit = { + w(foo) + } + +}