Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HasField for SqlExpr (Maybe (Entity a)) should use Nullable #407

Open
parsonsmatt opened this issue Dec 26, 2024 · 3 comments · Fixed by #422
Open

HasField for SqlExpr (Maybe (Entity a)) should use Nullable #407

parsonsmatt opened this issue Dec 26, 2024 · 3 comments · Fixed by #422
Labels
breaking Breaking changes to the API, requiring a major version bump
Milestone

Comments

@parsonsmatt
Copy link
Collaborator

@rampion suggested this in the Mercury slack, and I like the idea.

Quoting:

-- that is, change this
instance
    (PersistEntity rec, PersistField typ, SymbolToField sym rec typ)
  =>
    HasField sym (SqlExpr (Maybe (Entity rec))) (SqlExpr (Value (Maybe typ)))

-- to this
instance
    (PersistEntity rec, PersistField typ, SymbolToField sym rec typ)
  =>
    HasField sym (SqlExpr (Maybe (Entity rec))) (SqlExpr (Value (Maybe (Nullable typ))))

With this in place, mBar.thing would instead have type SqlExpr (Value (Maybe Thing)) .

@parsonsmatt parsonsmatt added beginner friendly breaking Breaking changes to the API, requiring a major version bump labels Dec 26, 2024
@parsonsmatt parsonsmatt added this to the 3.6 milestone Dec 26, 2024
@rampion
Copy link

rampion commented Dec 26, 2024

Proposed because one of the most common sources of the SqlExpr (Value (Maybe (Maybe a))) values I encounter is from mEntity.fieldA where mEntity :: SqlExpr (Maybe (Entity b)) and fieldA :: Maybe a in record b.

@parsonsmatt
Copy link
Collaborator Author

This may not actually be super beginner friendly - the Nullable type family may not have the right inference properties. Plus we need a value-level function witness to make the type work (though I guess veryUnsafeCoerceSqlExpr does it).

@parsonsmatt
Copy link
Collaborator Author

Ah, alas, we can't use Nullable directly:

/home/matt/Projects/esqueleto/src/Database/Esqueleto/Internal/Internal.hs:2483:5: error: [GHC-73138]
     Illegal type synonym family application Nullable typ in instance:
        HasField
          sym
          (SqlExpr (Maybe (Entity rec)))
          (SqlExpr (Value (Maybe (Nullable typ))))
     In the instance declaration for
        HasField sym (SqlExpr (Maybe (Entity rec))) (SqlExpr (Value (Maybe (Nullable typ))))
     |      
2483 |     HasField sym (SqlExpr (Maybe (Entity rec))) (SqlExpr (Value (Maybe (Nullable typ))))
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So we need a class...

This works, but it's real ugly:

instance
    (PersistEntity rec, PersistField typ, PersistField typ', SymbolToField sym rec typ
    , NullableFieldProjection typ typ'
    , HasField sym (SqlExpr (Maybe (Entity rec))) (SqlExpr (Value (Maybe typ')))
    )
  =>
    HasField sym (SqlExpr (Maybe (Entity rec))) (SqlExpr (Value (Maybe typ')))
  where
    getField expr = veryUnsafeCoerceSqlExpr (expr ?. symbolToField @sym)

class NullableFieldProjection typ typ'
instance {-# overlapping #-} NullableFieldProjection (Maybe typ) typ
instance {-# overlappable #-} NullableFieldProjection typ typ

The type inference is really bad, too. We can't have a functional dependency because there's an overlap in the two instances.

However, we do get this working:

instance
    (PersistEntity rec, PersistField typ, PersistField typ', SymbolToField sym rec typ
    , NullableFieldProjection typ typ'
    , HasField sym (SqlExpr (Maybe (Entity rec))) (SqlExpr (Value (Maybe typ')))
    )
  =>
    HasField sym (SqlExpr (Maybe (Entity rec))) (SqlExpr (Value (Maybe typ')))
  where
    getField expr = veryUnsafeCoerceSqlExpr (expr ?. symbolToField @sym)
class NullableFieldProjection typ typ'
instance {-# overlapping #-} (typ ~ typ') => NullableFieldProjection (Maybe typ) typ'
instance {-# overlappable #-} (typ ~ typ') => NullableFieldProjection typ typ'

But unfortunately the errors are pretty bad where joinV would be used. Consider this existing query:

    itDb "joins Maybe together" $ do
        void $ select $ do
            deed :& lord <-
                Experimental.from $
                    table @Deed
                    `leftJoin` table @Lord
                        `Experimental.on` do
                            \(deed :& lord) ->
                                lord.id ==. just deed.ownerId
            where_ $ joinV lord.dogs >=. just (val 10)
            pure lord

This fails with:

/home/matt/Projects/esqueleto/test/Common/Test.hs:2508:28: error: [GHC-18872]
    • Couldn't match type ‘Int’ with ‘Maybe typ0’
        arising from selecting the field ‘dogs’
    • In the first argument of ‘joinV’, namely ‘lord.dogs’
      In the first argument of ‘(>=.)’, namely ‘joinV lord.dogs’
      In the second argument of ‘($)’, namely
        ‘joinV lord.dogs >=. just (val 10)’
     |      
2508 |             where_ $ joinV lord.dogs >=. just (val 10)
     |                            ^^^^^^^^^

We can fix this by changing joinV and giving an Incoherent pragma:

joinV
    :: (NullableFieldProjection typ typ')
    => SqlExpr (Value (Maybe typ))
    -> SqlExpr (Value (Maybe typ'))
joinV = veryUnsafeCoerceSqlExprValue

class NullableFieldProjection typ typ'
instance {-# incoherent #-} (typ ~ typ') => NullableFieldProjection (Maybe typ) typ'
instance {-# overlappable #-} (typ ~ typ') => NullableFieldProjection typ typ'

This feels dicey enough that I want to experiment with this separately on the work project to see how it works out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking changes to the API, requiring a major version bump
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants