request for reviews for my first patch -- ticket 7401

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

request for reviews for my first patch -- ticket 7401

Ömer Sinan Ağacan
Hi all,

I just started GHC development made a commit for ticket 7401:
http://ghc.haskell.org/trac/ghc/ticket/7401

My patch is here:
https://github.com/osa1/ghc/commit/3ec257ff48372de30df59cd8854ce28954c9db95


`make test` succeeds. My test case for this patch is something like this:


    data D deriving Eq

    main :: IO ()
    main = print ((undefined :: D) == (undefined :: D))


example:


?  haskell  ./ghc/inplace/bin/ghc-stage2 --interactive derive.hs
GHCi, version 7.7.20130806: http://www.haskell.org/ghc/  :? for help
Loading package ghc-prim ... linking ... done.
Loading package integer-gmp ... linking ... done.
Loading package base ... linking ... done.
[1 of 1] Compiling Main             ( derive.hs, interpreted )
Ok, modules loaded: Main.
ghci> main
True
it :: ()


This behavior(returning True) is consistent with standalone deriving version:


    ?  haskell  cat derive_standalone.hs
    {-# LANGUAGE StandaloneDeriving #-}

    data D

    deriving instance Eq D

    main :: IO ()
    main = print ((undefined :: D) == (undefined :: D))

    ?  haskell  ./ghc/inplace/bin/ghc-stage2 --interactive derive_standalone.hs
    GHCi, version 7.7.20130806: http://www.haskell.org/ghc/  :? for help
    Loading package ghc-prim ... linking ... done.
    Loading package integer-gmp ... linking ... done.
    Loading package base ... linking ... done.
    [1 of 1] Compiling Main             ( derive_standalone.hs, interpreted )
    Ok, modules loaded: Main.
    ghci> main
    True
    it :: ()
    ghci>
    Leaving GHCi.



However, if you want (==) implementation in this case to be `error
"Void (==)`, I think I can also do that by first fixing the code
generated by StandaloneDeriving extension and then fixing my current
patch.



Any comments and reviews would be appreciated!


---
?mer Sinan A?acan
http://osa1.net



Reply | Threaded
Open this post in threaded view
|

request for reviews for my first patch -- ticket 7401

Edward Z. Yang
Hello Omer,

It is almost certainly the case that the implementation of these
functions should not be the constant return True function.  Furthermore,
the implementation should probably be done using:

    http://ghc.haskell.org/trac/ghc/ticket/2431

Edward

Excerpts from ?mer Sinan A?acan's message of Fri Aug 09 08:06:19 -0700 2013:

> Hi all,
>
> I just started GHC development made a commit for ticket 7401:
> http://ghc.haskell.org/trac/ghc/ticket/7401
>
> My patch is here:
> https://github.com/osa1/ghc/commit/3ec257ff48372de30df59cd8854ce28954c9db95
>
>
> `make test` succeeds. My test case for this patch is something like this:
>
>
>     data D deriving Eq
>
>     main :: IO ()
>     main = print ((undefined :: D) == (undefined :: D))
>
>
> example:
>
>
> ?  haskell  ./ghc/inplace/bin/ghc-stage2 --interactive derive.hs
> GHCi, version 7.7.20130806: http://www.haskell.org/ghc/  :? for help
> Loading package ghc-prim ... linking ... done.
> Loading package integer-gmp ... linking ... done.
> Loading package base ... linking ... done.
> [1 of 1] Compiling Main             ( derive.hs, interpreted )
> Ok, modules loaded: Main.
> ghci> main
> True
> it :: ()
>
>
> This behavior(returning True) is consistent with standalone deriving version:
>
>
>     ?  haskell  cat derive_standalone.hs
>     {-# LANGUAGE StandaloneDeriving #-}
>
>     data D
>
>     deriving instance Eq D
>
>     main :: IO ()
>     main = print ((undefined :: D) == (undefined :: D))
>
>     ?  haskell  ./ghc/inplace/bin/ghc-stage2 --interactive derive_standalone.hs
>     GHCi, version 7.7.20130806: http://www.haskell.org/ghc/  :? for help
>     Loading package ghc-prim ... linking ... done.
>     Loading package integer-gmp ... linking ... done.
>     Loading package base ... linking ... done.
>     [1 of 1] Compiling Main             ( derive_standalone.hs, interpreted )
>     Ok, modules loaded: Main.
>     ghci> main
>     True
>     it :: ()
>     ghci>
>     Leaving GHCi.
>
>
>
> However, if you want (==) implementation in this case to be `error
> "Void (==)`, I think I can also do that by first fixing the code
> generated by StandaloneDeriving extension and then fixing my current
> patch.
>
>
>
> Any comments and reviews would be appreciated!
>
>
> ---
> ?mer Sinan A?acan
> http://osa1.net
>



Reply | Threaded
Open this post in threaded view
|

request for reviews for my first patch -- ticket 7401

Ömer Sinan Ağacan
> It is almost certainly the case that the implementation of these
> functions should not be the constant return True function.

Yeah, I just checked that and looks like GHC generates `error $ Void ==`


    ==================== Derived instances ====================
    Derived instances:
      instance GHC.Classes.Eq Main.D where
        GHC.Classes.== = GHC.Err.error "Void =="
        GHC.Classes./= a_aok b_aol
          = GHC.Classes.not ((GHC.Classes.==) a_aok b_aol)

But I can't find code generation part of standalone deriving, any
ideas on where is this generation part?

---
?mer Sinan A?acan
http://osa1.net



Reply | Threaded
Open this post in threaded view
|

request for reviews for my first patch -- ticket 7401

Ömer Sinan Ağacan
Removing `null data_cons`(compiler/typecheck/TcDeriv.lhs 1105) check
for eq instance declarations fixes this, code generation part would be
same(changes in TcGenDeriv.hs were wrong and TcGenDeriv needs to be
unchanged)but I'm not sure how to remove that check only for eq
instance(Show, Ord, etc. sould still fail with same error message and
they're checked by same function, `cond_stdOK` -- line 1053).

---
?mer Sinan A?acan
http://osa1.net



Reply | Threaded
Open this post in threaded view
|

request for reviews for my first patch -- ticket 7401

Richard Eisenberg-2
Maybe it's best if Show, Ord, etc., echo the new behavior for Eq, if EmmptyDataDecls is specified. The reason for the check in cond_stdOK is to make sure that we're conforming to the Haskell standard. But, if the user has specified EmptyDataDecls, then we're not bound to that requirement anymore. So, to me, it seems reasonable that the "null data_cons" check should be omitted for any class if EmptyDataDecls is specified.

What do others think? This would go beyond the scope of the particular reported bug, but it seems to make sense to me.

Thanks,
Richard

On Aug 9, 2013, at 3:16 PM, ?mer Sinan A?acan wrote:

> Removing `null data_cons`(compiler/typecheck/TcDeriv.lhs 1105) check
> for eq instance declarations fixes this, code generation part would be
> same(changes in TcGenDeriv.hs were wrong and TcGenDeriv needs to be
> unchanged)but I'm not sure how to remove that check only for eq
> instance(Show, Ord, etc. sould still fail with same error message and
> they're checked by same function, `cond_stdOK` -- line 1053).
>
> ---
> ?mer Sinan A?acan
> http://osa1.net
>
> _______________________________________________
> ghc-devs mailing list
> ghc-devs at haskell.org
> http://www.haskell.org/mailman/listinfo/ghc-devs
>




Reply | Threaded
Open this post in threaded view
|

request for reviews for my first patch -- ticket 7401

Austin Seipp-4
Technically, EmptyDataDecls is part of Haskell 2010, so it is standard
these days (and H2010 is our default in GHC.)

As far as I know, the 2010 standard doesn't address this point about
deriving for empty data decls, but I agree with your reasoning here.
It's more consistent to have it apply to all of the base classes which
are derivable.

On Sun, Aug 11, 2013 at 9:14 PM, Richard Eisenberg <eir at cis.upenn.edu> wrote:

> Maybe it's best if Show, Ord, etc., echo the new behavior for Eq, if EmmptyDataDecls is specified. The reason for the check in cond_stdOK is to make sure that we're conforming to the Haskell standard. But, if the user has specified EmptyDataDecls, then we're not bound to that requirement anymore. So, to me, it seems reasonable that the "null data_cons" check should be omitted for any class if EmptyDataDecls is specified.
>
> What do others think? This would go beyond the scope of the particular reported bug, but it seems to make sense to me.
>
> Thanks,
> Richard
>
> On Aug 9, 2013, at 3:16 PM, ?mer Sinan A?acan wrote:
>
>> Removing `null data_cons`(compiler/typecheck/TcDeriv.lhs 1105) check
>> for eq instance declarations fixes this, code generation part would be
>> same(changes in TcGenDeriv.hs were wrong and TcGenDeriv needs to be
>> unchanged)but I'm not sure how to remove that check only for eq
>> instance(Show, Ord, etc. sould still fail with same error message and
>> they're checked by same function, `cond_stdOK` -- line 1053).
>>
>> ---
>> ?mer Sinan A?acan
>> http://osa1.net
>>
>> _______________________________________________
>> ghc-devs mailing list
>> ghc-devs at haskell.org
>> http://www.haskell.org/mailman/listinfo/ghc-devs
>>
>
>
> _______________________________________________
> ghc-devs mailing list
> ghc-devs at haskell.org
> http://www.haskell.org/mailman/listinfo/ghc-devs



--
Regards,
Austin - PGP: 4096R/0x91384671



Reply | Threaded
Open this post in threaded view
|

request for reviews for my first patch -- ticket 7401

Richard Eisenberg-2
According to the Haskell 2010 report (http://www.haskell.org/onlinereport/haskell2010/haskellch11.html#x18-18200011), a datatype with no constructors cannot derive any instances.

But, instead of creating a new extension for this feature, what about just co-opting EmptyDataDecls? More concretely, I propose this:

Under H98: EmptyDataDecls allows both the declaration of empty data decls and deriving instances for them.

Under H2010: EmptyDataDecls allows deriving instances for empty data decls.

This proposal brings the annoyance that H2010 no longer implies EmptyDataDecls.

Thoughts?

Richard

On Aug 11, 2013, at 10:28 PM, Austin Seipp wrote:

> Technically, EmptyDataDecls is part of Haskell 2010, so it is standard
> these days (and H2010 is our default in GHC.)
>
> As far as I know, the 2010 standard doesn't address this point about
> deriving for empty data decls, but I agree with your reasoning here.
> It's more consistent to have it apply to all of the base classes which
> are derivable.
>
> On Sun, Aug 11, 2013 at 9:14 PM, Richard Eisenberg <eir at cis.upenn.edu> wrote:
>> Maybe it's best if Show, Ord, etc., echo the new behavior for Eq, if EmmptyDataDecls is specified. The reason for the check in cond_stdOK is to make sure that we're conforming to the Haskell standard. But, if the user has specified EmptyDataDecls, then we're not bound to that requirement anymore. So, to me, it seems reasonable that the "null data_cons" check should be omitted for any class if EmptyDataDecls is specified.
>>
>> What do others think? This would go beyond the scope of the particular reported bug, but it seems to make sense to me.
>>
>> Thanks,
>> Richard
>>
>> On Aug 9, 2013, at 3:16 PM, ?mer Sinan A?acan wrote:
>>
>>> Removing `null data_cons`(compiler/typecheck/TcDeriv.lhs 1105) check
>>> for eq instance declarations fixes this, code generation part would be
>>> same(changes in TcGenDeriv.hs were wrong and TcGenDeriv needs to be
>>> unchanged)but I'm not sure how to remove that check only for eq
>>> instance(Show, Ord, etc. sould still fail with same error message and
>>> they're checked by same function, `cond_stdOK` -- line 1053).
>>>
>>> ---
>>> ?mer Sinan A?acan
>>> http://osa1.net
>>>
>>> _______________________________________________
>>> ghc-devs mailing list
>>> ghc-devs at haskell.org
>>> http://www.haskell.org/mailman/listinfo/ghc-devs
>>>
>>
>>
>> _______________________________________________
>> ghc-devs mailing list
>> ghc-devs at haskell.org
>> http://www.haskell.org/mailman/listinfo/ghc-devs
>
>
>
> --
> Regards,
> Austin - PGP: 4096R/0x91384671
>




Reply | Threaded
Open this post in threaded view
|

request for reviews for my first patch -- ticket 7401

Austin Seipp-4
On Sun, Aug 11, 2013 at 10:00 PM, Richard Eisenberg <eir at cis.upenn.edu> wrote:
> According to the Haskell 2010 report (http://www.haskell.org/onlinereport/haskell2010/haskellch11.html#x18-18200011), a datatype with no constructors cannot derive any instances.

You're quite right! I should have looked over the ticket first, where
Adam pointed this out.

> But, instead of creating a new extension for this feature, what about just co-opting EmptyDataDecls? More concretely, I propose this:
>
> Under H98: EmptyDataDecls allows both the declaration of empty data decls and deriving instances for them.
>
> Under H2010: EmptyDataDecls allows deriving instances for empty data decls.
>
> This proposal brings the annoyance that H2010 no longer implies EmptyDataDecls.
>
> Thoughts?
>
> Richard
>

IMHO, I'd find this inconsistency in extension behavior much more
annoying than just going against the standard on this note. But that's
just my 0.02c.

--
Regards,
Austin - PGP: 4096R/0x91384671



Reply | Threaded
Open this post in threaded view
|

request for reviews for my first patch -- ticket 7401

Richard Eisenberg-2
My proposal below doesn't really give different behavior for EmptyDataDecls in the two scenarios? the available constructs are the same under either H98 or H2010. It's just that the "distance" from the spec is different.

Personally, I'm loathe to stray from a well-defined note in a standard for this. I see my proposal below as a feasible solution to #7401, but I would actually favor not implementing any change here, because the workaround -- using standalone deriving -- is so easy and doesn't seem to have any real drawbacks (e.g. performance).

Richard

On Aug 12, 2013, at 12:55 AM, Austin Seipp wrote:

> On Sun, Aug 11, 2013 at 10:00 PM, Richard Eisenberg <eir at cis.upenn.edu> wrote:
>> But, instead of creating a new extension for this feature, what about just co-opting EmptyDataDecls? More concretely, I propose this:
>>
>> Under H98: EmptyDataDecls allows both the declaration of empty data decls and deriving instances for them.
>>
>> Under H2010: EmptyDataDecls allows deriving instances for empty data decls.
>>
>> This proposal brings the annoyance that H2010 no longer implies EmptyDataDecls.
>>
>> Thoughts?
>>
>> Richard
>>
>
> IMHO, I'd find this inconsistency in extension behavior much more
> annoying than just going against the standard on this note. But that's
> just my 0.02c.
>
> --
> Regards,
> Austin - PGP: 4096R/0x91384671