LF: Factorize the logic for Ast lookup #9871

remyhaemmerle-da · 2021-06-01T12:52:41Z

I strongly advice to review in the following order:

language package
CompiledPackages, PureCompiledPackages, ConcurrentCompiledPackages
the rest

part of #9974

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.

CHANGELOG_BEGIN CHANGELOG_END

cocreature

Nice work, thank you!

daml-lf/language/src/main/scala/com/digitalasset/daml/lf/language/Interface.scala

daml-lf/language/src/main/scala/com/digitalasset/daml/lf/language/LookupError.scala

daml-lf/language/src/main/scala/com/digitalasset/daml/lf/language/Interface.scala

cocreature · 2021-06-02T06:37:44Z

daml-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/Converter.scala

@@ -162,7 +162,7 @@ object Converter {
  }

  private def fromAnyChoice(
-      lookupChoice: (Identifier, Name) => Either[String, TemplateChoiceSignature],
+      lookupChoice: (Identifier, Name) => Either[String, GenTemplateChoice[_]],


What’s up with this? Why do we need more than the signature now?

You actually cannot do more with GenTemplateChoice[_] that you can do with TemplateChoiceSignature = TemplateChoice[Unit].

It is a bit annoying here, but the change is due to Interface returning GenXxxx[_] and not XxxSignature.
There is basically two reason for that:

Interface does not have to work with Signature it is just a wrapper around GenPackage[_], so we can use it with package where value are substituted with Unit or not. (It is more flexible like).

The only place where PackageSignature is very usefull is inside CompiledPackageto be sure we do not store the values.

Even if we change the type of Interface to take PackageSignature instead of GenPackage[_] we still have some issue with pattern matching and type erasure in to return GenXxxx[Unit] in some of the lookup (e.g. lookupValue).

I am considering one of the two changes (for an upcomming PR)

remove all The XxxSignature type aliases (exception PackageSignature) which are now a pretty useless

redefined all the XxxSignature to be alias of GenXxx[_] and defined a use directly GenPackage[Unit] in CompiledPackage

I’m not really worried about being able to do more. What I’m a bit worried about is that we end up leaking memory. Part of the reason for the signature changes was that things like triggers and daml script really should not keep the AST in memory and relying on signatures made that pretty explicit. Now, it’s very unclear to me if that is still working correctly and that it will keep working.

If we need Interface in both cases, maybe it’s worth adding a type parameter to it so we can enforce that we use Unit in some places and don’t leak and _ where we don’t care or Value where we need it.

Just to be clear, I don’t think this is merge-blocking but at least something to think about and potentially address later.

Making Interface generic is quite a pain.
So I try again to make it use PackageSignature and everything went smooth this time, i.e. I did not have problem of type erasure in this case.
7b6818e

This definitively prevent the usage of lookup in the ScenarioLoader as explained below but I think it not worthwhile to generalize Interface for only one use case.

daml-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/ScriptF.scala

cocreature · 2021-06-02T06:46:44Z

ledger/sandbox-classic/src/main/scala/platform/sandbox/stores/ledger/ScenarioLoader.scala

-          case Right(x) =>
-            List((Ref.Identifier(packageId, scenarioQualName) -> x))
-          case Left(_) => List()
+        pkg.modules


why are we no longer using one of the lookup functions here?

because we need the value attached to the definition, Interface returns GenDValue[_] while here we need GenDValue[Expr].
It is actually the only place where we do this kind of lookup, so I am not sure it worthwhile to generalize Interface for this use case, specially because we do not use the error.

daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Compiler.scala

daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Speedy.scala

cocreature · 2021-06-02T17:44:35Z

ledger/ledger-api-test-tool-on-canton/BUILD.bazel

@@ -82,6 +82,7 @@ conformance_test(
        ",ParticipantPruningIT" +  # see "conformance-test-participant-pruning" below
        ",ConfigManagementServiceIT,LedgerConfigurationServiceIT" +  # dynamic config management not supported by Canton
        ",ClosedWorldIT" +  # Canton currently fails this test with a different error (missing namespace in "unallocated" party id)
+        ",CommandServiceIT,CommandSubmissionCompletionI" + # FIXME: canton should be updated with last SDK to produce proper output message for those tests to pass


Suggested change

",CommandServiceIT,CommandSubmissionCompletionI" + # FIXME: canton should be updated with last SDK to produce proper output message for those tests to pass

",CommandServiceIT,CommandSubmissionCompletionIT" + # FIXME: canton should be updated with last SDK to produce proper output message for those tests to pass

you will also need to fix the compat tests

remyhaemmerle-da · 2021-06-03T08:12:17Z

compatibility/bazel_tools/testing.bzl

+    {
+        "start": "1.0.0",
+        "platform_ranges": [
+            {
+                "end": "0.0.0",
+                "exclusions": [
+                    "CommandServiceIT:CSCreateAndBadExerciseChoice",
+                    "CommandSubmissionCompletionIT:CSCRefuseBadChoice",
+                ],
+            },
+        ],
+    },
+    {
+        "end": "1.14.0-snapshot.20210602.7086.0.f36f556b",
+        "platform_ranges": [
+            {
+                "start": "0.0.0",
+                "exclusions": [
+                    "CommandServiceIT:CSCreateAndBadExerciseChoice",
+                    "CommandSubmissionCompletionIT:CSCRefuseBadChoice",
+                ],
+            },
+        ],
+    },
+    {
+        "start": "0.0.0",
+        "platform_ranges": [
+            {
+                "end": "1.14.0-snapshot.20210602.7086.0.f36f556b",
+                "exclusions": [
+                    "CommandServiceIT:CSCreateAndBadExerciseChoice",
+                    "CommandSubmissionCompletionIT:CSCRefuseBadChoice",
+                ],
+            },
+        ],
+    },


@cocreature Does this make sens ?

remyhaemmerle-da · 2021-06-03T08:16:12Z

ledger/ledger-api-test-tool-on-canton/BUILD.bazel

@@ -82,6 +82,7 @@ conformance_test(
        ",ParticipantPruningIT" +  # see "conformance-test-participant-pruning" below
        ",ConfigManagementServiceIT,LedgerConfigurationServiceIT" +  # dynamic config management not supported by Canton
        ",ClosedWorldIT" +  # Canton currently fails this test with a different error (missing namespace in "unallocated" party id)
+        ",CommandServiceIT:CSCreateAndBadExerciseChoice,CommandSubmissionCompletionIT:CSCRefuseBadChoice" +  # FIXME: canton should be updated with last SDK to produce proper output message for those tests to pass


@oliverse-da Fyi, I inactivated 2 tests. This is due to a mismatch of the error messages because an change in the engine.

You should reactivate once you have updated canton with the next SDK.

compatibility/bazel_tools/testing.bzl

cocreature · 2021-06-03T09:22:24Z

compatibility/bazel_tools/testing.bzl

+    {
+        "start": "1.0.0",
+        "platform_ranges": [
+            {
+                "end": "0.0.0",
+                "exclusions": [
+                    "CommandServiceIT:CSCreateAndBadExerciseChoice",
+                    "CommandSubmissionCompletionIT:CSCRefuseBadChoice",
+                ],
+            },
+        ],
+    },


What’s up with this? This is excluding literally all versions.

compatibility/bazel_tools/testing.bzl

Co-authored-by: Moritz Kiefer <[email protected]>

compatibility/bazel_tools/testing.bzl

CHANGELOG_BEGIN CHANGELOG_END

@garyverhaegen-da

This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @garyverhaegen-da is in charge of this release. Commit log: ``` 3cb8d5c Deprecate use of divulged contracts (#9930) e9a2e2e Allow two different time formats as input for the metrics reporting interval (#9926) 1dc8472 Fix variable name (#9927) 4037b1c Add metrics to the http json service (#9923) bd26c43 LF: fix TransactionCoderSpec (#9922) 69ef624 Fix flaky InstrumentedSourceSpec test (#9921) cbcec32 LF: clean up the remaining Exception TODOs (#9913) 5374209 Add dependency proto3-suite for Setup.hs (#9917) c77324d skip fix_bazel_cache on forks (#9920) ddf93fc Address TODOs in JdbcIndexer (#9911) a0d3e80 Add missing telemetryContext instances in the ledger-api-client's Ctx (#9916) 1e009b4 Upgrade canton to 0.25.0-snapshot supporting lf-1.13 and enable/fix tests (#9915) db14536 LF: Freeze archive proto for LF 1.14 (#9912) 15f9dfb update compat versions for 1.14.0-snapshot.20210602.7086.0.f36f556b (#9900) b4247c1 [In-memory fan-out] BuffersUpdater implementation (#9858) 7e733c3 DPP-390 remove appendonly todos (#9910) 512f1fd LF: Preview version of LF 1.14 (#9906) 2da94c3 Add warning about using divulged contracts in pruning doc (#9883) 3dee3d0 LF: Fix encoder for Exception (#9904) b22c046 ledger-api-test-tool: Only wait for parties on participants under test. (#9903) b8c02d6 Bump perf test for for #9871 (#9902) 4660e57 Fix typoed participant name in ExceptionsIT (#9901) 316069d Postgres batching refinements (#9898) 7bc925e LF: Factorize the logic for AST lookup (#9871) 2fa250f DPP-387 Remove migration todos (#9896) 65f9395 Try to fix windows skipping again (#9893) 850c731 Include PackageMetadata in LF interface (#9892) 41266c3 Fix the formatting on the LF spec. (#9889) dc79830 DPP-393 Enable sandbox tests for the append-only schema (#9876) 3c83b77 Parallel Ingestion renames (#9894) 6e48034 Dpp 392 todo cleanup postgre batch (#9875) 08ed7b6 rotate release duty after 1.14.0-snapshot.20210601.7081.0.7d716e6d (#9878) 081c54b Revert "Try to unflake JSON API failure tests (#9855)" (#9887) df0f3ff maybe fix deleted release tags (#9891) 4b020dd Release ledger-api-bench-tool (#9890) d801914 Release new snapshot (#9885) 4a4dde0 Implemented JdbcLedgerDaoTransactionLogUpdatesSpec (#9813) ``` Changelog: ``` - [Ledger API] Use of divulged contracts in later transactions is deprecated. Divulged contracts were already incompatible with pruning. We are working on features to handle the cases currently covered by divulgence. - for applications which support the --metrics-reporter-interval cli option, these now support both the java and scala duration string format (e.g. PT1M30S and 1.5min) Small fixes to the doc [HTTP/JSON API] - metrics reporting can now be chosen via the command line option --metrics-reporter (currently hidden), valid options are console, csv://, graphite:// and prometheus:// - metrics reporting interval can also now be chosen via a command line option --metrics-reporting-interval (currently hidden) [ledger api] Add warning about incompatibility between pruning and divulged contracts to pruning docs. ``` CHANGELOG_BEGIN CHANGELOG_END

@garyverhaegen-da

This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @garyverhaegen-da is in charge of this release. Commit log: ``` 3cb8d5c Deprecate use of divulged contracts (#9930) e9a2e2e Allow two different time formats as input for the metrics reporting interval (#9926) 1dc8472 Fix variable name (#9927) 4037b1c Add metrics to the http json service (#9923) bd26c43 LF: fix TransactionCoderSpec (#9922) 69ef624 Fix flaky InstrumentedSourceSpec test (#9921) cbcec32 LF: clean up the remaining Exception TODOs (#9913) 5374209 Add dependency proto3-suite for Setup.hs (#9917) c77324d skip fix_bazel_cache on forks (#9920) ddf93fc Address TODOs in JdbcIndexer (#9911) a0d3e80 Add missing telemetryContext instances in the ledger-api-client's Ctx (#9916) 1e009b4 Upgrade canton to 0.25.0-snapshot supporting lf-1.13 and enable/fix tests (#9915) db14536 LF: Freeze archive proto for LF 1.14 (#9912) 15f9dfb update compat versions for 1.14.0-snapshot.20210602.7086.0.f36f556b (#9900) b4247c1 [In-memory fan-out] BuffersUpdater implementation (#9858) 7e733c3 DPP-390 remove appendonly todos (#9910) 512f1fd LF: Preview version of LF 1.14 (#9906) 2da94c3 Add warning about using divulged contracts in pruning doc (#9883) 3dee3d0 LF: Fix encoder for Exception (#9904) b22c046 ledger-api-test-tool: Only wait for parties on participants under test. (#9903) b8c02d6 Bump perf test for for #9871 (#9902) 4660e57 Fix typoed participant name in ExceptionsIT (#9901) 316069d Postgres batching refinements (#9898) 7bc925e LF: Factorize the logic for AST lookup (#9871) 2fa250f DPP-387 Remove migration todos (#9896) 65f9395 Try to fix windows skipping again (#9893) 850c731 Include PackageMetadata in LF interface (#9892) 41266c3 Fix the formatting on the LF spec. (#9889) dc79830 DPP-393 Enable sandbox tests for the append-only schema (#9876) 3c83b77 Parallel Ingestion renames (#9894) 6e48034 Dpp 392 todo cleanup postgre batch (#9875) 08ed7b6 rotate release duty after 1.14.0-snapshot.20210601.7081.0.7d716e6d (#9878) 081c54b Revert "Try to unflake JSON API failure tests (#9855)" (#9887) df0f3ff maybe fix deleted release tags (#9891) 4b020dd Release ledger-api-bench-tool (#9890) d801914 Release new snapshot (#9885) 4a4dde0 Implemented JdbcLedgerDaoTransactionLogUpdatesSpec (#9813) ``` Changelog: ``` - [Ledger API] Use of divulged contracts in later transactions is deprecated. Divulged contracts were already incompatible with pruning. We are working on features to handle the cases currently covered by divulgence. - for applications which support the --metrics-reporter-interval cli option, these now support both the java and scala duration string format (e.g. PT1M30S and 1.5min) Small fixes to the doc [HTTP/JSON API] - metrics reporting can now be chosen via the command line option --metrics-reporter (currently hidden), valid options are console, csv://, graphite:// and prometheus:// - metrics reporting interval can also now be chosen via a command line option --metrics-reporting-interval (currently hidden) [ledger api] Add warning about incompatibility between pruning and divulged contracts to pruning docs. ``` CHANGELOG_BEGIN CHANGELOG_END Co-authored-by: Azure Pipelines DAML Build <[email protected]>

remyhaemmerle-da force-pushed the remy-error branch 2 times, most recently from 7dac774 to e66cb43 Compare June 1, 2021 18:40

remyhaemmerle-da changed the title ~~wip~~ LF: Factorize the logic for Ast lookup Jun 1, 2021

remyhaemmerle-da force-pushed the remy-error branch from e66cb43 to 9ff0a27 Compare June 1, 2021 19:11

remyhaemmerle-da marked this pull request as ready for review June 1, 2021 19:13

remyhaemmerle-da requested review from aherrmann-da, cocreature, fabiotudone-da, gerolf-da, miklos-da, mziolekda, nmarton-da and rautenrieth-da as code owners June 1, 2021 19:13

remyhaemmerle-da requested a review from a user June 1, 2021 19:13

remyhaemmerle-da requested review from stefanobaghino-da and tudor-da as code owners June 1, 2021 19:13

LF: Factorize the logic for Ast lookup

e3e21f6

CHANGELOG_BEGIN CHANGELOG_END

remyhaemmerle-da force-pushed the remy-error branch from 9ff0a27 to e3e21f6 Compare June 2, 2021 06:54

cocreature approved these changes Jun 2, 2021

View reviewed changes

remyhaemmerle-da added 7 commits June 2, 2021 12:18

fix LF Adapater & TailCallTest

9131654

fix LookupError name space

385374b

make lookup result more generic

1ad9355

Add LookupError.Defintiom

01fd6d0

comment lookupTypeSyn and lookupDataType

f751e76

reformulate LookupError message

0b4699d

add TODO comment around handleLookup definitions

20b2bb9

remyhaemmerle-da requested a review from cocreature June 2, 2021 12:13

cocreature approved these changes Jun 2, 2021

View reviewed changes

remyhaemmerle-da added 2 commits June 2, 2021 15:22

use PackageSignature inside Interface

7b6818e

fix ledger

f02a475

remyhaemmerle-da added 5 commits June 2, 2021 16:09

fix for scala 2.12

d14d590

fix triggers

55a1db4

fix LF test

369ff82

fix triggers tests

98664ce

try to tweak canton conformance test

783887d

cocreature reviewed Jun 2, 2021

View reviewed changes

remyhaemmerle-da added 2 commits June 2, 2021 19:56

formatting

157eca7

fix codec-benchmark

5e12cd4

remyhaemmerle-da requested a review from garyverhaegen-da as a code owner June 2, 2021 20:17

try to fix compatibility test

a81eab3

remyhaemmerle-da force-pushed the remy-error branch from 023182f to a81eab3 Compare June 2, 2021 20:18

remyhaemmerle-da added 3 commits June 2, 2021 22:44

try a bit more

89f6990

a bit more try

1777cfc

fix

1552f96

remyhaemmerle-da commented Jun 3, 2021

View reviewed changes

cocreature reviewed Jun 3, 2021

View reviewed changes

Apply suggestions from code review

94e2954

Co-authored-by: Moritz Kiefer <[email protected]>

remyhaemmerle-da commented Jun 3, 2021

View reviewed changes

compatibility/bazel_tools/testing.bzl Outdated Show resolved Hide resolved

remyhaemmerle-da and others added 3 commits June 3, 2021 11:46

Update compatibility/bazel_tools/testing.bzl

372d3b3

fix compatibility test exlusions

8a133be

fix

bdfe9d8

remyhaemmerle-da merged commit 7bc925e into main Jun 3, 2021

remyhaemmerle-da deleted the remy-error branch June 3, 2021 11:32

remyhaemmerle-da added a commit that referenced this pull request Jun 3, 2021

Bump perf test for for #9871

2a13450

CHANGELOG_BEGIN CHANGELOG_END

mergify bot pushed a commit that referenced this pull request Jun 3, 2021

Bump perf test for for #9871 (#9902)

b8c02d6

CHANGELOG_BEGIN CHANGELOG_END

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LF: Factorize the logic for Ast lookup #9871

LF: Factorize the logic for Ast lookup #9871

remyhaemmerle-da commented Jun 1, 2021 •

edited

Loading

cocreature left a comment

cocreature Jun 2, 2021

remyhaemmerle-da Jun 2, 2021

cocreature Jun 2, 2021

cocreature Jun 2, 2021

remyhaemmerle-da Jun 2, 2021 •

edited

Loading

cocreature Jun 2, 2021

remyhaemmerle-da Jun 2, 2021 •

edited

Loading

cocreature Jun 2, 2021

remyhaemmerle-da Jun 3, 2021

remyhaemmerle-da Jun 3, 2021

remyhaemmerle-da Jun 3, 2021 •

edited

Loading

cocreature Jun 3, 2021

	",CommandServiceIT,CommandSubmissionCompletionI" + # FIXME: canton should be updated with last SDK to produce proper output message for those tests to pass
	",CommandServiceIT,CommandSubmissionCompletionIT" + # FIXME: canton should be updated with last SDK to produce proper output message for those tests to pass

LF: Factorize the logic for Ast lookup #9871

LF: Factorize the logic for Ast lookup #9871

Conversation

remyhaemmerle-da commented Jun 1, 2021 • edited 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remyhaemmerle-da Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remyhaemmerle-da Jun 2, 2021 • edited Loading

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

remyhaemmerle-da Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remyhaemmerle-da commented Jun 1, 2021 •

edited

Loading

remyhaemmerle-da Jun 2, 2021 •

edited

Loading

remyhaemmerle-da Jun 2, 2021 •

edited

Loading

remyhaemmerle-da Jun 3, 2021 •

edited

Loading