Reverse null-check branch order in Optional operations (#2970)

zio · Dec 10, 2023 · dd028a7 · dd028a7
1 parent 2e8a6d5
commit dd028a7
Show file tree

Hide file tree

Showing 23 changed files with 108 additions and 209 deletions.
diff --git a/.scalafmt.conf b/.scalafmt.conf
@@ -1,19 +1,26 @@
 version = "3.7.17"
-maxColumn = 120
+maxColumn = 240
 align.preset = most
 align.multiline = false
 continuationIndent.defnSite = 2
 assumeStandardLibraryStripMargin = true
 docstrings.style = Asterisk
 docstrings.wrapMaxColumn = 80
 lineEndings = preserve
-includeCurlyBraceInSelectChains = false
 danglingParentheses.preset = true
 optIn.annotationNewlines = true
 newlines.alwaysBeforeMultilineDef = false
 runner.dialect = scala213
 rewrite.rules = [RedundantBraces]
 
+# If I've inserted extra newlines I know what I'm doing, don't wrap them back.
+newlines.source = keep
+
+# Don't change braces in one-liners to parens e.g. don't change this: `test("foo") { assertEquals(x,y) }`
+# to this `test("foo")(assertEquals(x,y))`. The `rewrite.rules = [RedundantBraces]` will introduce this behavior
+# unless you add the below option.
+rewrite.redundantBraces.parensForOneLineApply = false
+
 project.excludePaths = ["glob:**/scalafix/input/**", "glob:**/scalafix/output/**"]
 
 rewrite.redundantBraces.generalExpressions = false

diff --git a/build/setup_db_scripts.sh b/build/setup_db_scripts.sh
@@ -34,6 +34,12 @@ function setup_sqlite() {
    DB_FILE=quill-jdbc-monix/quill_test.db
    rm -f $DB_FILE
    sqlite3 $DB_FILE < $SQLITE_SCRIPT
+   chmod a+rw $DB_FILE
+
+   # DB File in quill-jdbc-monix
+   DB_FILE=quill-jdbc-test-sqlite/quill_test.db
+   rm -f $DB_FILE
+   sqlite3 $DB_FILE < $SQLITE_SCRIPT
    chmod a+rw $DB_FILE
 
     echo "Sqlite ready!"

diff --git a/...ssandra-pekko/src/test/scala/io/getquill/context/cassandra/pekko/CassandraPekkoSpec.scala b/...ssandra-pekko/src/test/scala/io/getquill/context/cassandra/pekko/CassandraPekkoSpec.scala
@@ -3,10 +3,7 @@ package io.getquill.context.cassandra.pekko
 import org.apache.pekko.actor.ActorSystem
 import org.apache.pekko.stream.Materializer
 import org.apache.pekko.stream.connectors.cassandra.CassandraSessionSettings
-import org.apache.pekko.stream.connectors.cassandra.scaladsl.{
-  CassandraSessionRegistry,
-  CassandraSession => CassandraPekkoSession
-}
+import org.apache.pekko.stream.connectors.cassandra.scaladsl.{CassandraSessionRegistry, CassandraSession => CassandraPekkoSession}
 import org.apache.pekko.testkit.TestKit
 import io.getquill.base.Spec
 import io.getquill.context.cassandra.CassandraTestEntities

diff --git a/quill-cassandra/src/main/scala/io/getquill/context/cassandra/encoding/CassandraTypes.scala b/quill-cassandra/src/main/scala/io/getquill/context/cassandra/encoding/CassandraTypes.scala
@@ -1,14 +1,6 @@
 package io.getquill.context.cassandra.encoding
 
-import java.lang.{
-  Boolean => JBoolean,
-  Byte => JByte,
-  Double => JDouble,
-  Float => JFloat,
-  Integer => JInt,
-  Long => JLong,
-  Short => JShort
-}
+import java.lang.{Boolean => JBoolean, Byte => JByte, Double => JDouble, Float => JFloat, Integer => JInt, Long => JLong, Short => JShort}
 import java.math.{BigDecimal => JBigDecimal}
 import java.nio.ByteBuffer
 import java.util.{Date, UUID}

diff --git a/quill-core/src/test/scala/io/getquill/norm/FlattenOptionOperationSpec.scala b/quill-core/src/test/scala/io/getquill/norm/FlattenOptionOperationSpec.scala
@@ -1,14 +1,14 @@
 package io.getquill.norm
 
-import io.getquill.ast._
+import io.getquill.ast.{+&&+, _}
 import io.getquill.MirrorContexts.testContext._
 import io.getquill.ast.Implicits._
 import io.getquill.norm.ConcatBehavior.{AnsiConcat, NonAnsiConcat}
 import io.getquill.MoreAstOps._
 import io.getquill.base.Spec
 import io.getquill.util.TraceConfig
 
-class FlattenOptionOperationSpec extends Spec { // hello
+class FlattenOptionOperationSpec extends Spec {
 
   def o      = Ident("o")
   def c1     = Constant.auto(1)
@@ -156,51 +156,51 @@ class FlattenOptionOperationSpec extends Spec { // hello
       }
       new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(
         q.ast.body: Ast
-      ).toString mustEqual "((o != null) && (o < 1)) || ((o == null) && true)"
+      ).toString mustEqual "((o < 1) && (o != null)) || (true && (o == null))"
     }
     "map + getOrElse(false)" in {
       val q = quote { (o: Option[Int]) =>
         o.map(_ < 1).getOrElse(false)
       }
       new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(
         q.ast.body: Ast
-      ).toString mustEqual "((o != null) && (o < 1)) || ((o == null) && false)"
+      ).toString mustEqual "((o < 1) && (o != null)) || (false && (o == null))"
     }
     "forall" - {
       "regular operation" in {
         val q = quote { (o: Option[Int]) =>
           o.forall(i => i != 1)
         }
         new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          (IsNullCheck(o) +||+ (o +!=+ c1))
+          ((o +!=+ c1) +||+ IsNullCheck(o))
         new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          (IsNullCheck(o) +||+ (o +!=+ c1))
+          ((o +!=+ c1) +||+ IsNullCheck(o))
       }
       "possible-fallthrough operation" in {
         val q = quote { (o: Option[String]) =>
           o.forall(s => s + "foo" == "bar")
         }
         new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          (IsNullCheck(o) +||+ ((o +++ cFoo) +==+ cBar))
+          (((o +++ cFoo) +==+ cBar) +||+ IsNullCheck(o))
         new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          (IsNullCheck(o) +||+ (IsNotNullCheck(o) +&&+ ((o +++ cFoo) +==+ cBar)))
+          ((((o +++ cFoo) +==+ cBar) +&&+ IsNotNullCheck(o)) +||+ IsNullCheck(o))
       }
       "never-fallthrough operation" in {
         val q = quote { (o: Option[String]) =>
           o.forall(s => if (s + "foo" == "bar") true else false)
         }
         new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          (IsNullCheck(o) +||+ (IsNotNullCheck(o) +&&+ If(
+          ((If(
             (o +++ cFoo) +==+ cBar,
             Constant.auto(true),
             Constant.auto(false)
-          )))
+          ) +&&+ IsNotNullCheck(o)) +||+ IsNullCheck(o))
         new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          (IsNullCheck(o) +||+ (IsNotNullCheck(o) +&&+ If(
+          ((If(
             (o +++ cFoo) +==+ cBar,
             Constant.auto(true),
             Constant.auto(false)
-          )))
+          ) +&&+ IsNotNullCheck(o)) +||+ IsNullCheck(o))
       }
     }
     "map + forall + binop" - {
@@ -209,19 +209,16 @@ class FlattenOptionOperationSpec extends Spec { // hello
           o.map(_.i).forall(i => i != 1) && true
         }
         new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          ((IsNullCheck(Property(o, "i")) +||+ ((Property(o, "i") +!=+ c1))) +&&+ Constant.auto(true))
+          ((((Property(o, "i") +!=+ c1)) +||+ IsNullCheck(Property(o, "i"))) +&&+ Constant.auto(true))
       }
       "possible-fallthrough operation" in {
         val q = quote { (o: Option[TestEntity2]) =>
           o.map(_.s).forall(s => s + "foo" == "bar") && true
         }
         new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          ((IsNullCheck(Property(o, "s")) +||+ ((Property(o, "s") +++ cFoo) +==+ cBar) +&&+ Constant.auto(true)))
+          ((((Property(o, "s") +++ cFoo) +==+ cBar) +||+ IsNullCheck(Property(o, "s")) +&&+ Constant.auto(true)))
         new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          ((IsNullCheck(Property(o, "s")) +||+ (IsNotNullCheck(Property(o, "s")) +&&+ ((Property(
-            o,
-            "s"
-          ) +++ cFoo) +==+ cBar))) +&&+ Constant.auto(true))
+          (((((Property(o, "s") +++ cFoo) +==+ cBar) +&&+ IsNotNullCheck(Property(o, "s"))) +||+ IsNullCheck(Property(o, "s"))) +&&+ Constant.auto(true))
       }
     }
     "exists" - {
@@ -241,16 +238,16 @@ class FlattenOptionOperationSpec extends Spec { // hello
         new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
           ((o +++ cFoo) +==+ cBar)
         new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          (IsNotNullCheck(o) +&&+ ((o +++ cFoo) +==+ cBar))
+          (((o +++ cFoo) +==+ cBar) +&&+ IsNotNullCheck(o))
       }
       "never-fallthrough operation" in {
         val q = quote { (o: Option[String]) =>
           o.exists(s => if (s + "foo" == "bar") true else false)
         }
         new FlattenOptionOperation(AnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          (IsNotNullCheck(o) +&&+ If((o +++ cFoo) +==+ cBar, Constant.auto(true), Constant.auto(false)))
+          (If((o +++ cFoo) +==+ cBar, Constant.auto(true), Constant.auto(false)) +&&+ IsNotNullCheck(o))
         new FlattenOptionOperation(NonAnsiConcat, TraceConfig.Empty)(q.ast.body: Ast) mustEqual
-          (IsNotNullCheck(o) +&&+ If((o +++ cFoo) +==+ cBar, Constant.auto(true), Constant.auto(false)))
+          (If((o +++ cFoo) +==+ cBar, Constant.auto(true), Constant.auto(false)) +&&+ IsNotNullCheck(o))
       }
     }
     "exists row" in {

diff --git a/quill-engine/src/main/scala/io/getquill/norm/FlattenOptionOperation.scala b/quill-engine/src/main/scala/io/getquill/norm/FlattenOptionOperation.scala
@@ -34,11 +34,11 @@ class FlattenOptionOperation(concatBehavior: ConcatBehavior, traceConfig: TraceC
       validateBody(body),
       () => {
         val reduction = BetaReduction(body, alias -> ast)
-        apply((IsNullCheck(ast) +||+ (IsNotNullCheck(ast) +&&+ reduction)): Ast)
+        apply(((reduction +&&+ IsNotNullCheck(ast)) +||+ IsNullCheck(ast)): Ast)
       },
       () => {
         val reduced = BetaReduction(body, alias -> ast)
-        apply((IsNullCheck(ast) +||+ reduced): Ast)
+        apply((reduced +||+ IsNullCheck(ast)): Ast)
       }
     )
 
@@ -81,7 +81,7 @@ class FlattenOptionOperation(concatBehavior: ConcatBehavior, traceConfig: TraceC
 
       case OptionGetOrElse(HasBooleanQuat(OptionMap(ast, alias, body)), HasBooleanQuat(alternative)) =>
         val expr        = BetaReduction(body, alias -> ast)
-        val output: Ast = (IsNotNullCheck(ast) +&&+ expr) +||+ (IsNullCheck(ast) +&&+ alternative)
+        val output: Ast = (expr +&&+ IsNotNullCheck(ast)) +||+ (alternative +&&+ IsNullCheck(ast))
         apply(output)
 
       case OptionGetOrElse(ast, body) =>
@@ -96,6 +96,14 @@ class FlattenOptionOperation(concatBehavior: ConcatBehavior, traceConfig: TraceC
       case OptionMap(ast, alias, body) =>
         uncheckedReduction(ast, alias, body, containsNonFallthroughElement)
 
+      // a.orElse(b).forAll(alias => body) becomes:
+      //    body(->a) || a==null && body(->b) || a==null && b==null
+      //
+      // Note that since all of the clauses are boolean this a.orElse(...) can be replaced
+      // by a||(b && a==null). If the clause was actually returning value (e.g. if(a) foo else bar)
+      // then these kinds of reductions would not be possible.
+      // Leaving the ||a==null clause without reversing for now despite the fact that ==null
+      // clauses shuold generally be the 2nd in the order because of the <> issue.
       case OptionForall(OptionOrElse(a, b), alias, body) =>
         val reducedA = BetaReduction(body, alias -> a)
         val reducedB = BetaReduction(body, alias -> b)
@@ -117,7 +125,7 @@ class FlattenOptionOperation(concatBehavior: ConcatBehavior, traceConfig: TraceC
           containsNonFallthroughElement(body),
           () => {
             val reduction = BetaReduction(body, alias -> ast)
-            apply(IsNotNullCheck(ast) +&&+ reduction: Ast)
+            apply(reduction +&&+ IsNotNullCheck(ast): Ast)
           },
           () => apply(BetaReduction(body, alias -> ast))
         )

diff --git a/quill-engine/src/main/scala/io/getquill/norm/SheathLeafClauses.scala b/quill-engine/src/main/scala/io/getquill/norm/SheathLeafClauses.scala
@@ -1,24 +1,6 @@
 package io.getquill.norm
 
-import io.getquill.ast.{
-  Aggregation,
-  Ast,
-  CaseClass,
-  ConcatMap,
-  Distinct,
-  Filter,
-  FlatMap,
-  GroupBy,
-  GroupByMap,
-  Ident,
-  Join,
-  Map,
-  Property,
-  Query,
-  StatefulTransformerWithStack,
-  Union,
-  UnionAll
-}
+import io.getquill.ast.{Aggregation, Ast, CaseClass, ConcatMap, Distinct, Filter, FlatMap, GroupBy, GroupByMap, Ident, Join, Map, Property, Query, StatefulTransformerWithStack, Union, UnionAll}
 import io.getquill.ast.Ast.LeafQuat
 import io.getquill.ast.StatefulTransformerWithStack.History
 import io.getquill.util.{Interpolator, TraceConfig}

diff --git a/quill-engine/src/main/scala/io/getquill/norm/capture/AvoidAliasConflict.scala b/quill-engine/src/main/scala/io/getquill/norm/capture/AvoidAliasConflict.scala
@@ -1,19 +1,6 @@
 package io.getquill.norm.capture
 
-import io.getquill.ast.{
-  Entity,
-  Filter,
-  FlatJoin,
-  FlatMap,
-  GroupBy,
-  Ident,
-  Join,
-  Map,
-  Query,
-  SortBy,
-  StatefulTransformer,
-  _
-}
+import io.getquill.ast.{Entity, Filter, FlatJoin, FlatMap, GroupBy, Ident, Join, Map, Query, SortBy, StatefulTransformer, _}
 import io.getquill.ast.Implicits._
 import io.getquill.norm.{BetaReduction, Normalize}
 import io.getquill.util.{Interpolator, TraceConfig}

diff --git a/quill-engine/src/main/scala/io/getquill/sql/idiom/SqlIdiom.scala b/quill-engine/src/main/scala/io/getquill/sql/idiom/SqlIdiom.scala
@@ -17,12 +17,7 @@ import io.getquill.norm.ConcatBehavior.AnsiConcat
 import io.getquill.norm.EqualityBehavior.AnsiEquality
 import io.getquill.norm.{ConcatBehavior, EqualityBehavior, ExpandReturning, NormalizeCaching, ProductAggregationToken}
 import io.getquill.quat.Quat
-import io.getquill.sql.norm.{
-  HideTopLevelFilterAlias,
-  NormalizeFilteredActionAliases,
-  RemoveExtraAlias,
-  RemoveUnusedSelects
-}
+import io.getquill.sql.norm.{HideTopLevelFilterAlias, NormalizeFilteredActionAliases, RemoveExtraAlias, RemoveUnusedSelects}
 import io.getquill.util.{Interleave, Interpolator, Messages, TraceConfig}
 import io.getquill.util.Messages.{TraceType, fail, trace}
 

diff --git a/quill-engine/src/main/scala/io/getquill/sql/norm/RemoveUnusedSelects.scala b/quill-engine/src/main/scala/io/getquill/sql/norm/RemoveUnusedSelects.scala
@@ -1,20 +1,7 @@
 package io.getquill.sql.norm
 
 import io.getquill.ast.{Ast, CollectAst, Ident, Property, StatefulTransformer}
-import io.getquill.context.sql.{
-  DistinctKind,
-  FlatJoinContext,
-  FlattenSqlQuery,
-  FromContext,
-  InfixContext,
-  JoinContext,
-  QueryContext,
-  SelectValue,
-  SetOperationSqlQuery,
-  SqlQuery,
-  TableContext,
-  UnaryOperationSqlQuery
-}
+import io.getquill.context.sql.{DistinctKind, FlatJoinContext, FlattenSqlQuery, FromContext, InfixContext, JoinContext, QueryContext, SelectValue, SetOperationSqlQuery, SqlQuery, TableContext, UnaryOperationSqlQuery}
 import io.getquill.norm.PropertyMatryoshka
 import io.getquill.quat.Quat
 

diff --git a/quill-engine/src/main/scala/io/getquill/sql/norm/StatelessQueryTransformer.scala b/quill-engine/src/main/scala/io/getquill/sql/norm/StatelessQueryTransformer.scala
@@ -1,17 +1,6 @@
 package io.getquill.sql.norm
 
-import io.getquill.context.sql.{
-  FlatJoinContext,
-  FlattenSqlQuery,
-  FromContext,
-  InfixContext,
-  JoinContext,
-  QueryContext,
-  SetOperationSqlQuery,
-  SqlQuery,
-  TableContext,
-  UnaryOperationSqlQuery
-}
+import io.getquill.context.sql.{FlatJoinContext, FlattenSqlQuery, FromContext, InfixContext, JoinContext, QueryContext, SetOperationSqlQuery, SqlQuery, TableContext, UnaryOperationSqlQuery}
 import io.getquill.quat.Quat
 
 sealed trait QueryLevel {

diff --git a/quill-jdbc-test-sqlite/quill_test.db b/quill-jdbc-test-sqlite/quill_test.db
diff --git a/quill-jdbc-zio/src/main/scala/io/getquill/ZioJdbcContexts.scala b/quill-jdbc-zio/src/main/scala/io/getquill/ZioJdbcContexts.scala
@@ -1,15 +1,7 @@
 package io.getquill
 
 import com.typesafe.config.Config
-import io.getquill.context.jdbc.{
-  H2JdbcTypes,
-  MysqlJdbcTypes,
-  OracleJdbcTypes,
-  PostgresJdbcTypes,
-  SqlServerExecuteOverride,
-  SqlServerJdbcTypes,
-  SqliteJdbcTypes
-}
+import io.getquill.context.jdbc.{H2JdbcTypes, MysqlJdbcTypes, OracleJdbcTypes, PostgresJdbcTypes, SqlServerExecuteOverride, SqlServerJdbcTypes, SqliteJdbcTypes}
 import io.getquill.context.sql.idiom.SqlIdiom
 import io.getquill.context.json.PostgresJsonExtensions
 import io.getquill.context.qzio.{ZioJdbcContext, ZioJdbcUnderlyingContext}

diff --git a/quill-spark/src/main/scala/io/getquill/context/spark/SparkDialect.scala b/quill-spark/src/main/scala/io/getquill/context/spark/SparkDialect.scala
@@ -1,29 +1,9 @@
 package io.getquill.context.spark
 
 import io.getquill.{IdiomContext, NamingStrategy}
-import io.getquill.ast.{
-  Ast,
-  BinaryOperation,
-  CaseClass,
-  Constant,
-  ExternalIdent,
-  Ident,
-  Operation,
-  Property,
-  Query,
-  StringOperator,
-  Tuple,
-  Value
-}
+import io.getquill.ast.{Ast, BinaryOperation, CaseClass, Constant, ExternalIdent, Ident, Operation, Property, Query, StringOperator, Tuple, Value}
 import io.getquill.context.spark.norm.EscapeQuestionMarks
-import io.getquill.context.sql.{
-  FlattenSqlQuery,
-  SelectValue,
-  SetOperationSqlQuery,
-  SqlQuery,
-  SqlQueryApply,
-  UnaryOperationSqlQuery
-}
+import io.getquill.context.sql.{FlattenSqlQuery, SelectValue, SetOperationSqlQuery, SqlQuery, SqlQueryApply, UnaryOperationSqlQuery}
 import io.getquill.context.sql.idiom.SqlIdiom
 import io.getquill.context.sql.norm.SqlNormalize
 import io.getquill.idiom.StatementInterpolator._