Skip to content

Unify function and nullaryFunction#258

Merged
shane-circuithub merged 1 commit intomasterfrom
function
Jul 11, 2023
Merged

Unify function and nullaryFunction#258
shane-circuithub merged 1 commit intomasterfrom
function

Conversation

@shane-circuithub
Copy link
Copy Markdown
Contributor

This does away with the weird variadic arguments thing we had going on with function.

Functions with no arguments are now written as:

now :: Expr UTCTime
now = function "now" ()

Functions with multiple arguments are now written as:

quot :: Sql DBIntegral a => Expr a -> Expr a -> Expr a
quot n d = function "div" (n, d)

Single-argument functions are written exactly as before.

@shane-circuithub
Copy link
Copy Markdown
Contributor Author

@ocharles I know you're ambivalent about this but I figured I'd make a proper PR regardless. It's based on #257.

@shane-circuithub shane-circuithub requested a review from ocharles July 8, 2023 15:25
Copy link
Copy Markdown
Contributor

@ocharles ocharles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm overall on board with the idea (I was just in a bit of a foul mood yesterday 😅). That said, I think removing nullaryFunction is going a bit too far. If we have function and nullaryFunction we can get rid of the Arguments type class entirely, and things become even simpler IMO (fewer concepts to understand).

Alternatively, is it possible to just give () a Table instance? I haven't tried it, but looking at https://hackage.haskell.org/package/rel8-1.4.1.0/docs/Rel8.html#t:Table it feels like it might be possible? Columns is just a constant functor, but maybe the context stuff causes it to fall apart.

This also needs a changelog entry.

@shane-circuithub
Copy link
Copy Markdown
Contributor Author

I knew you wouldn't like Arguments, but I really don't think it's too onerous to "understand". I think if tuples are used for multiple arguments it's pretty natural to expect () to work for zero arguments. I'm also thinking about queryFunction here, which I will rebase on top of this. Without Arguments we will need a nullaryQueryFunction too. Arguments, function and queryFunction is less "things" (3) than function, nullaryFunction, queryFunction and nullaryQueryFunction (4).

@shane-circuithub
Copy link
Copy Markdown
Contributor Author

And yeah, it's not really possible to make () a Table, for two reasons. The first one is, as you said, what Context is it in? If we want to make it work with function as above, then could arbitrarily assign it the Expr context, but the deeper problem is that it doesn't contain any Exprs. Columns has to be a HTable, and a t Expr (where t is a HTable) must contain actual Exprs, because HTable's htraverse method has an Apply constraint (not Applicative), so a HTable can't be empty.

@ocharles
Copy link
Copy Markdown
Contributor

ocharles commented Jul 9, 2023

And yeah, it's not really possible to make () a Table, for two reasons. The first one is, as you said, what Context is it in?

My thinking was actually that () is in all contexts, but that's not possible because of the fun dep on Table (a -> context), so that's a completely non-starter.

I knew you wouldn't like Arguments, but I really don't think it's too onerous to "understand".

I just don't like type classes for this kind of overloading because it's tedious to interactively understand. If you're using holes, a _ at the point of arguments will give you a useless hole ("I need an a"), and if you're in Haddock you still have to do some left-to-right parsing to get to the argument, then back to the left to find the constraint, then find the type class, then hunt down instances for it. The openness of a type class just throws everything around everywhere. Of course it's not a lot to understand and the names are intuitive, but I still think it's asking more than zero from a new user (we will of course never get to zero, but I want to get as close to it as possible)

I'm also thinking about #241 here, which I will rebase on top of this.

Thanks, of course I wasn't quite aware of that based on what was in this PR, but that does explode the API more (though it at least still has some symmetry).


I'll present just one more alternative, which is again to ditch the type class, but to replace with a simple sum-type:

function :: Sql DBType a => QualifiedName -> Arguments a -> Expr b

data Arguments a where
  NoArguments :: Arguments ()
  Arguments :: Table Expr a => a -> Arguments a

(I wouldn't go further than this. E.g., I wouldn't go towards a fullblown HList or anything).

It's a tiny bit more work to use:

rem :: Sql DBIntegral a => Expr a -> Expr a -> Expr a
rem n d = function "rem" $ Argument (n, d)

now :: Expr UTCTime
now = function "now" NoArguments

But I think it's absolutely obvious what's going on.

With this I think we've exhausted all options:

  1. Variadic arguments (status quo)
  2. Type class overloading
  3. Separate nullary/n-ary functions
  4. A GADT to choose between nullary/n-ary.

My vote is the GADT as everything becomes very self-contained, but I'll leave it with you to pick which you think is best as my +1 is fairly weak. I think any of 2/3/4 improve the current situation.

@shane-circuithub
Copy link
Copy Markdown
Contributor Author

shane-circuithub commented Jul 11, 2023

I prefer 2 to 3 or 4.

In PostgreSQL, a function is just a function, whether it takes zero or more arguments. A nullaryFunction is not a separate type of thing to a function, they're all just functions. The only reason we had nullaryFunction as a separate thing was that the implementation of variadic arguments that function used didn't allow a way to supply zero arguments. I don't think it's a meaningful distinction or one we should seek to preserve.

As for the GADT approach, I understand your point about typed holes, but it feels pretty weak here. There's not much that GHC can tell you about what argument to supply to function anyway, because it depends on what Postgres function is being called and GHC doesn't know anything about that. In any case, function "foo" _ in GHCi results in two different errors, but one of them is the following:

ghci>  Rel8.function "foo" _

<interactive>:3:2: error:
     Overlapping instances for Arguments arguments0
        arising from a use of function
      Matching instance:
        instance Table Expr a => Arguments a
          -- Defined at src/Rel8/Expr/Function.hs:43:10
      Potentially matching instance:
        instance [overlap ok] Arguments ()
          -- Defined at src/Rel8/Expr/Function.hs:47:27
      (The choice depends on the instantiation of arguments0
       To pick the first instance above, use IncoherentInstances
       when compiling the other instance declarations)
     In the expression: function "foo" _
      In an equation for it’: it = function "foo" _

Maybe it's a little bit confusing and indirect but GHC is basically saying "you can either put a Table Expr here or ()", which is about as much as it can possibly say. But again, I don't think typed holes are ever going to be very useful here due to the nature of function being deliberately maximally polymorphic.

Really though, I just have really hard time believing that even you would rather type function "now" NoArguments and function "div" (Arguments (n, d)) than function "now" () or function "div" (n, d). Maybe it's slightly easier for someone using typed holes to discover what to do with function, but not by much I don't think. But even then, you only discover how to use function once, but you use it many times. Having to import and type NoArguments or Arguments every time thereafter feels like an unreasonable punishment on anyone that already knows how to use function.

@ocharles
Copy link
Copy Markdown
Contributor

Ah, I didn't remember that GHC told us about potential instances. That does weaken my argument about typed holes.

@shane-circuithub shane-circuithub force-pushed the QualifiedName branch 3 times, most recently from c0e44bd to e65d852 Compare July 11, 2023 12:03
Base automatically changed from QualifiedName to master July 11, 2023 12:06
This does away with the weird variadic arguments thing we had going on with `function`.

Functions with no arguments are now written as:

```haskell
now :: Expr UTCTime
now = function "now" ()
```

Functions with multiple arguments are now written as:

```haskell
quot :: Sql DBIntegral a => Expr a -> Expr a -> Expr a
quot n d = function "div" (n, d)
```

Single-argument functions are written exactly as before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants