(>=.) and friends have unsound types with respect to Maybe

[lf-]

If you use a comparison operator on a Maybe value, there is nothing stopping a null getting into your conditionals if the values going into >=. are actually Nothing. This may wind up ruining your day or otherwise creating ... funny runtime bugs, since you create a NULL in a Value Bool, which is not supposed to be there.

oops :: SqlQuery (SqlExpr (Value Bool))
oops = do
  pure (val Nothing >=. (val . Just) (2::Int))

I think that in the current type signatures, val . Just is probably universally an invitation for bugs with the sole exception of if the result goes into ||..

Real-world example

If you write a query like this and omit the isNothing check you can cause this issue:

-- | Schedule to accept calls
ScheduleEntry sql=schedule_entries
    -- | Time at which this schedule starts applying
    startDate Day Maybe
    endDate Day Maybe

    -- | Start time in each day that this schedule applies
    startHours Int
    startMins Int

    endHours Int
    endMins Int

applicableRules :: LocalTime -> SqlQuery (SqlExpr (Entity ScheduleEntry))
applicableRules ts = do
  sched <- from $ table @ScheduleEntry
  let day = localDay ts
  let TimeOfDay {todHour, todMin} = localTimeOfDay ts
  where_ $ isNothing sched.startDate ||. (val . Just) day >=. sched.startDate
  where_ $ isNothing sched.endDate ||. (val . Just) day <=. sched.endDate
  where_ $ val todHour >=. sched.startHours &&. val todHour <=. sched.endHours
  where_ $ val todMin >=. sched.startMins &&. val todMin <=. sched.endMins
  pure sched

This could possibly be fixed in a way that only breaks suspicious uses (although my condolences extend to anyone with a large codebase dealing with migrating this; probably would want to keep around the old buggy signatures for compat under a different name, so you could just replace all usages and migrate piecewise):

type family BoolOpResult (a :: Type) :: Type where
  BoolOpResult (Maybe a) = Maybe Bool
  BoolOpResult a = Bool

(>=.) :: (PersistField typ) => SqlExpr (Value typ) -> SqlExpr (Value typ) -> SqlExpr (Value (BoolOpResult typ))
(>=.) = unsafeSqlBinOp ">= "

cc @parsonsmatt, I'm unsure if this type level shenanigans is fully sound either

[lf-]

A note about a consequent API change to fixing all the boolean operators to properly model tristate: you would need to fix stuff like

q =
  from $ table @Catgirls
  `leftJoin` table @Catears
  `on` (\(cg :& earb) -> just cg.id ==. earb.owner)

where owner may be null, so you use just in this comparison. Under the suggested fix, this comparison would now return a Maybe, which is totally reasonable as an on clause input; it will do the right thing if it gets a null, but the on function would have to be adjusted to accept this.

I am increasingly thinking that perhaps the sound comparison operators should be introduced into a new module that would become part of the default set when Experimental is actually stabilized, and then you could migrate module by module, because I can already see how much this might suck for industry code.

[isomorpheme]

I think this is duplicate with #130 - the writeup here is much more detailed though. 😁

[lf-]

I think you're correct! Maybe we should close that one :p