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

WIP: Rewrite how arguments are passed #33

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

WIP: Rewrite how arguments are passed #33

wants to merge 7 commits into from

Conversation

ocharles
Copy link
Member

@ocharles ocharles commented Feb 6, 2021

We're not passing all possible arguments to dear-imgui functions. This is my proposal of something lightweight.

@sheaf
Copy link
Contributor

sheaf commented Feb 6, 2021

I agree that we need something that emulates the functionality of named parameters, but I find this approach to be rather syntactically heavy.

How are optional fields handled? For instance we have:

DragFloat4(const char* label, float v[4], float v_speed = 1.0f, float v_min = 0.0f, float v_max = 0.0f, const char* format = "%.3f", ImGuiSliderFlags flags = 0);

with this approach we have a record

data DragFloat4 =
  DragFloat4
    { label :: String
    , value :: StateVar (Float, Float, Float, Float)
    , speed, minValue, maxValue :: Float
    , format :: String
    , flags :: ImGuiSliderFlags
    }

now format, speed, minValue, maxValue and flags all have default values, and the others don't.

Generally, I think I would prefer keeping the basic bindings as is (as you argued for in the past), and then provide various convenience wrappers that handle optional arguments and named arguments in extra modules.

@ocharles
Copy link
Member Author

ocharles commented Feb 6, 2021

Usually I would supply a default DragFloat4 record and use record updating. The problem is as we use duplicate record fields that won't infer with type annotations :/

@sheaf
Copy link
Contributor

sheaf commented Feb 6, 2021

But you'd want a type error if the user doesn't pass in one of the required arguments, no? I don't think that there are sensible default records in general, there are only defaults for a subset of the fields.

@ocharles
Copy link
Member Author

ocharles commented Feb 6, 2021

Oh, right, yes. I wouldn't supply defaults for required arguments. This leaves us with:

  • defDragFloat4 :: String -> StateVar ImVec4 -> DragFloat4, so you have to supply the required arguments but the rest are filled in with defaults. Record update is messy here, as you end up with (defDragFloat4 foo bar) { x = y, u = v }, which I find a mess. It also has all the type inference problems due to DuplicateRecordFields
  • All parameters that can have a default can be wrapped in Default a (isomorphic to Maybe a). Then we would have dragFloat4 DragFloat4{ label = "xyz", value = someStateVar, speed = Default, minValue = Default, maxValue = Default, format = Default, flags = Default }. This is quite heavy, though.

It's that, or we're going to have to have two versions of every function with defaults.

Generally, I think I would prefer keeping the basic bindings as is (as you argued for in the past), and then provide various convenience wrappers that handle optional arguments and named arguments in extra modules.

Or maybe you have something else in mind here? Could you sketch out what that would look like for dragFloat4, which seems to be particularly pathological?

@sheaf
Copy link
Contributor

sheaf commented Feb 6, 2021

Or maybe you have something else in mind here?

Well my point was more that I can imagine several ways to go about it: using extensible records, using type classes, using overloaded labels, etc. So I thought it would be best not to commit to any one approach, unless we can find a clearly superior solution.

@ocharles
Copy link
Member Author

ocharles commented Feb 6, 2021

Ok, but we certainly need to do something. Will have a think. I'm quite reluctant to introduce anything that I'd describe as "novel". How about we have dragFloat4 as is in main and dragFloat4Full :: Drag4Float -> m () which is what's in this PR? dragFloat4 essentially calls dragFloat4Full and fills in the extra fields with defaults. That does seem to give us everything without too much commitment to any particular approach. It's a bit of duplication, but I can live with that.

@sheaf
Copy link
Contributor

sheaf commented Feb 7, 2021

I'm trying to whip something up with generic-lens, basically a customised version of the Subtype lens that works with explicitly labelled types instead of record field names.

I've managed to set it up with decent error messages IMO:

data Label ( lbl :: Symbol ) = Label
instance ( lbl' ~ lbl ) => IsLabel lbl ( Label lbl' ) where
  fromLabel = Label
data ( lbl :: Symbol ) := ( a :: Type ) = Label lbl := a

-- | 'project' a smaller type out from a larger one, discarding the rest.
class Project big small where
  project :: big -> small

-- [ Omitted: instances with custom type errors using generic-lens machinery ]

-------------
-- Examples

projOK1 :: ( "b" := Float, "c" := Int, "a" := Double ) -> ( "a" := Double, "b" := Float )
projOK1 = project

projOK2 :: ( "b" := Int, "a" := Float )
projOK2 = project ( #a := 17.7, #b := 9 )

projErr1 :: ( "b" := Float, "a" := Bool, "c" := Double ) -> ( "c" := Double, "b" := Int )
projErr1 = project
-- * 'project': no instance for
--       Project
--         ("b" := Float, "a" := Bool, "c" := Double)
--         ("c" := Double, "b" := Int)
--   The type being projected down is missing the following fields:
--     - #b := Int

projErr2 :: ( "b" := Float, "c" := Int, "a" := Bool ) -> ( "a" := Bool, "b" := Float, "d" := Int )
projErr2 = project
--  * 'project': no instance for
--        Project
--          ("b" := Float, "c" := Int, "a" := Bool)
--          ("a" := Bool, "b" := Float, "d" := Int)
--    The type being projected down is missing the following fields:
--      - #d := Int

projErr3 :: ( "b" := Int, "a" := Int )
projErr3 = project ( #a := 17.7, #b := 9 )
-- * No instance for (Fractional Int) arising from the literal `17.7'

projErr4 :: ( "a" := Bool, "b" := Int, "b" := Int ) -> ( "b" := Int, "b" := Int ) 
projErr4 = project
--  * 'project': no instance for
--        Project
--          ("a" := Bool, "b" := Int, "b" := Int) ("b" := Int, "b" := Int)
--    The following duplicate fields cause a problem:
--      - [ Both] #b := Int

Unfortunately I couldn't directly re-use Subtype as the inference doesn't constrain fields of the same name to have the same type, which would prevent the projOK2 example above from type-checking (because numeric literals are polymorphic).

The above handles projecting down the collection of given arguments to extract the required arguments. To handle optional arguments, there's a similar setup with the following typeclass

-- | 'inject' a smaller type into a large one, overriding the fields
-- in the larger type with those from the smaller type.
class Inject small big where
  inject :: small -> big -> big

With this setup, the type signatures would look like

dragFloat4
  :: forall args m req opt
  .  ( MonadIO m, Project args req, Inject args opt
     , req ~ ( "label" := String, "speed" := Float, "value" := StateVar ( Float, Float, Float, Float ) )
     , opt ~ ( "minValue" := Float, "maxValue" := Float, "format" := String, "flags" := ImGuiSliderFlags )
     )
  => args
  -> m Bool

and the syntax is quite nice:

dragFloat4 ( #label := "hello", #speed := 3.4, #value := valueRef )
dragFloat4 ( #label := "hello", #flags := ImGuiSliderFlags_Logarithmic, #speed := 3.4, #value := valueRef )

Any thoughts? I'll try to polish up the functionality and put it up as a library.

@chekoopa
Copy link
Contributor

RecordDotSyntax in GHC 9 enables nicest (and much anticipated) record access syntax. With it, the described approach is possible and very handy. Optional fields are usually handled through Maybe type (which is converted to null).

@ocharles
Copy link
Member Author

the described approach is possible and very hand

Do you mean the approach in my PR?

@chekoopa
Copy link
Contributor

Do you mean the approach in my PR?

Yes, like that. Though, it suits better for low-level bindings (#61), and concise shortcuts are still in demand.

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.

3 participants