Dysfunctional Dependencies (Trac #8634)
Needs RevisionPublic

Authored by wojciech.danilo on Jul 14 2014, 9:13 AM.

Details

Reviewers
simonpj
austin
Trac Issues
#8634
Summary

Signed-off-by: Wojciech Danilo <wojtek.danilo@gmail.com>

mend

Test Plan

To test this behaviour we can just compile following minimal program:

{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE FunctionalDependencies #-}
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE DysfunctionalDependencies #-}

class CTest a b | a -> b where

ctest :: a -> b

data X = X

instance Monad m => CTest X (m Int) where

ctest _ = return 5

main = print (ctest X :: [Int])

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
dysfunctionalDependencies
Lint
Lint OK
SeverityLocationCodeMessage
Auto-Fixdocs/users_guide/glasgow_exts.xml:8631TXT6Trailing Whitespace
Unit
No Unit Test Coverage
Build Status
Buildable 21
Build 21: GHC Patch Validation (amd64/Linux)
wojciech.danilo retitled this revision from to implementation of ticket #8634.Jul 14 2014, 9:13 AM
wojciech.danilo updated this object.
wojciech.danilo edited the test plan for this revision. (Show Details)
wojciech.danilo added reviewers: austin, simonpj.

Nothing in the user manual! I don't yet know what this patch is trying to achieve. What is the specification of which this is the implementation?

Simon

Whoops, Build D69/B20: Diff 153 has failed! Full logs available at F3501. The testsuite summary sez:

testsuite_summary.txt
Unexpected results from:
TEST="T9203 T4437"

OVERALL SUMMARY for test run started at Mon Jul 14 14:42:05 2014 UTC
 0:06:20 spent to go through
    4040 total tests, which gave rise to
   13341 test cases, of which
    9642 were skipped

      26 had missing libraries
    3611 expected passes
      60 expected failures

       0 caused framework failures
       0 unexpected passes
       2 unexpected failures

Unexpected failures:
   driver           T4437 [bad stdout] (normal)
   perf/should_run  T9203 [stat not good enough] (normal)
austin requested changes to this revision.Jul 14 2014, 10:22 AM
  • Needs a user manual entry
  • Needs tests before it can go upstream, to make sure it doesn't break.

Also, note that @phaskell caught a test failure:

+GHC-only flags: Unexpected flags
+-----
+DysfunctionalDependencies
+-----
+
*** unexpected failure for T4437(normal)

Please update the expected output for T4437.

The failure for T9203 seems unrelated.

This revision now requires changes to proceed.Jul 14 2014, 10:23 AM
In D69#4, @simonpj wrote:

Nothing in the user manual! I don't yet know what this patch is trying to achieve. What is the specification of which this is the implementation?

Simon

Hello! This patch is resolving the issue described here (according to the solution proposed by @goldfire, see the comments on the bottom of the trac site): https://ghc.haskell.org/trac/ghc/ticket/8634 .
I will add manual entry and fix the tests as fast as possible :)

I've just commented on the Trac page. Short version: I'm not convinced that this idea is type-safe.

kgadek added a subscriber: kgadek.Jul 14 2014, 1:48 PM
wojciech.danilo added a comment.EditedJul 14 2014, 2:02 PM
In D69#13, @goldfire wrote:

I've just commented on the Trac page. Short version: I'm not convinced that this idea is type-safe.

I appologize @goldfire If my comment was not appropriate. I didn't wanted it to look like convincing anybody to this idea, etc :)

I'm very attracted to the idea, that we would be able to throw away liberage coverage condition - in some places it could give interesting effects (like IncoherentInstances). But in opposite to IncoherentInstances, I've got a real-life code, which is right now working using DysfunctionalDependencies and is working great. I and some people from the company I'm working in were thinking for a long time if we can replace a very special type class instance with other mechanisms and we came to a conclusion, that using this extension would be a life saver for us - and this is the reason I started looking into ghc and implementing it. I would be very happy If you agree with me and would allow this extension to exist within GHC.

Anyway I hope @goldfire, that my comment didn't harm you in any way :)

  • Added documentation note and fixing test T4437 test.

Yay! Build D69/B21: Diff 154 has succeeded! Full logs available at F3547.

I don't see any regression tests! Please tests from the relevant tickets. (or boiled out from them; we don't want dependencies on libraries in the regression tests)

Simon

compiler/typecheck/TcValidity.lhs
884

Better to pass dynfundep_ok to checkInstCoverage, and deal with it there, so that there is just one place where the coverage condition is dealt with.

You then need to modify Note [Coverage conditions] in FunDeps to describe the behaviour, referring explicitly to Trac Trac #8634.

docs/users_guide/glasgow_exts.xml
4935

Rather than adding a "Please note" here, can you instead modify the description of the Coverage Condition (earlier in the section) to say precisely what it does. My remarks in Trac Trac #8634 give the general idea,.

4936

By all means then include an example or two to show the effect of the three different choices for the Coverage condition.

Otherwise OK. See my long remarks in Trac #8634.

ezyang retitled this revision from implementation of ticket #8634 to Dysfunctional Dependencies (Trac #8634).Jul 19 2014, 11:14 AM
austin updated the Trac tickets for this revision.Jul 22 2014, 8:01 AM
austin requested changes to this revision.Oct 17 2014, 7:08 PM

Pending all the discussion on this ticket, I'm currently blocking it until we come to a conclusion: re Trac #8634.

This revision now requires changes to proceed.Oct 17 2014, 7:08 PM

What's not concluded? It feels like we're in agreement here -- we just need the last bits to be done. Or am I missing something?

Yes, AFAICT, we just need a couple of regression tests, a bit more fleshed out docs and perhaps a name change of the extension, as suggested in the Trac ticket.

Is this being worked on?

Is this being worked on?

This weekend I will finish it finally. sorry for the delay, it was caused by too much work and lack of completely any "free" time. I would just like to know if should I make only regression tests for 7.8 or move the implementation over 7.10?

Move to HEAD! it's too late for 7.10.

Simon

This weekend I will finish it finally. sorry for the delay, it was caused by too much work and lack of completely any "free" time. I would just like to know if should I make only regression tests for 7.8 or move the implementation over 7.10?

Wojciech, I forgot to say: I would much, much prefer a per-instance pragma than a LANGUAGE extension; see comment:21 of https://ghc.haskell.org/trac/ghc/ticket/8634. We now do that for overlapping instances; see cid_overlap_mode in ClsInstDecl, so you can do it the same way.

Simon

In D69#18787, @simonpj wrote:

This weekend I will finish it finally. sorry for the delay, it was caused by too much work and lack of completely any "free" time. I would just like to know if should I make only regression tests for 7.8 or move the implementation over 7.10?

Wojciech, I forgot to say: I would much, much prefer a per-instance pragma than a LANGUAGE extension; see comment:21 of https://ghc.haskell.org/trac/ghc/ticket/8634. We now do that for overlapping instances; see cid_overlap_mode in ClsInstDecl, so you can do it the same way.

Simon

Simon I was looking for the code and I think it would be of course possible. For now I moved it to head as LANGUAGE pragma (to see if it works and behaves well) and I will be working on doing it the per-instance pragma instead. I just need another weekend. I will report my progress.
For now my time is very limited, but if I will not handle it late after hours, it could take forever.

Simon I was looking for the code and I think it would be of course possible. For now I moved it to head as LANGUAGE pragma (to see if it works and behaves well) and I will be working on doing it the per-instance pragma instead. I just need another weekend. I will report my progress.
For now my time is very limited, but if I will not handle it late after hours, it could take forever.

Fine. So we'll just stand by for the per-instance pragma. Thanks!

Simon

In D69#30479, @bgamari wrote:

@wojciech.danilo, any update on this?

Because we do not longer use this extension in our main codebase I had to do ton of other stuff and I didn't moved it yet to the new Haskell syntax, but another guy from our company moved it in the current state (with global -XDysfunctionalDependencies pragma to GHC 7.10). I can ask him to upload it here, but Simon wanted it to be in the new per-instance form, so it is not yet developed so. Because of the amount of work it is hard to me to find time to now move it to the new syntax though :(