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

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

First of all I'm fully aware of this and would call this an deliberate change. The commit you've linked in your comment even goes into details why this was changed that way (See the last bullet point). That written: I'm open to a change here as long as BoolExpressionMethod::and continue to return a diesel::dsl::And<Self, T>

Unless I'm mistaken, there's indeed no way to extract nullability directly from T as we cannot set higher requirements on T than AsExpression<ST>, for which we need to know ST.

So it indeed looks like without specialization we have to make a choice between:

Having helper_types::And and helper_types::Or return purely XXX<Self,T>
Knowing that a.and(b) can be non-null.

However:

This looks like a blocker for a standard (to boolean) .select(a.and(b)) with neither a nor b nullable, which may additionally be a nuisance when updating to diesel 2 (especially if the boolean is lost in the middle of some large structure and you have a 50 lines query that stops compiling ^^)
It seems slightly incorrect (forces the user to handle the None case where it actually cannot happen)
It looks like the And/Or syntax having to take ST or T::AsExpression as parameter would only be slight complexification
It seems more rare that a user will need to express query parts types than select(a.and(b)) (at least it is for me).
- Especially since when I do express query parts types it's usually because I have a large sub-query that I want to factor into a function, in which case, because it's large and I just need to check the query for correctness and not its type and I don't want to have too much maintenance to do on it, I don't bother re-creating the type for the query, and I just set the return type to (), and then copy-paste the type from the compilation error.

So overall I believe paying the cost of And<Self, T::Expression> is worth, though I'm not sure which is better between And<Self, T::Expression> and And<Self, T, ST> (depends on how much And is used I guess).

It seems slightly incorrect (forces the user to handle the None case where it actually cannot happen)

I would like to see a correct reference for this for at least all supported databases. NULL is really a strange concept in most SQL implementation, therefore I think it's not as simple as just saying stating that something like that "actually cannot happen".

It looks like the And/Or syntax having to take ST or T::AsExpression as parameter would only be slight complexification

But a breaking change for the existing API, while it's up to discussion to assume anything other than diesel::dsl::SqlTypeOf<diesel::dsl::And<A, T>> is part of the existing public API. I think non of the existing API documentation mentions what's exactly the current sql type of expr.eq(other).

Especially since when I do express query parts types it's usually because I have a large sub-query that I want to factor into a function, in which case, because it's large and I just need to check the query for correctness and not its type and I don't want to have too much maintenance to do on it, I don't bother re-creating the type for the query, and I just set the return type to (), and then copy-paste the type from the compilation error.

A main argumentation point here is: As soon as you are doing that you don't use the diesel public API but some internal API (as most types there are #[doc(hidden)]) where we don't give any guarantees about. It's fine to do that, but it means I won't consider that as valid argument here for reverting this change.

It seems slightly incorrect (forces the user to handle the None case where it actually cannot happen)

I would like to see a correct reference for this for at least all supported databases. NULL is really a strange concept in most SQL implementation, therefore I think it's not as simple as just saying stating that something like that "actually cannot happen".

So question is: are there cases where applying AND or OR to two non-nullable booleans would result in NULL being returned? I guess that would be kind of a troll implementation if it was ever the case, but in any case the following seems to confirm that: https://www.postgresql.org/docs/13/functions-logical.html https://dev.mysql.com/doc/refman/8.0/en/logical-operators.html#operator_and https://sqlite.org/nulls.html

It looks like the And/Or syntax having to take ST or T::AsExpression as parameter would only be slight complexification

But a breaking change for the existing API, while it's up to discussion to assume anything other than diesel::dsl::SqlTypeOf<diesel::dsl::And<A, T>> is part of the existing public API. I think non of the existing API documentation mentions what's exactly the current sql type of expr.eq(other).

As far as I'm concerned I'd be ok with any sort of breaking change so I meant I wouldn't mind if we had to add ST to And, but if that's an issue I still beleive it's worth having And<Self, T::Expression> instead of And<Self, T>. In which case that wouldn't be breaking for transitions between the two stable versions, since it would be the same as in 1.4, right? (https://docs.diesel.rs/1.4.x/diesel/expression_methods/trait.BoolExpressionMethods.html#method.and)

It's fine to do that, but it means I won't consider that as valid argument here for reverting this change.

Fair enough. Still seems worth given the versus between

slight inconsistency with the rest of the diesel_helpers types
blocker for select(a.and(b)).load::<bool>() (that is still in our codebase more used than explicit typing of queries) and expressions that don't match SQL behavior as closely as they could

I'm trying to workaround this and it's spiraling out of control.

Given the following kind of function:

    pub fn to_diesel_expr<QS: 'static>(
        &self,
    ) -> Option<Box<dyn BoxableExpression<QS, diesel::pg::Pg, SqlType = diesel::sql_types::Bool> + 'l>>
    where
        table::id: diesel::expression::SelectableExpression<QS>,
        /// and others
    {
        let mut filter: Option<Box<dyn BoxableExpression<QS, diesel::pg::Pg, SqlType = diesel::sql_types::Bool> + 'l>> =
            None;

        if !self.ids.is_empty() {
            filter = Some(match filter {
                Some(filter) => Box::new(filter.and(table::id.eq(dsl::any(self.ids)))),
                None => Box::new(table::id.eq(dsl::any(self.ids))),
            });
        }

        // and others

        filter
    }
}

Just adding Nullable around the Bools doesn't work: it gives errors stating that we don't have e.g. Nullable<table::id>: SelectableExpression<QS>, and changing where clause to this doesn't work either as it starts giving the following error, referring to a Rust issue:

error[E0277]: the trait bound `diesel::sql_types::Nullable<schema::table::columns::id>: diesel::Expression` is not satisfied
   --> mod.rs:75:2
    |
75  |       pub fn to_diesel_expr<QS>(
    |  _____^
76  | |         &self,
77  | |     ) -> Option<
78  | |         Box<
...   |
123 | |         filter
124 | |     }
    | |_____^ the trait `diesel::Expression` is not implemented for `diesel::sql_types::Nullable<schema::table::columns::id>`
    |
    = help: see issue #48214

Just to emphasize that this has rather strong implications regarding how complex migrating to Diesel 2.0 is.

Haven't found a way to implement this yet. (QS does not implement Table, as it may be a join.)

So question is: are there cases where applying AND or OR to two non-nullable booleans would result in NULL being returned? I guess that would be kind of a troll implementation if it was ever the case, but in any case the following seems to confirm that: https://www.postgresql.org/docs/13/functions-logical.html https://dev.mysql.com/doc/refman/8.0/en/logical-operators.html#operator_and https://sqlite.org/nulls.html

It may sound like I'm very pedantic here, but just have a look at how expr BETWEEN a AND b is handled in different database implementation and for NULL values for expr, a, b (at least the last time I've looked at this, there where differences what was returned there for postgresql depending on the actual non null values)

Just adding Nullable around the Bools doesn't work: it gives errors stating that we don't have e.g. Nullable: SelectableExpression, and changing where clause to this doesn't work either as it starts giving the following error, referring to a Rust issue:

I'm not sure why you thing Nullable<table::id> is the correct bound. Seems like rustc is suggesting something that won't work here again. You likely just need to use one of the upper level bounds listed under requiered because …

As far as I'm concerned I'd be ok with any sort of breaking change so I meant I wouldn't mind if we had to add ST to And, but if that's an issue I still believe it's worth having And<Self, T::Expression> instead of And<Self, T>. In which case that wouldn't be breaking for transitions between the two stable versions, since it would be the same as in 1.4, right? (https://docs.diesel.rs/1.4.x/diesel/expression_methods/trait.BoolExpressionMethods.html#method.and)

The problem with And<Self, T::Expression> is that T::Expression is something that is currently not part of the public API for all cases. For example for binds that would be Bound<ST, U> which is a private type. I would rather not make that part of the public API as this would unnecessarily bring even more complex types into the public API.

slight inconsistency with the rest of the diesel_helpers types

I think there is a much larger problem here. We currently "only" handle the mess with nullable types somewhat for and/or now, in such a way that allow nullable types here. For all other operators that is not implemented. I think it's "worked" around by just not allowing mixing nullable and not nullable values, but that does interact not that well with allowing AsExpression as argument for any query builder method. (AsExpression is required as we need a way to convert rust values via binds to sql expressions without having the user call something like .into_sql() (or something similar) for each value).

That all is not a solution to the underlying issues, just a lot of comments. That written will likely not have the time to work on fixing that anytime soon (there's only that much time each day and diesel is something that I do in my free time, so it's much less important than my real life work for example). So if that's an blocker for 2.0 that either means the work must be done by someone or there will not be a release anytime soon or the 2.0 release will contain the current changes as breaking changes (as it is currently documented).

Just encountered this with the following code:

use diesel::{
    expression::BoxableExpression,
    pg::Pg,
    sql_types::{Bool, Integer},
    BoolExpressionMethods, ExpressionMethods, IntoSql,
};

mod schema {
    diesel::table! {
        users (id) {
            id -> Int4,
            name -> Varchar,
        }
    }
}

fn main() {
    let mut statement: Box<dyn BoxableExpression<schema::users::table, Pg, SqlType = Bool>>;
    let one: i32 = 1;

    statement = Box::new(one.into_sql::<Integer>().eq(one));
    statement = Box::new(statement.and(schema::users::id.eq(1)));
}

giving the error:

error[E0271]: type mismatch resolving `<Grouped<diesel::expression::operators::And<diesel::expression::nullable::Nullable<Box<dyn BoxableExpression<table, Pg, SqlType = Bool>>>, diesel::expression::nullable::Nullable<Grouped<diesel::expression::operators::Eq<columns::id, diesel::expression::bound::Bound<Integer, i32>>>>>> as diesel::Expression>::SqlType == Bool`
  --> src/main.rs:22:17
   |
22 |     statement = Box::new(statement.and(schema::users::id.eq(1)));
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 BoxableExpression<table, Pg, SqlType = Bool>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0271`.
error: could not compile `example`

Investigation into Postgres:

select null between 1 and 10; -- NULL
select 1 between null and 10; -- NULL
select 1 between 2 and null; -- FALSE
select 1 between 1 and null; -- NULL

select 1=1 and null; -- NULL

From the last query, It looks like and does need to support NULL for diesel to correctly represent stuff.

I think focusing on fixing the Nullable<table::id>: SelectableExpression<QS> error is probably the correct way to fix this issue.

So, as suspected, using the Nullable<Bool> boxed expression in select doesn't seem to work. I think we can comfortably say that all previous BoxableExpression don't work anymore.

    let statement = Box::new(users::name.eq("John").and(users::id.eq(1)));

    let users = users::table
        .select(statement)
        .get_results::<User>(&pool.get());

error[E0277]: the trait bound `User: diesel::Queryable<diesel::sql_types::Nullable<Bool>, _>` is not satisfied
  --> src/main.rs:37:10
   |
37 |         .get_results::<User>(&pool.get());
   |          ^^^^^^^^^^^ the trait `diesel::Queryable<diesel::sql_types::Nullable<Bool>, _>` is not implemented for `User`
   |
   = help: the following implementations were found:
             <User as diesel::Queryable<(__ST0, __ST1), __DB>>
   = note: required because of the requirements on the impl of `FromSqlRow<diesel::sql_types::Nullable<Bool>, _>` for `User`
   = note: required because of the requirements on the impl of `LoadQuery<_, User>` for `SelectStatement<table, query_builder::select_clause::SelectClause<Box<Grouped<diesel::expression::operators::And<diesel::expression::nullable::Nullable<Grouped<diesel::expression::operators::Eq<columns::name, diesel::expression::bound::Bound<Text, &str>>>>, diesel::expression::nullable::Nullable<Grouped<diesel::expression::operators::Eq<columns::id, diesel::expression::bound::Bound<Integer, i32>>>>>>>>>`

I can't set id field in User as Option because schema doesn't have it as Nullable.

I might be missing some context, but if self can be nullable, I don't understand why there is a need to wrap self with Nullable when constructing And.

@pksunkara

So, as suspected, using the Nullable boxed expression in select doesn't seem to work. I think we can comfortably say that all previous BoxableExpression don't work anymore.

That's not correct. Your example is just something that was never supported. How would an query that returns Nullable<Bool> (or Bool on 1.4.x) map to your user struct, that has 2 fields?

From the last query, It looks like and does need to support NULL for diesel to correctly represent stuff.

I think the point here is slightly different. If we know that one argument to and() can be nullable we need to assume that the result can also be nullable, but if we know that non of the arguments is nullable, we can assume that the result is also not nullable. In fact the underlying And operator already has this behavior since https://github.com/diesel-rs/diesel/pull/2182/commits/ee470bb07c1366fb643aba588cf989552e05b02c. The problem is how to expose that behind some usable API while preventing unnecessary breaking changes to the public API.

I might be missing some context, but if self can be nullable, I don't understand why there is a need to wrap self with Nullable when constructing And.

As already pointed out above: The issue is not that we cannot represent the conditionally nullability for BooleanExpressionMethods::and, but how it interacts with AsExpression<ST> for the second argument. To just use an example here: We want to be able to easily write expressions like foo.eq(42).and(true) Now this mixes rust values with diesel query dsl expressions. We need some way to transform those rust values to the underlying query dsl node. This is done by calling AsExpression<ST> on those values, which will basically create a bind query node there. Now we need to know the sql type there for type checking the query later on to prevent someone passing in a rust String where the sql side expects an Integer expression. Now let's have a look at the definition of and():

pub trait BoolExpressionMethods: Expression + Sized {
    fn and<T>(self, other: T) -> crate::dsl::And<Self, T>;
}

(left of the where clause, as this is the critical part of this issue).

Now we now the following things for the implementation of and():

self is an Expression. The implementation later on enforces that the SqlType is either Bool or Nullable<Bool>. For 1.4 we only allowed Bool there.
We return crate::dsl::And<…> as return type. This is a typedef that hides implementation details. In general users are encouraged to use the types in diesel::dsl in where ever they need to specify the type of some query expression as we consider only this types to be part of the public API (and not the underlying query dsl structure). This means changing something at this return type is a rather large breaking change
The second argument is generic. For the 1.4 release it was constrained on AsExpression<Bool>, as we only allowed not nullable boolean expression to be joined via .and(). For 2.0 we need the same AsExpression<ST> bound there, but it's harder to write any concrete type for ST because we have basically the following cases:
1. The left and the right expression type are both Bool -> The resulting expression type can be Bool + we should probably use ST = Bool
2. The left expression type is Bool and the right expression type is Nullable<Bool> -> The resulting expression type is Nullable<Bool> + we must use ST = Nullable<Bool>
3. The left expression type is Nullable<Bool> and the right expression type is Bool> -> The resulting expression type is Nullable<Bool> + we can use either ST = Bool or ST = Nullable<Bool> (as we can coerce non nullable expressions into nullable).
4. Both expression types are Nullable<Bool> -> The resulting expression type needs to be Nullable<Bool> + ST = Nullable<Bool>.

Now the problem is that I've found no way yet to somehow write the required trait bounds there without changing the return type substantial. As already written above: I've currently neither time nor motivation for another deep dive into diesels internal type system, as I expect that to cost me another month or two of free time that I would rather like to spend on something else. So if something would like to have this fixed, feel free to work on a substantial fix, rather than just trying to fix the syntomes by softening the gurantees given by our type system (That is what fixing the Nullable<table::id>: SelectableExpression<QS> means).

The same discussion applies to BooleanExpressionMethod::or or basically every other binary expression out there (I think most of them just disallow the mixed nullable/not nullable case), but the mapping there is different…

To just write down briefly what I would expect from such a fix:

It does not change any return type of *ExpressionMethods compared to the 1.4.5 release (Changing the type mapping of the type def itself is ok)
It does address the fundamental issue on how we handle mixed nullability of arguments for *ExpressionMethods, which means it not only fixes .and() but at least also .or() and adds some (compile-fail) test cases for other methods (Between, in, …)

I had started to write an answer when the thread was at https://github.com/diesel-rs/diesel/issues/2589#issuecomment-740637515 but apparently I forgot to conclude it and somehow lost the text :( So let's rewrite. :)

I'm not sure why you thing Nullable<table::id> is the correct bound. Seems like rustc is suggesting something that won't work here again. You likely just need to use one of the upper level bounds listed under requiered because …

That is not a rustc suggestion, but something I attempted because it seemed to match the requirement change of and. The error message I get when attempting to use this seems to refer to a Rust issue, and I beleive the bound it mentions is not satisfied is actually satisfied. Doing this manually, one trait bound suggested in the chain that makes this compile singlehandedly is QS: Table, however it's not one I can use because QS can actually be a set of joins, which does not implement Table. Instead, these are the query bounds I had to use (which are I beleive the ones that make the compiler eventually able to understand that Nullable<table::id>: diesel::expression::SelectableExpression<QS>):

    pub fn to_diesel_expr<QS>(
        &self,
    ) -> Option<Box<dyn BoxableExpression<QS, diesel::pg::Pg, SqlType = Nullable<diesel::sql_types::Bool>> + 'l>>
    where
        QS: diesel::query_source::QuerySource
            + diesel::query_source::joins::ToInnerJoin
            + diesel::query_source::AppearsInFromClause<parcels::table, Count = diesel::query_source::Once>
            + 'static,
        table1::id: diesel::expression::SelectableExpression<QS::InnerJoin>,
        table2::field1: diesel::expression::SelectableExpression<QS::InnerJoin>,
        dyn BoxableExpression<QS, diesel::pg::Pg, SqlType = Nullable<diesel::sql_types::Bool>> + 'l:
            SelectableExpression<QS::InnerJoin>,
        Box<dyn BoxableExpression<QS, diesel::pg::Pg, SqlType = Nullable<diesel::sql_types::Bool>> + 'l>:
            diesel::expression::Expression<SqlType = Nullable<diesel::sql_types::Bool>>

And this took me 2 or 3 hours to write.

have a look at how expr BETWEEN a AND b is handled in different database implementation and for NULL values for expr, a, b (at least the last time I've looked at this, there where differences what was returned there for postgresql depending on the actual non null values)

Looks like in this case, a AND b is not an expression by itself, and instead BETWEEN a AND b is, in the sense of whatever other ordered SQL type.

I think there is a much larger problem here. We currently "only" handle the mess with nullable types somewhat for and/or now, in such a way that allow nullable types here. For all other operators that is not implemented. I think it's "worked" around by just not allowing mixing nullable and not nullable values, but that does interact not that well with allowing AsExpression as argument for any query builder method. (AsExpression is required as we need a way to convert rust values via binds to sql expressions without having the user call something like .into_sql() (or something similar) for each value).

As far as I'm concerned, I'm pretty happy with the fact non-nullable expressions are not automatically coerced into nullable expressions, the same way rust does not go and coerce stuff into Options. That matches pretty well the general rust philosophy of being explicit. This avoids propagating type errors further than they should be, and ending up wondering for ages why in the world type XXX is nullable when you thought the query part you wrote did not lead to that.

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.

In any case, though I don't see this as an issue this is not a change I'd necessarily be opposed to. However, if there's going to be a global solution working towards that direction, it's probably not going to be "making the output of every call nullable", so I'd rather keep this uniform with the rest until we find a solution that does not have this issue we're trying to solve here, and then apply it everywhere. (Especially since moving BooleanExpressions back to non-nullable would be an even more breaking change than it currently is, so this issue would be likely to follow us for a very long time if retrocompatibility is such a concern).

The problem with And<Self, T::Expression> is that T::Expression is something that is currently not part of the public API for all cases. For example for binds that would be Bound<ST, U> which is a private type. I would rather not make that part of the public API as this would unnecessarily bring even more complex types into the public API.

So currently people have to write And<Self, T>, where both Self and T can be expressed. Then nothing prevents them from writing And<Self, <T as AsExpression<Nullable<Bool>>>::Expression> or And<Self, <T as AsExpression<Bool>>::Expression>, as AsExpression, Nullable and Bool are public types.

So in the end it looks like this wouldn't make this return type impossible to express with public types.

In order to make that easy if we want to maintain the largest possible amount of retrocompatibility (that is, by not using And<Self, T, ST>), we could create NullableAnd (=And<Self, T, Nullable<Bool>>) and NonNullableAnd (=And<Self, T, Bool>) aliases and keep And<Self, T::Expression>.

We return crate::dsl::And<…> as return type. This is a typedef that hides implementation details. In general users are encouraged to use the types in diesel::dsl in where ever they need to specify the type of some query expression as we consider only this types to be part of the public API (and not the underlying query dsl structure). This means changing something at this return type is a rather large breaking change

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

One is super easy to fix while the other requires to select in different structures (because we have to use Option<bool> instead of bool, and we all know how much harder to read these errors are than And expects 3 arguments, got 2) then either .expecting stuff in the possibly spread-out places where these structures are used, or remapping into a structure that has all the same fields except these Options that get turned in bool just after the query, either way lessening the type system guarantees and severely increasing the amount of code required.
We way more often select(a.and(b)) than write explicit types for sub-queries.

Now the problem is that I've found no way yet to somehow write the required trait bounds there without changing the return type substantial.

So it looks like an approach that essentially consists to ~reverting BooleanExpressionMethods to ~the state it was in at ee470bb07c1366fb643aba588cf989552e05b02c ? (with #2140 still fixed of course) would be a somewhat substantial change of And<...> (though it would still be And<...>), but it seems that would solve this issue, and also seems to satisfy all the conditions specified in https://github.com/diesel-rs/diesel/issues/2589#issuecomment-746077224 (since they were satisfied at ee470bb07c1366fb643aba588cf989552e05b02c, right?).

if something would like to have this fixed, feel free to work on a substantial fix

This would probably be shorter for me (and way better on the long run) than updating all the places where we select(a.and(b)) and where we Box filters. Before I put the work in, can you confirm that's a path that could theoretically be merged (provided the implementation looks good)?

(Note that I'd personally be even happier with And<Self, T, ST> because it seems cleaner and more consistent with the rest as you pointed out in 7ff5e6327d5e88095cb4b342da13bf06d6d0d1cd, and after all 2.0 is for allowing breaking improvements, but I'd be fine with implementing either.)

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, 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.

Oh right, so we set dsl::And with ST = Bool for backward compatibility, and have dsl::NullableAnd with ST = Nullable<Bool> for easily expressing the type when Self::SqlType is nullable, and possibly another MaybeNullableAnd<Self, T, ST> (any idea on a better naming?) for the actual return type? LGTM. I can probably provide a PR shortly (like, next week at most) :)

I think we do not need to provide any other type wrapper other than dsl::And here. For the PR, please change the or method as well. Everthing written in this thread applies there too.

diesel-rs / diesel