From f278a5039cbdb4b00f800f3b2571496c3ca89a15 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] 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 inject states into the priority queue with a generation value and increment it 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 | 18 +++--- .../src/test/resources/default/Lambda.stat | 26 ++++++++- .../newlines/afterCurlyLambdaNever.stat | 16 +++++- ...SingleArgParenLambdaParamsF_danglingT.stat | 56 ++++++++++++++++++- .../VerticalMultilineDefnSite.stat | 12 +++- 5 files changed, 113 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 5e3da43431..6d2c60b072 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 @@ -139,12 +139,13 @@ class BestFirstSearch( depth: Int = 0, maxCost: Int = Integer.MAX_VALUE ): State = { - val Q = new mutable.PriorityQueue[State]() + val Q = new mutable.PriorityQueue[(Int, State)]() var result = start - Q += start + Q += 0 -> start + // TODO(olafur) this while loop is waaaaaaaaaaaaay tooo big. while (Q.nonEmpty) { - val curr = Q.dequeue() + var (gen, curr) = Q.dequeue() explored += 1 runner.event(Explored(explored, depth, Q.size)) if (explored > runner.maxStateVisits) @@ -171,13 +172,14 @@ class BestFirstSearch( dequeueOnNewStatements && dequeueSpots.contains(tokenHash) && (depth > 0 || !isInsideNoOptZone(splitToken))) - Q.clear() + if (depth == 0) gen += 1 + else Q.clear() } val blockClose = shouldRecurseOnBlock(splitToken, stop, style) if (blockClose.nonEmpty) shortestPathMemo(curr, blockClose.get, depth + 1, maxCost) - .foreach(Q.enqueue(_)) + .foreach(x => Q.enqueue(gen -> x)) else if (escapeInPathologicalCases && visits(curr.depth) > maxVisitsPerToken) { Q.clear() @@ -219,17 +221,17 @@ class BestFirstSearch( if (hasReachedEof(nextNextState, token)) { optimalNotFound = false // logger.elem(split, splitToken, formatWriter.mkString(nextNextState.splits), tokens(nextNextState.splits.length)) - Q.enqueue(nextNextState) + Q.enqueue(gen -> nextNextState) } else if (!killOnFail && nextState.cost - curr.cost <= maxCost) { // TODO(olafur) DRY. This solution can still be optimal. // logger.elem(split, splitToken) - Q.enqueue(nextState) + Q.enqueue(gen -> nextState) } // else kill branch case _ if optimalNotFound && nextState.cost - curr.cost <= maxCost => - Q.enqueue(nextState) + Q.enqueue(gen -> nextState) case _ => // Kill branch. } } 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) + } + +}