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

Allow fields to occur in multiple constructors #2

Merged
merged 1 commit into from
Jul 25, 2016

Conversation

andy-morris
Copy link
Contributor

@andy-morris andy-morris commented Jul 24, 2016

Currently, if multiple constructors of a datatype have a field, then multiple copies of the instances are generated.

For example:

{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE MultiParamTypeClasses #-}
module E where

import Data.OverloadedRecords.TH
import Data.Default.Class

data Foo = A {_size :: Int} | B {_size :: Int}

pure []
overloadedRecord def ''Foo

E.hs:15:1: error:
    Duplicate instance declarations:
      instance Data.OverloadedRecords.ModifyField "size" Foo Foo Int Int
        -- Defined at E.hs:15:1
      instance Data.OverloadedRecords.ModifyField "size" Foo Foo Int Int
        -- Defined at E.hs:15:1

E.hs:15:1: error:
    Duplicate instance declarations:
      instance Data.OverloadedRecords.HasField "size" Foo Int
        -- Defined at E.hs:15:1
      instance Data.OverloadedRecords.HasField "size" Foo Int
        -- Defined at E.hs:15:1

This patch fixes this, by keeping track of the instances already made. While doing this, it also checks that multiple fields don't map to the same thing. In the above example, if the constructors instead had fields aSize and bSize (both mapping to HasField "size" ...), then that would be an error, even though they have the same type, since it may lead to surprising behaviour.

A possible alternative approach could be extending DeriveOverloadedRecordsParams to specify what to do in case of conflicts. But just bailing is obviously much simpler and probably(?) usually the right answer, so that's what I went for.

@trskop
Copy link
Owner

trskop commented Jul 25, 2016

Thanks.

@trskop trskop merged commit 9602be2 into trskop:master Jul 25, 2016
trskop added a commit that referenced this pull request Jul 25, 2016
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.

None yet

2 participants