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: Fix code generation for HKD records via plugin #34

Merged
merged 16 commits into from Oct 1, 2020
51 changes: 51 additions & 0 deletions examples/HkdPlugin.hs
@@ -0,0 +1,51 @@
-- Test for everything that is supported by both the plugin and the preprocessor

{-# OPTIONS_GHC -Werror -Wall -Wno-type-defaults -Wno-partial-type-signatures -Wincomplete-record-updates -Wno-unused-top-binds #-} -- can we produce -Wall clean code
{-# LANGUAGE PartialTypeSignatures, GADTs, StandaloneDeriving, DataKinds, KindSignatures, TypeFamilies #-} -- also tests we put language extensions before imports

import Data.Functor.Identity
import Control.Monad (unless)
import Database.Beam
import qualified Database.Beam as Beam

-- ---------------------------------------------------------------------
-- Deal with HKD

data Foo f = Foo {
bar :: C f Int,
baz :: String
}

data Foo1 f = Foo1 {
bar1 :: Beam.C f Int,
baz1 :: String
}

data Foo2 f = Foo2 {
bar2 :: C (Nullable f) Int,
baz2 :: String
}

(===) :: (Show a, Eq a) => a -> a -> IO ()
a === b = unless (a == b) (fail $ "Mismatch, " ++ show a ++ " /= " ++ show b)

main :: IO ()
main = test1 >> putStrLn "All worked"

tstObj :: Foo Identity
tstObj = Foo 1 "test"

tstObj1 :: Foo1 Identity
tstObj1 = Foo1 1 "test"

tstObj2 :: Foo2 Identity
tstObj2 = Foo2 (Just 1) "test"

test1 :: IO ()
test1 = do
tstObj.baz === "test"
tstObj.bar === 1
tstObj1.baz1 === "test"
tstObj1.bar1 === 1
tstObj2.baz2 === "test"
tstObj2.bar2 === Just 1
43 changes: 33 additions & 10 deletions plugin/RecordDotPreprocessor.hs
@@ -1,4 +1,4 @@
{-# LANGUAGE RecordWildCards, ViewPatterns, NamedFieldPuns #-}
{-# LANGUAGE RecordWildCards, ViewPatterns, NamedFieldPuns, PatternSynonyms, OverloadedStrings #-}
{- HLINT ignore "Use camelCase" -}

-- | Module containing the plugin.
Expand All @@ -11,10 +11,10 @@ import Compat
import Bag
import qualified GHC
import qualified GhcPlugins as GHC
import qualified PrelNames as GHC
import SrcLoc
import TcEvidence


---------------------------------------------------------------------
-- PLUGIN WRAPPER

Expand Down Expand Up @@ -52,22 +52,25 @@ onModule x = x { hsmodImports = onImports $ hsmodImports x
onImports :: [LImportDecl GhcPs] -> [LImportDecl GhcPs]
onImports = (:) $ qualifiedImplicitImport mod_records


{-
instance Z.HasField "name" (Company) (String) where hasField _r = (\_x -> _r{name=_x}, (name:: (Company) -> String) _r)

instance HasField "selector" Record Field where
hasField r = (\x -> r{selector=x}, (name :: Record -> Field) r)
-}
instanceTemplate :: FieldOcc GhcPs -> HsType GhcPs -> HsType GhcPs -> InstDecl GhcPs
instanceTemplate selector record field = ClsInstD noE $ ClsInstDecl noE (HsIB noE typ) (unitBag has) [] [] [] Nothing
instanceTemplate selector record field = instance'
where
typ = mkHsAppTys
(noL (HsTyVar noE GHC.NotPromoted (noL var_HasField)))
[noL (HsTyLit noE (HsStrTy GHC.NoSourceText (GHC.occNameFS $ GHC.occName $ unLoc $ rdrNameFieldOcc selector)))
,noL record
,noL field
]
instance' = ClsInstD noE $ ClsInstDecl noE (HsIB noE typ) (unitBag has) [] [] [] Nothing

typ' a = mkHsAppTys
(noL (HsTyVar noE GHC.NotPromoted (noL var_HasField)))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep 4 space indents please.

[noL (HsTyLit noE (HsStrTy GHC.NoSourceText (GHC.occNameFS $ GHC.occName $ unLoc $ rdrNameFieldOcc selector)))
,noL record
,noL a
]

typ = noL $ makeEqQualTy field (unLoc . typ')

has :: LHsBindLR GhcPs GhcPs
has = noL $ FunBind noE (noL var_hasField) (mg1 eqn) WpHole []
Expand Down Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

makeEqQualTy rArg fAbs = let
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var = GHC.nameRdrName $ GHC.mkUnboundName $ GHC.mkTyVarOcc "aplg"

tyVar :: HsType GhcPs
tyVar = HsTyVar noE GHC.NotPromoted (noL var)

var_tilde = GHC.mkOrig GHC.gHC_TYPES $ GHC.mkClsOcc "~"

eqQual :: HsType GhcPs
eqQual = HsOpTy noE (noL rArg) (noLoc var_tilde) (noLoc tyVar)

qualCtx :: HsContext GhcPs
qualCtx = [noLoc (HsParTy noE (noLoc eqQual))]

qualType :: HsType GhcPs
qualType = HsQualTy noE (noL qualCtx) (noL (fAbs tyVar))
in qualType
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include a trailing newline for the while.

4 changes: 3 additions & 1 deletion record-dot-preprocessor.cabal
Expand Up @@ -69,12 +69,14 @@ test-suite record-dot-preprocessor-test
base == 4.*,
extra,
record-hasfield,
filepath
filepath,
beam-core
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@neongreen neongreen Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Preprocessor
Edit
Lexer
Expand Down
4 changes: 2 additions & 2 deletions test/PluginExample.hs
Expand Up @@ -16,9 +16,9 @@ main = pure ()
#else

{-# OPTIONS_GHC -fplugin=RecordDotPreprocessor #-}
{-# LANGUAGE DuplicateRecordFields, TypeApplications, FlexibleContexts, DataKinds, MultiParamTypeClasses, TypeSynonymInstances, FlexibleInstances #-}
{-# LANGUAGE DuplicateRecordFields, TypeApplications, FlexibleContexts, DataKinds, MultiParamTypeClasses, TypeSynonymInstances, FlexibleInstances, TypeFamilies, TypeOperators, GADTs, UndecidableInstances #-}
-- 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 #-}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 _)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it works after pulling from upstream master

{-# LANGUAGE PartialTypeSignatures, GADTs, StandaloneDeriving, KindSignatures #-}
module PluginExample where
#include "../examples/Both.hs"
Expand Down
26 changes: 26 additions & 0 deletions test/PluginHkdExample.hs
@@ -0,0 +1,26 @@
{-# LANGUAGE CPP #-}

#if __GLASGOW_HASKELL__ < 806

module PluginHkdExample where
main :: IO ()
main = pure ()

#elif mingw32_HOST_OS

module PluginHkdExample where
import RecordDotPreprocessor () -- To check the plugin compiles
main :: IO ()
main = pure ()

#else

{-# OPTIONS_GHC -fplugin=RecordDotPreprocessor #-}
{-# LANGUAGE DuplicateRecordFields, TypeApplications, FlexibleContexts, DataKinds, MultiParamTypeClasses, TypeSynonymInstances, FlexibleInstances, TypeFamilies, TypeOperators, GADTs, UndecidableInstances #-}
-- things that are now treated as comments
{-# OPTIONS_GHC -Wall -Wno-type-defaults -Wno-partial-type-signatures -Wincomplete-record-updates -Wno-unused-top-binds #-}
{-# LANGUAGE PartialTypeSignatures, GADTs, StandaloneDeriving, KindSignatures #-}
module PluginHkdExample where
#include "../examples/HkdPlugin.hs"

#endif
8 changes: 6 additions & 2 deletions test/Test.hs
Expand Up @@ -3,6 +3,7 @@ module Test(main) where

import qualified Preprocessor
import qualified PluginExample -- To test the plugin
import qualified PluginHkdExample -- To test the plugin on hkd
import GHC.Records.Extra() -- To ensure the runtime dependency is present

import System.Directory.Extra
Expand All @@ -23,10 +24,13 @@ main = do
files <- listFiles "examples"
let installed = "--installed" `elem` args
unless installed $ do
putStrLn "# Plugin Example.hs"
putStrLn "# PluginExample.hs"
PluginExample.main
unless installed $ do
putStrLn "# PluginHkdExample.hs"
PluginHkdExample.main
forM_ (reverse files) $ \file ->
when (takeExtension file == ".hs" && not ("_out.hs" `isSuffixOf` file)) $
when (takeExtension file == ".hs" && not ("_out.hs" `isSuffixOf` file) && not("HkdPlugin.hs" `isSuffixOf` file)) $
if installed then do
src <- readFile' file
forM_ [("Preprocessor", "{-# OPTIONS_GHC -F -pgmF=record-dot-preprocessor #-}")
Expand Down