Issue: `Expression.__bool__` should raise an exception
Author: abarnert
State: closed
Created: 2014-09-11 17:52:09

I haven't looked carefully enough to know if this is a plausible idea or not, but…

I've seen multiple people getting confused because `where(Meal.spam==2 and Meal.eggs==3)` ignores the `Meal.spam=2` part. To anyone who understands how Python's `and` operator works, this is pretty obvious, but Peewee is very useful to a lot of people who aren't experienced Python users.

One obvious solution is:

```
def __bool__(self):
    raise ValueError("The truth value of an expression is ambiguous until "
                     "executed in a query. Use x&y, x|y, ~x, x.bin_and(y), etc.")
```

Now, instead of silently getting a query they didn't want and having no idea how to debug it, they immediately get an error that tells them what's wrong and what to do about it.

Numpy and Pandas do effectively the same thing; if you try to use an array/matrix/series/dataframe in a boolean context (except a scalar or an array whose dimensions are all 1), you get:

```
ValueError('The truth value of an array with more than one element is ambiguous. '
           'Use a.any() or a.all()')
```

Of course if there's a good use case for using `Expression`s as truthy in a boolean context, this would be a bad change; just because I can't think of one off the top of my head doesn't mean it doesn't exist.


I think raising an exception will prevent a lot of hard-to-diagnose bugs...great suggestion.


Unfortunately, this will mean overriding the `__nonzero__` method and there are just too many places where the truthiness of a node object is checked. Maybe I could override it only for `Expression`...


One thing I just realized, from looking over SQLAlchemy:

If you make `__eq__` return an `Expression`, and you make `Expression.__bool__` raise an exception, then you can't use nodes as set elements or dict keys. And I can see wanting to be able to, e.g., build a set of column nodes, or map expressions to result sets (which works in Peewee today).

SQLAlchemy solves this by making any node _except_ for a `!=` or `==` `BinaryExpression` raise an exception in boolean context, but have those two expressions compare hash values. So a column is equal to itself and nothing else. Like this:

```
def __bool__(self):
    if self.operator in (operator.eq, operator.ne):
        return self.operator(hash(self._orig[0]), hash(self._orig[1]))
    else:
        raise TypeError("Boolean value of this clause is not defined")
```

I'm actually not sure this is why they've added the special case, but I can't see any other reason to do it.

That just seems wrong to me. `==` and `!=` are the two most commonly-used operators. All of the relevant questions I've seen on StackOverflow were of the form "I wrote `where(Person.name != 'Guido' and Person.commits > 1000)` but I still get Guido back". So SQLAlchemy protects people from this bug everywhere except the place they're most likely to see it. I think that may actually be worse than not protecting them and just trying to explain it in the docs.

Also, the only time a set or dict will ever ask you for a `==` comparison is if it already knows the hashes are equal and needs to check for a hash collision; if you do this, you need to ensure that no two live nodes ever have the same hash value as each other or as anything else they can be in a dict with, or you're going to have either those fun one-in-a-trillion bugs that are so much fun to reproduce or someone taking over a server using a collision attack…


> Also, the only time a set or dict will ever ask you for a == comparison is if it already knows the hashes are equal and needs to check for a hash collision

Very interesting!


Thanks for your insight and for sharing the behavior of SQA. I've thought about it some and I don't really think there's a great way to prevent people from accidentally using `not`, `or`, and `and`. Overriding `__nonzero__` seems like it might have too many unintended/unpredictable side-effects. Perhaps the solution is just to add better documentation.