Converting functions in data dependencies. #4182

ghost · 2020-01-23T14:28:28Z

Now with tests!

changelog_begin
changelog_end

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

cocreature

Looks great, thanks for doing this so quickly! We should definitely add a test before merging. I would suggest that we use something like a template in DAML-LF 1.6 or 1.7 + top-level monomorphized create, archive and signatory functions. That provides a fairly user-friendly way of upgrading packages with templates from before the typeclass changes (so in particular anything on DAML-LF <= 1.7) so this is probably something that we’ll actually need.

cocreature · 2020-01-23T14:48:41Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/DataDependencies.hs

+    (noLoc $ HsVar noExt (mkRdrName "error"))
+    (mkStringLit msg)
+
+mkStringLit :: T.Text -> LHsExpr GhcPs


Judging from CI this seems to break probably due to OverloadedStrings, maybe try generating {-# LANGUAGE NoOverloadedStrings #-} or whatever the extension is called to disable this.

Trying it out. I spent way too much time looking for where to add this extension in HsModule ... turns out there was a plaintext header being added with the list of extensions. 😅

digitalasset-cla · 2020-01-24T10:43:25Z

All committers have signed the CLA.

cocreature · 2020-01-24T12:32:27Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/DataDependencies.hs

+        | (lfName, lfType) <- dvalBinder
+        , lfNameText <- LF.unExprValName lfName
+        = not (LF.getIsTest dvalIsTest)
+        && not (T.null lfNameText)


Do we actually generate definitions with an empty name? I thought that wasn’t even allowed in DAML-LF.

I removed this check anyway :) It's back to being

not ("$" `T.isPrefixOf` LF.unExprValName lfName)

Not yet tested! changelog_begin changelog_end

ghost · 2020-01-24T15:40:10Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/DataDependencies.hs

+        -- typeclasses, so we can assume there are no old-style
+        -- typeclasses being referenced. HOWEVER, we don't support
+        -- type synonyms here yet. TODO: Fix this, and change
+        -- the above to False.


Changing the line above to True causes us to not expose functions that rely on new-style typeclass constraints, as well. This is a temporary measure, and once we support new-style typeclasses in data-dependencies (in a separate PR) we can change it back to False.

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/DataDependencies.hs

ghost · 2020-01-25T15:06:55Z

Looks great, thanks for doing this so quickly! We should definitely add a test before merging. I would suggest that we use something like a template in DAML-LF 1.6 or 1.7 + top-level monomorphized create, archive and signatory functions. That provides a fairly user-friendly way of upgrading packages with templates from before the typeclass changes (so in particular anything on DAML-LF <= 1.7) so this is probably something that we’ll actually need.

I added a test based on this comment :-)

cocreature

Awesome, thanks a lot!

cocreature · 2020-01-27T09:27:37Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/DataDependencies.hs

+    shouldExposeDefValue :: LF.DefValue -> Bool
+    shouldExposeDefValue LF.DefValue{..}
+        | (lfName, lfType) <- dvalBinder
+        = not (LF.getIsTest dvalIsTest)


Is there actually a problem with exposing scenarios? I could see it being useful, e.g., if you have some function to initialize the state in scenarios you might want to use it across upgrades.

I'll expose them in a separate PR :)

cocreature · 2020-01-27T09:33:54Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/DataDependencies.hs

+defDataTypeIsOldTypeClass :: LF.DefDataType -> Bool
+defDataTypeIsOldTypeClass LF.DefDataType{..}
+    | LF.DataRecord fields <- dataCons
+    = notNull fields && all isDesugarField fields


Fun fact: We have typeclasses with no methods in daml-stdlib which this will not hit but it should be fine to not handle that and I don’t see a reasonable way of detecting those.

cocreature · 2020-01-27T09:36:15Z

compiler/damlc/lib/DA/Cli/Damlc.hs

+            stableDalfPkgMap <- useNoFile_ GenerateStablePackages
+            pure (dalfPkgMap, stableDalfPkgMap)
+
+        let allDalfPkgs =


Not necessary to address this in this PR but some type signatures for these maps might be nice.

ghost requested a review from cocreature January 23, 2020 14:28

ghost requested a review from hurryabit as a code owner January 23, 2020 14:28

cocreature reviewed Jan 23, 2020

View reviewed changes

cocreature reviewed Jan 24, 2020

View reviewed changes

associahedron added 12 commits January 24, 2020 15:28

Converting functions in data dependencies.

03fc4f6

Not yet tested! changelog_begin changelog_end

Add NoOverloadedStrings to the set of extensions in generated packages.

3fe21ab

Just use the fn as its own right hand side

df44156

Restore order of package map (not that it makes a difference)

d674cba

Adjust imports

df825e9

Weird lint but ok

3f5e9e8

Make the test pass somehow

5006f61

Dont preprocess enums in GHC.Prim

44bda80

Preprocess enums everywhere, and add mod ref as needed.

5db9acc

Revert preprocessor changes

3055d4a

Dont expose old-style typeclasses

7ebf6de

Dont convert newstyle typeclasses temporarily either

ed61993

ghost force-pushed the data-dep-fns branch from d653f45 to ed61993 Compare January 24, 2020 15:36

ghost commented Jan 24, 2020

View reviewed changes

ghost commented Jan 25, 2020

View reviewed changes

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/DataDependencies.hs Show resolved Hide resolved

Add test for function importing

06a85e2

cocreature approved these changes Jan 27, 2020

View reviewed changes

ghost merged commit 3c93b5e into master Jan 27, 2020

ghost deleted the data-dep-fns branch January 27, 2020 10:05

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converting functions in data dependencies. #4182

Converting functions in data dependencies. #4182

ghost commented Jan 23, 2020 •

edited by ghost

Loading

cocreature left a comment

cocreature Jan 23, 2020

ghost Jan 24, 2020

digitalasset-cla commented Jan 24, 2020 •

edited

Loading

cocreature Jan 24, 2020

ghost Jan 24, 2020

ghost Jan 24, 2020 •

edited by ghost

Loading

ghost commented Jan 25, 2020

cocreature left a comment

cocreature Jan 27, 2020

ghost Jan 27, 2020

cocreature Jan 27, 2020

cocreature Jan 27, 2020

Converting functions in data dependencies. #4182

Converting functions in data dependencies. #4182

Conversation

ghost commented Jan 23, 2020 • edited by ghost Loading

Pull Request Checklist

cocreature left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalasset-cla commented Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jan 24, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Jan 25, 2020

cocreature 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

ghost commented Jan 23, 2020 •

edited by ghost

Loading

digitalasset-cla commented Jan 24, 2020 •

edited

Loading

ghost Jan 24, 2020 •

edited by ghost

Loading