WIP: Fix code generation for HKD records via plugin #34

cattingcat · 2020-09-30T11:09:50Z

Currently we have a problem with records like this:

type family C where ...

data Foo f = Foo 
  { field :: C f String
  }

Because generated code looks like this:

instance HasField "field" (Foo f) (C f String) where ...

It is impossible to have type family invocation in instance types

So, we moved this invocation into context and generate code like this:

instance (a ~ C f String) => HasField "field" (Foo f) a where ...

And it works well, with UndecidableInstances

neongreen · 2020-09-30T11:19:03Z

I tried this out in a large project and it's working for me.

I think the change in this PR should be fine since there's a fundep x r -> a anyway and we are not preventing any other instances from existing, but I'm not 100% sure.

neongreen · 2020-09-30T11:19:28Z

This PR would fix #29.

neongreen · 2020-09-30T17:57:56Z

(a ~ C f String)

Put (C f String) into parentheses here — otherwise it will fail in cases like a ~ x := y with -XTypeOperators.

ndmitchell

A few little questions, but this looks very good. I note you've added this code to the plugin but NOT the preprocessor - is that deliberate? Do you think it can't work in the preprocessor? (If that's a divergence between the two, it will need documenting, as in most other ways they are equivalent)

ndmitchell · 2020-09-30T19:13:15Z

plugin/RecordDotPreprocessor.hs

+
+
+makeEqQualTy :: HsType GhcPs -> (HsType GhcPs -> HsType GhcPs) -> HsType GhcPs
+makeEqQualTy rArg fAbs = let


I usually prefer where over let like this - but I'm not too fussed.

ndmitchell · 2020-09-30T19:13:32Z

plugin/RecordDotPreprocessor.hs

+        instance' = ClsInstD noE $ ClsInstDecl noE (HsIB noE typ) (unitBag has) [] [] [] Nothing
+
+        typ' a = mkHsAppTys
+          (noL (HsTyVar noE GHC.NotPromoted (noL var_HasField)))


Can we keep 4 space indents please.

ndmitchell · 2020-09-30T19:14:57Z

plugin/RecordDotPreprocessor.hs

@@ -228,3 +231,23 @@ adjacentBy i (L (srcSpanEnd -> RealSrcLoc a) _) (L (srcSpanStart -> RealSrcLoc b
    srcLocLine a == srcLocLine b &&
    srcLocCol a + i == srcLocCol b
 adjacentBy _ _ _ = False
+
+
+makeEqQualTy :: HsType GhcPs -> (HsType GhcPs -> HsType GhcPs) -> HsType GhcPs


Can you include an example - e.g. Given t and f, it produces t ~ f aplg` or whatever (its super hard to read these syntax productions in the abstract)

ndmitchell · 2020-09-30T19:15:00Z

plugin/RecordDotPreprocessor.hs

+
+  qualType :: HsType GhcPs
+  qualType = HsQualTy noE (noL qualCtx) (noL (fAbs tyVar))
+  in qualType


Can we include a trailing newline for the while.

ndmitchell · 2020-09-30T19:15:50Z

record-dot-preprocessor.cabal

@@ -69,12 +69,14 @@ test-suite record-dot-preprocessor-test
        base == 4.*,
        extra,
        record-hasfield,
-        filepath
+        filepath,
+        beam-core


I'd rather avoid the beam dependency if we can. Are there no types in base that let us express similar problems and will fail in similar ways?

This PR is the result of a problem we had at @juspay internally, so right now references beam because it was the primary usecase — but if the approach seems good to you, the PR should definitely be polished and beam can be scrubbed out of it.

Same re/ plugin vs preprocessor. Agreed that both should be implemented.

Yep, I finisher preprocessor patching. But I have some problems with beam dependency removing. I have to add new library module with my own type family for testing. But as a result I have strange errors:

record-dot-preprocessor/utils/Utils.hs:1:1: error: File name does not match module name: Saw: ‘Main’ Expected: ‘Utils’

If someone have any idea how to add new module to this project structure, could you advice me

That sounds like you haven't written module Utils where at the top of the module. I'd still encourage you to look for something pre-installed with base - Const and WrappedMonad might work: https://hackage.haskell.org/package/base-4.14.0.0/docs/Control-Applicative.html#t:Const

Well, I tried with/without module name, but it doesn't work for now. I'll try more and write more specific question later :)

As about pre-installed base types - I don't think it is possible (probably I wrong) because the current version of the plugin should work well with constructions like

data MyData f = MyData {field :: f String} instance HasField "field" (MyData f) (f String) where ...

Because there are no type families in the instance type

So we need some type family instead of C from Beam, and it should be in a separate module for tests with qualified import. But, probably we can remove tests for qualified imports of C

I'd be tempted to just skip the qualified import tests - probably more hassle than they are worth. I think you do need the module header, but once you have that, I can imagine there are import path issues too.

@ndmitchell Well, I tried to make it as a lib few more times. It works well when I add a utility module with C type family to the plugin folder, because it is marked as library in cabal-file, I don't think it is correct approach. But I have problems when I tried to add it into tet folder, because of using system_ "runhaskell" on it. So, I decided to inlne this test-type-family into Both.hs. Please take a lok at result. I think I finished all review fixes

ndmitchell · 2020-09-30T19:16:39Z

record-dot-preprocessor.cabal

    if impl(ghc >= 8.6)
        build-depends:
            record-dot-preprocessor
    other-modules:
        PluginExample
+        PluginHkdExample


Why not add the plugin example to PluginExample? Why a specific module for this one?

ndmitchell · 2020-09-30T19:17:06Z

test/PluginExample.hs

 -- things that are now treated as comments
-{-# OPTIONS_GHC -Werror -Wall -Wno-type-defaults -Wno-partial-type-signatures -Wincomplete-record-updates -Wno-unused-top-binds #-}
+{-# OPTIONS_GHC -Wall -Wno-type-defaults -Wno-partial-type-signatures -Wincomplete-record-updates -Wno-unused-top-binds #-}


This has ended up as a conflict. Can you rebase over master? Why remove the -Werror?

Well, I had these problems with werror. I'll try to fix if it possible

record-dot-preprocessor/PluginExample:1:1: error: [-Wincomplete-record-updates, -Werror=incomplete-record-updates] Pattern match(es) are non-exhaustive In a record-update construct: Patterns not matched: (Foo8 _ _) record-dot-preprocessor/PluginExample:1:1: error: [-Wincomplete-record-updates, -Werror=incomplete-record-updates] Pattern match(es) are non-exhaustive In a record-update construct: Patterns not matched: (Quux8 _) record-dot-preprocessor/PluginExample:1:1: error: [-Wincomplete-record-updates, -Werror=incomplete-record-updates] Pattern match(es) are non-exhaustive In a record-update construct: Patterns not matched: (Quux8 _) record-dot-preprocessor/PluginExample:1:1: error: [-Wincomplete-record-updates, -Werror=incomplete-record-updates] Pattern match(es) are non-exhaustive In a record-update construct: Patterns not matched: (Nonhuman _)

Oh, it works after pulling from upstream master

ndmitchell · 2020-09-30T19:18:38Z

And thanks very much for the patch, it does look great.

…processor into simple-hkd-fix-2

ndmitchell · 2020-10-01T07:43:40Z

So fundamentally, yes, this approach sounds great - just a question of getting the code into shape and will definitely want to land it!

ndmitchell · 2020-10-01T12:51:52Z

Looks perfect. Not sure what's happening to the CI reports, but Travis seems to think it ran, so lets err on the side of optimism and merge it: https://travis-ci.org/github/ndmitchell/record-dot-preprocessor/builds/731921087

Many thanks! Are you planning any more changes in the near future? If not, I'll make a release.

neongreen · 2020-10-01T13:04:25Z

Hang on, let us try it internally and report back

neongreen · 2020-10-01T13:04:52Z

But likely no further changes for now

ndmitchell · 2020-10-01T13:23:45Z

Cool, will await your confirmation then release.

pkapustin · 2020-10-02T10:35:47Z

Could there be cases where this approach with the type equality constraint can lead to more complex error messages for simple cases?
From my testing, I haven't seen it. All complex error messages would come from the cases that somehow involve type families.

ndmitchell · 2020-10-02T11:02:46Z

Let me know if you see worse error messages - internally GHC does approximately the same transformation when type checking, so I'm hopeful it will only be worse when you use the extra power.

neongreen · 2020-10-02T12:03:24Z

Cool, will await your confirmation then release.

Looks good! Can release, I think

ndmitchell · 2020-10-02T12:34:50Z

Done and tweeted: https://twitter.com/ndm_haskell/status/1312008016183603201

Thanks for the patches!

pkapustin · 2020-10-02T18:57:16Z

Should we mention somewhere in the readme that UndecidableInstances will be needed?

ndmitchell · 2020-10-02T20:06:08Z

@pkapustin - seems reasonable - done in f6bba3b

pkapustin · 2020-10-05T09:28:13Z

Also, in addition to UndecidableInstances, the type equality constraint syntax requires either GADTs or TypeFamilies to be enabled, so I think we need to add this information to the part where the extensions needed to use the preprocessor are listed.

ndmitchell · 2020-10-05T18:09:47Z

@pkapustin is 23518a5 the change you think we need?

pkapustin · 2020-10-05T19:36:20Z

@ndmitchell Yes, I think this is great. Maybe I would be even more specific and say that you either need GADTs or TypeFamilies, because this is what is required to use the equality constraint syntax and because there are other effects to enabling those extensions, see https://gitlab.haskell.org/ghc/ghc/-/issues/10431.

ndmitchell · 2020-10-06T08:05:21Z

@pkapustin if we had EqualityConstraints I'd definitely suggest that. As it is, offering a choice, with a complicated set of criteria for making that choice, is probably worse than just picking one arbitrarily. I thought the biggest issue with either of these extensions is they turn on mono local binds, which can break code - but since they both turn on the same feature, the choice of which is less important. Or is there some case where you'd prefer one or the other - beyond actually using GADTs/TypeFamilies - which I'm a lot less concerned with (an extension you don't use isn't the end of the world for a preprocessor).

Mark Martynov and others added 10 commits August 18, 2020 00:49

Fix for simple HKD types with C type family

4089577

Add HKD support for plugin

97b0f49

Add real test with real Beam-core package

e6371b0

Fix qualified imports for Beam

9d684c9

fix type family detection problem

621e604

Add Nullable processing

cd4e5b7

Rewrite using ~

8818f9e

revert useless changes

7d02733

remove useless imports

6a123aa

Revert useless code of preprocessor

92c2c73

cattingcat changed the title ~~Fix code generation for HKD records via plugin~~ WIP: Fix code generation for HKD records via plugin Sep 30, 2020

ndmitchell reviewed Sep 30, 2020

View reviewed changes

Mark added 5 commits October 1, 2020 02:02

Add code generation to preprocessor

45561d8

review Fixes

b56c4a0

Merge branch 'master' of https://github.com/ndmitchell/record-dot-pre…

d8c4cd6

…processor into simple-hkd-fix-2

Additional () aroud possible tf application in preproc

bc75d5b

minor fixes

33db9c6

Inline C type family into test and remove beam-core dep

b697f4c

ndmitchell merged commit 16bc1dc into ndmitchell:master Oct 1, 2020

ndmitchell added a commit that referenced this pull request Oct 2, 2020

#34, mention UndecidableInstances

f6bba3b

neongreen mentioned this pull request Oct 30, 2020

GHC 8.10.2 incompatibility with preprocessor plugin #35

Closed



		makeEqQualTy :: HsType GhcPs -> (HsType GhcPs -> HsType GhcPs) -> HsType GhcPs
		makeEqQualTy rArg fAbs = let

WIP: Fix code generation for HKD records via plugin #34

WIP: Fix code generation for HKD records via plugin #34

Conversation

cattingcat commented Sep 30, 2020

neongreen commented Sep 30, 2020

neongreen commented Sep 30, 2020

neongreen commented Sep 30, 2020

ndmitchell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neongreen Sep 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndmitchell commented Sep 30, 2020

ndmitchell commented Oct 1, 2020

ndmitchell commented Oct 1, 2020

neongreen commented Oct 1, 2020

neongreen commented Oct 1, 2020

ndmitchell commented Oct 1, 2020

pkapustin commented Oct 2, 2020

ndmitchell commented Oct 2, 2020

neongreen commented Oct 2, 2020

ndmitchell commented Oct 2, 2020

pkapustin commented Oct 2, 2020

ndmitchell commented Oct 2, 2020

pkapustin commented Oct 5, 2020

ndmitchell commented Oct 5, 2020

pkapustin commented Oct 5, 2020

ndmitchell commented Oct 6, 2020

neongreen Sep 30, 2020 •

edited