Make BoolExpressionMethods not always return a Nullable<bool>

[Ten0]

Fixes #2589

With new ideas from #2589 and specifically #2589 (comment), it turns out we can solve the issue encountered when writing 7ff5e63 of "not all methods had an equivalent type in diesel::dsl" in a simpler way, without making "BoolExpressionMethods::and (and or) unconditonally nullable" nor being a breaking change in most places.

As a consequence, this mostly reverts the part of 7ff5e63 that was focusing on this change, while introducing typing suggested in #2589 (comment), reducing a bit the overall amount of code.

[pksunkara]

I might be misunderstanding something, but don't we need to revert the following?

• File 7ff5e63#diff-cb055b4bb9e7efa6083a07a27019f0d52abeedb051162b636867fe309730d854
• Line 7ff5e63#diff-354105a6b67774b9eeb51dd06d81db7a1b30232369b26c872385803da0f7698eR449

Otherwise looks good.

[pksunkara]

I am still having trouble using this PR.

If there's a column email VARCHAR (it is nullable), why does email.eq(...) produce Nullable<Bool> for boxed expression? Shouldn't .eq always result in Bool?

error[E0271]: type mismatch resolving `<Grouped<diesel::expression::operators::And<Box<dyn BoxableExpression<table, Pg, SqlType = Bool>>, Grouped<diesel::expression::operators::Eq<columns::email, diesel::expression::bound::Bound<diesel::sql_types::Nullable<Text>, Option<&str>>>>>> as diesel::Expression>::SqlType == Bool`
  --> src/main.rs:25:17
   |
25 |     statement = Box::new(statement.and(users::email.eq(Some("John"))));
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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>`

[Ten0]

If there's a column email VARCHAR (it is nullable), why does email.eq(...) produce Nullable<Bool> for boxed expression? Shouldn't .eq always result in Bool?

In SQL, if one of the values of an operator such as AND or OR is null, the result may be null, so this is the correct mapping. It was incorrectly expressed in Diesel 1 due to #104 being pretty difficult to really solve, and which was only solved recently (#2182 (comment)). (NULL being deserialized as false ended up causing other issues in the deserialization of Option (#2274/#2182 (comment)), as well as some silent fails when #1459/#2001 happened (#2063), so it appears deserializing as false is not a correct approach).

Consequently, the behavior you're looking for should be achieved by using an SQL operator that has this behavior internally to your database, e.g. IS DISTINCT FROM for PostgreSQL (doc: postgres, diesel).

Note: it looks like as declared in Diesel, this one does not know it can never return null regardless of whether the input values are NULL:

diesel/diesel/src/pg/expression/operators.rs

Line 5 in 06494e8

infix_operator!(IsDistinctFrom, " IS DISTINCT FROM ", backend: Pg);

diesel/diesel/src/expression/operators.rs

Lines 229 to 237 in 06494e8

	return_ty = (
	$crate::sql_types::is_nullable::MaybeNullable<
	$crate::sql_types::is_nullable::IsOneNullable<
	<T as $crate::expression::Expression>::SqlType,
	<U as $crate::expression::Expression>::SqlType
	>,
	$($return_ty)::*
	>
	),

That would be a separate issue to solve.

[pksunkara]

In SQL, if one of the values of an operator such as AND or OR is null, the result may be null, so this is the correct mapping.

But we are not doing AND or OR here. I am doing eq which is checking for equality.

I understand eq doesn't give me true when comparing two NULLs, but it still gives me a boolean and not null.

[weiznich]

I understand eq doesn't give me true when comparing two NULLs, but it still gives me a boolean and not null.

Thats not correct. 1 = NULL resolves to NULL.

[Ten0]

In SQL, if one of the values of an operator such as AND or OR is null, the result may be null, so this is the correct mapping.

But we are not doing AND or OR here. I am doing eq which is checking for equality.

I understand eq doesn't give me true when comparing two NULLs, but it still gives me a boolean and not null.

I meant = as well yes, which is why I pointed to IS DISTINCT FROM.

[weiznich]

Looks good but we should talk about that minor nit regarding to the not function.

[weiznich]

I'm not sure if that should only accept non nullable values. I think accepting both Bool and Nullable<Bool> here is more meaningful. On the other hand that could potentially lead to situations where rustc cannot infer the correct type for ST from AsExpression here.

[Ten0]

Hmm that looks like kind of the same problem as and/or no? Where if we enforce that it takes an AsExpression<Nullable<Bool>> we'll never be able to make it return a non-nullable bool?

Additionally, not all expressions of SqlType Bool implement AsExpression<Nullable<Bool>> anymore, because otherwise this creates type inference issues with and/or: #2597 (comment)
Hence the following does not compile anymore if we do this: not(name.eq("Sean") (from tests)

Finally, I believe the behavior of not on Nullable<Bool> is a bit of a trap in that NULL values result in NULL, so they do not change "truthiness" with regards to e.g. a filter, which may lead to nasty bugs in case this kind of confusion is made and the compiler does not prevent it.

So overall I believe we should keep this behavior where we only accept non-nullable values here.

[pksunkara]

I think accepting both Bool and Nullable

I had encountered a situation where I want to store the same. Is there a way we can specify that?

[weiznich]

We must accept both variants (Bool and Nullable<Bool>) here otherwise it's not possible to write queries like not(foo.and(bar)) or even not(foo.eq(bar)) anymore in cases one of the arguments is Nullable<Bool>

Edit: @pksunkara Seems like you've added your comment while I typed mine. Yes we need this method to automatically accept both variants, otherwise we would restrict quite a lot of possible queries using not.

[Ten0]

I had encountered a situation where I want to store the same

Not sure what you mean by "the same".

We must accept both variants (Bool and Nullable<Bool>) here otherwise it's not possible to write queries like not(foo.and(bar)) or even not(foo.eq(bar)) anymore in cases one of the arguments is Nullable<Bool>

So I thought it maybe wasn't a good idea to have not support this implicitly, as this may easily lead to nasty bugs:

I believe the behavior of not on Nullable<Bool> is a bit of a trap in that NULL values result in NULL, so they do not change "truthiness" with regards to e.g. a filter, which may lead to nasty bugs in case this kind of confusion is made and the compiler does not prevent it.

Example:

table.filter(not(foo.eq(bar)))

Where foo = true and bar = NULL, the lines won't show, because not won't change the "truthiness" of the expression, which is a bit counter-intuitive.

Also, our codebase is rather large and we have zero such patterns.

Given that, do you still believe it's important for not to support this? (and we couldn't be happy with a separate nullable_not?)

[weiznich]

I still believe that not should support that as this is just the behavior of the underlying SQL operator. I feel that we had this discussion already about a few operators (for example eq in another context). Every time the final answer was: We follow the semantic of the underlying SQL operator as we do not try to hide complexity at this level of abstraction. Hiding this complexity can be done in a crate build on top of diesel, providing it owns set of operators that is not based on SQL in my opinion.

[Ten0]

I agree. Done in df8b5e5.