Don't write all parentheses (compare precedence, #231)

moor/CHANGELOG.md

@@ -8,6 +8,7 @@
 - Allow transactions inside a `beforeOpen` callback
 - New `batch` method on generated databases to execute multiple queries in a single batch
 - Experimental support to run moor on a background isolate
+- Reduce use of parentheses in SQL code generated at runtime
 
 ## 2.0.1
 

moor/lib/src/dsl/columns.dart

@@ -2,7 +2,10 @@ part of 'dsl.dart';
 
 /// Base class for columns in sql. The [T] type refers to the type a value of
 /// this column will have in Dart, [S] is the mapping class from moor.
-abstract class Column<T, S extends SqlType<T>> extends Expression<T, S> {}
+abstract class Column<T, S extends SqlType<T>> extends Expression<T, S> {
+  @override
+  final Precedence precedence = Precedence.primary;
+}
 
 /// A column that stores int values.
 abstract class IntColumn extends Column<int, IntType> {}

moor/lib/src/runtime/query_builder/expressions/algebra.dart

@@ -7,7 +7,8 @@ extension MonoidExpr<DT, ST extends Monoid<DT>> on Expression<DT, ST> {
     assert(other is! Expression<String, StringType>,
         'Used Monoid extension but should have resolved to String extension');
 
-    return _BaseInfixOperator(this, '+', other);
+    return _BaseInfixOperator(this, '+', other,
+        precedence: Precedence.plusMinus);
   }
 }
 
@@ -16,7 +17,8 @@ extension StringMonoidExpr on Expression<String, StringType> {
   /// Performs a string concatenation in sql by appending [other] to `this`.
   Expression<String, StringType> operator +(
       Expression<String, StringType> other) {
-    return _BaseInfixOperator(this, '||', other);
+    return _BaseInfixOperator(this, '||', other,
+        precedence: Precedence.stringConcatenation);
   }
 }
 
@@ -25,7 +27,8 @@ extension ArithmeticExpr<DT, ST extends FullArithmetic<DT>>
     on Expression<DT, ST> {
   /// Performs a subtraction (`this` - [other]) in sql.
   Expression<DT, ST> operator -(Expression<DT, ST> other) {
-    return _BaseInfixOperator(this, '-', other);
+    return _BaseInfixOperator(this, '-', other,
+        precedence: Precedence.plusMinus);
   }
 
   /// Returns the negation of this value.
@@ -35,11 +38,13 @@ extension ArithmeticExpr<DT, ST extends FullArithmetic<DT>>
 
   /// Performs a multiplication (`this` * [other]) in sql.
   Expression<DT, ST> operator *(Expression<DT, ST> other) {
-    return _BaseInfixOperator(this, '*', other);
+    return _BaseInfixOperator(this, '*', other,
+        precedence: Precedence.mulDivide);
   }
 
   /// Performs a division (`this` / [other]) in sql.
   Expression<DT, ST> operator /(Expression<DT, ST> other) {
-    return _BaseInfixOperator(this, '/', other);
+    return _BaseInfixOperator(this, '/', other,
+        precedence: Precedence.mulDivide);
   }
 }

moor/lib/src/runtime/query_builder/expressions/bools.dart

@@ -5,16 +5,18 @@ part of '../query_builder.dart';
 /// This is now deprecated. Instead of `and(a, b)`, use `a & b`.
 @Deprecated('Use the operator on BooleanExpressionOperators instead')
 Expression<bool, BoolType> and(
-        Expression<bool, BoolType> a, Expression<bool, BoolType> b) =>
-    _AndExpression(a, b);
+    Expression<bool, BoolType> a, Expression<bool, BoolType> b) {
+  return a & b;
+}
 
 /// Returns an expression that is true iff [a], [b] or both are true.
 ///
 /// This is now deprecated. Instead of `or(a, b)`, use `a | b`;
 @Deprecated('Use the operator on BooleanExpressionOperators instead')
 Expression<bool, BoolType> or(
-        Expression<bool, BoolType> a, Expression<bool, BoolType> b) =>
-    _OrExpression(a, b);
+    Expression<bool, BoolType> a, Expression<bool, BoolType> b) {
+  return a | b;
+}
 
 /// Returns an expression that is true iff [a] is not true.
 ///
@@ -31,43 +33,26 @@ extension BooleanExpressionOperators on Expression<bool, BoolType> {
 
   /// Returns an expression that is true iff both `this` and [other] are true.
   Expression<bool, BoolType> operator &(Expression<bool, BoolType> other) {
-    return _AndExpression(this, other);
+    return _BaseInfixOperator(this, 'AND', other, precedence: Precedence.and);
   }
 
   /// Returns an expression that is true if `this` or [other] are true.
   Expression<bool, BoolType> operator |(Expression<bool, BoolType> other) {
-    return _OrExpression(this, other);
+    return _BaseInfixOperator(this, 'OR', other, precedence: Precedence.or);
   }
 }
 
-class _AndExpression extends _InfixOperator<bool, BoolType> {
-  @override
-  Expression<bool, BoolType> left, right;
-
-  @override
-  final String operator = 'AND';
-
-  _AndExpression(this.left, this.right);
-}
-
-class _OrExpression extends _InfixOperator<bool, BoolType> {
-  @override
-  Expression<bool, BoolType> left, right;
-
-  @override
-  final String operator = 'OR';
-
-  _OrExpression(this.left, this.right);
-}
-
 class _NotExpression extends Expression<bool, BoolType> {
-  Expression<bool, BoolType> inner;
+  final Expression<bool, BoolType> inner;
 
   _NotExpression(this.inner);
 
+  @override
+  Precedence get precedence => Precedence.unary;
+
   @override
   void writeInto(GenerationContext context) {
     context.buffer.write('NOT ');
-    inner.writeInto(context);
+    writeInner(context, inner);
   }
 }

moor/lib/src/runtime/query_builder/expressions/comparable.dart

@@ -77,6 +77,10 @@ extension ComparableExpr<DT, ST extends ComparableType<DT>>
 class _BetweenExpression extends Expression<bool, BoolType> {
   final Expression target;
 
+  // https://www.sqlite.org/lang_expr.html#between
+  @override
+  final Precedence precedence = Precedence.comparisonEq;
+
   /// Whether to negate this between expression
   final bool not;
 
@@ -91,13 +95,13 @@ class _BetweenExpression extends Expression<bool, BoolType> {
 
   @override
   void writeInto(GenerationContext context) {
-    target.writeInto(context);
+    writeInner(context, target);
 
     if (not) context.buffer.write(' NOT');
     context.buffer.write(' BETWEEN ');
 
-    lower.writeInto(context);
+    writeInner(context, lower);
     context.buffer.write(' AND ');
-    higher.writeInto(context);
+    writeInner(context, higher);
   }
 }

moor/lib/src/runtime/query_builder/expressions/datetimes.dart

@@ -44,6 +44,9 @@ const Expression<DateTime, DateTimeType> currentDateAndTime =
 
 class _CustomDateTimeExpression
     extends CustomExpression<DateTime, DateTimeType> {
+  @override
+  final Precedence precedence = Precedence.primary;
+
   const _CustomDateTimeExpression(String content) : super(content);
 }
 

moor/lib/src/runtime/query_builder/expressions/expression.dart

@@ -7,6 +7,10 @@ abstract class Expression<D, T extends SqlType<D>> implements Component {
   /// Constant constructor so that subclasses can be constant.
   const Expression();
 
+  /// The precedence of this expression. This can be used to automatically put
+  /// parentheses around expressions as needed.
+  Precedence get precedence => Precedence.unknown;
+
   /// Whether this expression is a literal. Some use-sites need to put
   /// parentheses around non-literals.
   bool get isLiteral => false;
@@ -21,6 +25,107 @@ abstract class Expression<D, T extends SqlType<D>> implements Component {
   /// an SQL-injection.
   Expression<bool, BoolType> equals(D compare) =>
       _Comparison.equal(this, Variable<D, T>(compare));
+
+  /// Writes this expression into the [GenerationContext], assuming that there's
+  /// an outer expression with [precedence]. If the [Expression.precedence] of
+  /// `this` expression is lower, it will be wrapped in
+  ///
+  /// See also:
+  ///  - [Component.writeInto], which doesn't take any precedence relation into
+  ///  account.
+  void writeAroundPrecedence(GenerationContext context, Precedence precedence) {
+    if (this.precedence < precedence) {
+      context.buffer.write('(');
+      writeInto(context);
+      context.buffer.write(')');
+    } else {
+      writeInto(context);
+    }
+  }
+
+  /// If this [Expression] wraps an [inner] expression, this utility method can
+  /// be used inside [writeInto] to write that inner expression while wrapping
+  /// it in parentheses if necessary.
+  @protected
+  void writeInner(GenerationContext ctx, Expression inner) {
+    assert(precedence != Precedence.unknown,
+        "Expressions with unknown precedence shouldn't have inner expressions");
+    inner.writeAroundPrecedence(ctx, precedence);
+  }
+}
+
+/// Used to order the precedence of sql expressions so that we can avoid
+/// unnecessary parens when generating sql statements.
+class Precedence implements Comparable<Precedence> {
+  /// Higher means higher precedence.
+  final int _value;
+
+  const Precedence._(this._value);
+
+  @override
+  int compareTo(Precedence other) {
+    return _value.compareTo(other._value);
+  }
+
+  @override
+  int get hashCode => _value;
+
+  @override
+  bool operator ==(other) {
+    // runtimeType comparison isn't necessary, the private constructor prevents
+    // subclasses
+    return other is Precedence && other._value == _value;
+  }
+
+  /// Returns true if this [Precedence] is lower than [other].
+  bool operator <(Precedence other) => compareTo(other) < 0;
+
+  /// Returns true if this [Precedence] is lower or equal to [other].
+  bool operator <=(Precedence other) => compareTo(other) <= 0;
+
+  /// Returns true if this [Precedence] is higher than [other].
+  bool operator >(Precedence other) => compareTo(other) > 0;
+
+  /// Returns true if this [Precedence] is higher or equal to [other].
+  bool operator >=(Precedence other) => compareTo(other) >= 0;
+
+  /// Precedence is unknown, assume lowest. This can be used for a
+  /// [CustomExpression] to always put parens around it.
+  static const Precedence unknown = Precedence._(-1);
+
+  /// Precedence for the `OR` operator in sql
+  static const Precedence or = Precedence._(10);
+
+  /// Precedence for the `AND` operator in sql
+  static const Precedence and = Precedence._(11);
+
+  /// Precedence for most of the comparisons operators in sql, including
+  /// equality, is (not) checks, in, like, glob, match, regexp.
+  static const Precedence comparisonEq = Precedence._(12);
+
+  /// Precedence for the <, <=, >, >= operators in sql
+  static const Precedence comparison = Precedence._(13);
+
+  /// Precedence for bitwise operators in sql
+  static const Precedence bitwise = Precedence._(14);
+
+  /// Precedence for the (binary) plus and minus operators in sql
+  static const Precedence plusMinus = Precedence._(15);
+
+  /// Precedence for the *, / and % operators in sql
+  static const Precedence mulDivide = Precedence._(16);
+
+  /// Precedence for the || operator in sql
+  static const Precedence stringConcatenation = Precedence._(17);
+
+  /// Precedence for unary operators in sql
+  static const Precedence unary = Precedence._(20);
+
+  /// Precedence for postfix operators (like collate) in sql
+  static const Precedence postfix = Precedence._(21);
+
+  /// Highest precedence in sql, used for variables and literals.
+  static const Precedence primary = Precedence._(100);
 }
 
 /// An expression that looks like "$a operator $b", where $a and $b itself
@@ -36,29 +141,13 @@ abstract class _InfixOperator<D, T extends SqlType<D>>
   /// The sql operator to write
   String get operator;
 
-  /// Whether we should put parentheses around the [left] and [right]
-  /// expressions.
-  bool get placeBrackets => true;
-
   @override
   void writeInto(GenerationContext context) {
-    _placeBracketIfNeeded(context, true);
-
-    left.writeInto(context);
-
-    _placeBracketIfNeeded(context, false);
+    writeInner(context, left);
     context.writeWhitespace();
     context.buffer.write(operator);
     context.writeWhitespace();
-    _placeBracketIfNeeded(context, true);
-
-    right.writeInto(context);
-
-    _placeBracketIfNeeded(context, false);
-  }
-
-  void _placeBracketIfNeeded(GenerationContext context, bool open) {
-    if (placeBrackets) context.buffer.write(open ? '(' : ')');
+    writeInner(context, right);
   }
 }
 
@@ -72,7 +161,11 @@ class _BaseInfixOperator<D, T extends SqlType<D>> extends _InfixOperator<D, T> {
   @override
   final Expression<D, T> right;
 
-  _BaseInfixOperator(this.left, this.operator, this.right);
+  @override
+  final Precedence precedence;
+
+  _BaseInfixOperator(this.left, this.operator, this.right,
+      {this.precedence = Precedence.unknown});
 }
 
 /// Defines the possible comparison operators that can appear in a [_Comparison].
@@ -112,10 +205,16 @@ class _Comparison extends _InfixOperator<bool, BoolType> {
   final _ComparisonOperator op;
 
   @override
-  final bool placeBrackets = false;
+  String get operator => _operatorNames[op];
 
   @override
-  String get operator => _operatorNames[op];
+  Precedence get precedence {
+    if (op == _ComparisonOperator.equal) {
+      return Precedence.comparisonEq;
+    } else {
+      return Precedence.comparison;
+    }
+  }
 
   /// Constructs a comparison from the [left] and [right] expressions to compare
   /// and the [ComparisonOperator] [op].
@@ -130,6 +229,9 @@ class _UnaryMinus<DT, ST extends SqlType<DT>> extends Expression<DT, ST> {
 
   _UnaryMinus(this.inner);
 
+  @override
+  Precedence get precedence => Precedence.unary;
+
   @override
   void writeInto(GenerationContext context) {
     context.buffer.write('-');

moor/lib/src/runtime/query_builder/expressions/in.dart

@@ -20,11 +20,14 @@ class _InExpression<X extends SqlType<T>, T>
   final Iterable<T> _values;
   final bool _not;
 
+  @override
+  Precedence get precedence => Precedence.comparisonEq;
+
   _InExpression(this._expression, this._values, this._not);
 
   @override
   void writeInto(GenerationContext context) {
-    _expression.writeInto(context);
+    writeInner(context, _expression);
 
     if (_not) {
       context.buffer.write(' NOT');

moor/lib/src/runtime/query_builder/expressions/null_check.dart

@@ -12,11 +12,14 @@ class _NullCheck extends Expression<bool, BoolType> {
   final Expression _inner;
   final bool _isNull;
 
+  @override
+  final Precedence precedence = Precedence.comparisonEq;
+
   _NullCheck(this._inner, this._isNull);
 
   @override
   void writeInto(GenerationContext context) {
-    _inner.writeInto(context);
+    writeInner(context, _inner);
 
     context.buffer.write(' IS ');
     if (!_isNull) {

moor/lib/src/runtime/query_builder/expressions/text.dart

@@ -25,14 +25,17 @@ class _LikeOperator extends Expression<bool, BoolType> {
   /// The regex-like expression to test the [target] against.
   final Expression<String, StringType> regex;
 
+  @override
+  final Precedence precedence = Precedence.comparisonEq;
+
   /// Perform a like operator with the target and the regex.
   _LikeOperator(this.target, this.regex);
 
   @override
   void writeInto(GenerationContext context) {
-    target.writeInto(context);
+    writeInner(context, target);
     context.buffer.write(' LIKE ');
-    regex.writeInto(context);
+    writeInner(context, regex);
   }
 }
 
@@ -65,13 +68,16 @@ class _CollateOperator extends Expression<String, StringType> {
   /// The [Collate] to use.
   final Collate collate;
 
+  @override
+  final Precedence precedence = Precedence.postfix;
+
   /// Constructs a collate expression on the [inner] expression and the
   /// [Collate].
   _CollateOperator(this.inner, this.collate);
 
   @override
   void writeInto(GenerationContext context) {
-    inner.writeInto(context);
+    writeInner(context, inner);
     context.buffer..write(' COLLATE ')..write(_operatorNames[collate]);
   }
 

moor/lib/src/runtime/query_builder/expressions/variables.dart

@@ -6,6 +6,9 @@ class Variable<T, S extends SqlType<T>> extends Expression<T, S> {
   /// The Dart value that will be sent to the database
   final T value;
 
+  @override
+  final Precedence precedence = Precedence.primary;
+
   /// Constructs a new variable from the [value].
   const Variable(this.value);
 
@@ -65,6 +68,9 @@ class Constant<T, S extends SqlType<T>> extends Expression<T, S> {
   /// Constructs a new constant (sql literal) holding the [value].
   const Constant(this.value);
 
+  @override
+  final Precedence precedence = Precedence.primary;
+
   /// The value that will be converted to an sql literal.
   final T value;
 

moor/lib/src/runtime/query_builder/statements/query.dart

@@ -204,19 +204,23 @@ mixin SingleTableQueryMixin<T extends Table, D extends DataClass>
         'UpdateStatement.write respectively. In that case, you need to use a '
         'custom where statement.');
 
-    final primaryKeys = table.$primaryKey.map((c) => c.$name);
+    final primaryKeyColumns = Map.fromEntries(table.$primaryKey.map((column) {
+      return MapEntry(column.$name, column);
+    }));
 
     final updatedFields = table.entityToSql(d.createCompanion(false));
-    // Extract values of the primary key as they are needed for the where clause
+    // Construct a map of [GeneratedColumn] to [Variable] where each column is
+    // a primary key and the associated value was extracted from d.
     final primaryKeyValues = Map.fromEntries(updatedFields.entries
-        .where((entry) => primaryKeys.contains(entry.key)));
+            .where((entry) => primaryKeyColumns.containsKey(entry.key)))
+        .map((columnName, value) {
+      return MapEntry(primaryKeyColumns[columnName], value);
+    });
 
     Expression<bool, BoolType> predicate;
     for (var entry in primaryKeyValues.entries) {
-      // custom expression that references the column
-      final columnExpression = CustomExpression(entry.key);
       final comparison =
-          _Comparison(columnExpression, _ComparisonOperator.equal, entry.value);
+          _Comparison(entry.key, _ComparisonOperator.equal, entry.value);
 
       if (predicate == null) {
         predicate = comparison;

moor/test/delete_test.dart

@@ -32,14 +32,14 @@ void main() {
           .go();
 
       verify(executor.runDelete(
-          'DELETE FROM users WHERE (NOT is_awesome) OR (id < ?);', [100]));
+          'DELETE FROM users WHERE NOT is_awesome OR id < ?;', [100]));
     });
 
     test('to delete an entity via a dataclasss', () async {
       await db.delete(db.sharedTodos).delete(SharedTodo(todo: 3, user: 2));
 
       verify(executor.runDelete(
-          'DELETE FROM shared_todos WHERE (todo = ?) AND (user = ?);', [3, 2]));
+          'DELETE FROM shared_todos WHERE todo = ? AND user = ?;', [3, 2]));
     });
   });
 

moor/test/expressions/algebra_test.dart

@@ -10,11 +10,12 @@ void main() {
   final s2 = GeneratedTextColumn('s2', 'tbl', true);
 
   test('arithmetic test', () {
-    _expectSql(i1 + i2 * i1, '(i1) + ((i2) * (i1))');
+    _expectSql(i1 + i2 * i1, 'i1 + i2 * i1');
+    _expectSql((i1 + i2) * i1, '(i1 + i2) * i1');
   });
 
   test('string concatenation', () {
-    _expectSql(s1 + s2, '(s1) || (s2)');
+    _expectSql(s1 + s2, 's1 || s2');
   });
 }
 

moor/test/select_test.dart

@@ -70,7 +70,7 @@ void main() {
           .get();
 
       verify(executor.runSelect(
-          'SELECT * FROM users WHERE (NOT name = ?) AND (id > ?);',
+          'SELECT * FROM users WHERE NOT (name = ?) AND id > ?;',
           ['Dash', 12]));
     });