diesel: master: a.and(b) is nullable even though a and b are non nullable

Setup

Versions

Rust: latest stable - 1.48.0 (7eac88abb 2020-11-16)
Diesel: latest master (ff8e5b15ea0dfaa22e9c12c5ed2a0e23992b0617)
Database: postgresql
Operating System Linux

Feature Flags

diesel: postgres, chrono

Problem Description

a.and(b) is nullable even though a and b are non nullable

What are you trying to accomplish?

Writing an and condition of which I expect the output to be non-nullable.

What is the expected output?

compiles

What is the actual output?

error[E0271]: type mismatch resolving `<Grouped<diesel::expression::operators::And<diesel::expression::nullable::Nullable<Grouped<diesel::expression::operators::Gt<C, diesel::expression::bound::Bound<Timestamptz, DateTime<Utc>>>>>, diesel::expression::nullable::Nullable<Grouped<diesel::expression::operators::Lt<C, diesel::expression::bound::Bound<Timestamptz, DateTime<Utc>>>>>>> as diesel::Expression>::SqlType == Bool`
  --> src/lib.rs:49:29
   |
49 |             InInterval { gt, lt } => Box::new(column.gt(gt).and(column.lt(lt)))
   |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `Bool`, found struct `diesel::sql_types::Nullable`
   |
   = note: expected struct `Bool`
              found struct `diesel::sql_types::Nullable<Bool>`
   = note: required for the cast to the object type `dyn diesel::BoxableExpression<T, Pg, SqlType = Bool>`

Are you seeing any additional errors?

Steps to reproduce

git clone https://github.com/Ten0/diesel_2589_repro
cd diesel_2589_repro
cargo check

Code for reference in case that repo is removed later:

use diesel::prelude::*;

pub enum TimestampFilter {
	LowerThan(chrono::DateTime<chrono::Utc>),
	GreaterThan(chrono::DateTime<chrono::Utc>),
	InInterval {
		gt: chrono::DateTime<chrono::Utc>,
		lt: chrono::DateTime<chrono::Utc>,
	},
}


pub trait IntoDieselExpr<SqlType> {
	type OutSqlType: diesel::sql_types::SingleValue;
	fn into_diesel_expr<T, C>(
		self,
		column: C,
	) -> Box<dyn BoxableExpression<T, diesel::pg::Pg, SqlType = Self::OutSqlType>>
	where
		T: diesel::Table + 'static,
		SqlType: diesel::sql_types::SingleValue,
		C: Expression<SqlType = SqlType>
			+ diesel::query_builder::QueryFragment<diesel::pg::Pg>
			+ diesel::expression::ValidGrouping<(), IsAggregate = diesel::expression::is_aggregate::No>
			+ diesel::expression::SelectableExpression<T>
			+ Copy
			+ 'static;
}

impl IntoDieselExpr<diesel::sql_types::Timestamptz> for TimestampFilter {
	type OutSqlType = diesel::sql_types::Bool;
	fn into_diesel_expr<T, C>(
		self,
		column: C,
	) -> Box<dyn BoxableExpression<T, diesel::pg::Pg, SqlType = Self::OutSqlType>>
	where
		T: diesel::Table + 'static,
		C: Expression<SqlType = diesel::sql_types::Timestamptz>
			+ diesel::query_builder::QueryFragment<diesel::pg::Pg>
			+ diesel::expression::ValidGrouping<(), IsAggregate = diesel::expression::is_aggregate::No>
			+ diesel::expression::SelectableExpression<T>
			+ Copy
			+ 'static,
	{
		use TimestampFilter::*;
		match self {
			LowerThan(date) => Box::new(column.lt(date)),
			GreaterThan(date) => Box::new(column.gt(date)),
			InInterval { gt, lt } => Box::new(column.gt(gt).and(column.lt(lt)))
				as Box<dyn BoxableExpression<T, diesel::pg::Pg, SqlType = Self::OutSqlType>>,
		}
	}
}

impl IntoDieselExpr<diesel::sql_types::Nullable<diesel::sql_types::Timestamptz>> for TimestampFilter {
	type OutSqlType = diesel::sql_types::Nullable<diesel::sql_types::Bool>;
	fn into_diesel_expr<T, C>(
		self,
		column: C,
	) -> Box<dyn BoxableExpression<T, diesel::pg::Pg, SqlType = Self::OutSqlType>>
	where
		T: diesel::Table + 'static,
		C: Expression<SqlType = diesel::sql_types::Nullable<diesel::sql_types::Timestamptz>>
			+ diesel::query_builder::QueryFragment<diesel::pg::Pg>
			+ diesel::expression::ValidGrouping<(), IsAggregate = diesel::expression::is_aggregate::No>
			+ diesel::expression::SelectableExpression<T>
			+ Copy
			+ 'static,
	{
		use TimestampFilter::*;
		match self {
			LowerThan(date) => Box::new(column.lt(date)),
			GreaterThan(date) => Box::new(column.gt(date)),
			InInterval { gt, lt } => Box::new(column.gt(gt).and(column.lt(lt))),
		}
	}
}

Checklist

I have already looked over the issue tracker for similar issues.
This issue can be reproduced on Rust’s stable channel. (Your issue will be closed if this is not the case)

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 15 (15 by maintainers)

Commits related to this issue

Allow ~type inference on `And` and `Or` helper types This is [particularly useful for query fragment functions type inference](https://github.com/diesel-rs/diesel/discussions/3661#discussioncomment-6... — committed to Ten0/diesel by Ten0 a year ago
Allow ~type inference on `And` and `Or` helper types This is [particularly useful for query fragment functions type inference](https://github.com/diesel-rs/diesel/discussions/3661#discussioncomment-6... — committed to Ten0/diesel by Ten0 a year ago

Most upvoted comments

I thought there may also be cases where it’s nice that .nullable() forces you to consider that the expression e.g. a.ne(b) may actually be NULL and hence be falsy in a filter, though a is not null and b null, so the behaviour would be different from Option, but as it turns out I haven’t found a concrete example of such a scenario in our codebase so I guess that’s not so much of an issue.

The point is that strategy is not really possible for .and() because otherwise you would have a filter() call with quite a lot of .nullable() calls in it. I wouldn’t call that a good API, especially as this was not the case for 1.x.

In my opinion adding ST to And is a far less breaking change than making a.and(b) always return Nullable<bool>, because:

Having thought about this a bit more I think I’ve found a solution that does not require a breaking change. On 1.4.x the impl for BooleanExpressionMethods::and looks like this:

pub trait BoolExpressionMethods: Expression<SqlType = Bool> + Sized {
    fn and<T: AsExpression<Bool>>(self, other: T) -> dsl::And<Self, T> {
        And::new(self.as_expression(), other.as_expression())
    }
}

Now as already elaborated quite a lot of times, just changing that to

pub trait BoolExpressionMethods: Expression<SqlType = Bool> + Sized {
    fn and<T: AsExpression<ST>, ST>(self, other: T) -> dsl::And<Self, T, ST> {
        And::new(self.as_expression(), other.as_expression())
    }
}

is breaking change I’m not really comfortable with, but I think we can easily workaround that by just setting the ST parameter on dsl::And to ST = Bool as this is the case for all expressions on 1.4 if I correctly see that. If anyone is willing to provide a PR doing that I’m likely fine with that change.

weiznich on Dec 18, 2020