LF: clean daml-lf/language module #11152

remyhaemmerle-da · 2021-10-06T19:45:32Z

enable NonUnitStatements, and find a useless statement.
add missing finals,
replace { ... } by ( ... ) for one statement anonymous function
drop useless new for case class.
replace sealed class X { ... object Y extend X by
class X private[Z] { ... val Y = new X to avoid unecessary class
alocation.
move invariant check inside the body of some case class instead of
the companion object.
other cosmetic changes.

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.

- enable NonUnitStatements, and find a useless statement. - add missing `final`s, - replace `{ ... }` by `( ... )` for one statement anonymous function - drop useless `new` for case class. - replace `sealed class X { ... object Y extend X` by `class X private[Z] { ... val Y = new X` to avoid unecessary class alocation. - move invariant check inside the body of some case class instead of the companion object. - other cosmetic changes. CHANGELOG_BEGIN CHANGELOG_END

remyhaemmerle-da · 2021-10-06T19:47:08Z

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

-  object TemplateKey extends GenTemplateKeyCompanion[Expr]
+  val TemplateKey = new GenTemplateKeyCompanion[Expr]

  type TemplateKeySignature = GenTemplateKey[Unit]
-  object TemplateKeySignature extends GenTemplateKeyCompanion[Unit]
+  val TemplateKeySignature = new GenTemplateKeyCompanion[Unit]


By using new instead of extends we avoid allocation of two extra java classes.

remyhaemmerle-da · 2021-10-06T19:48:29Z

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

@@ -686,9 +681,9 @@ object Ast {
      observers: E, // Observers of the contract.
      key: Option[GenTemplateKey[E]],
      implements: Map[TypeConName, GenTemplateImplements[E]],
-  ) extends NoCopy


There is no special invariant for this case class, so no reason to have a private constructor and prevent copy method.

remyhaemmerle-da · 2021-10-06T19:49:40Z

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

+  ) {
+    templates.keysIterator.foreach(name =>
+      if (exceptions.keySet.contains(name))
+        throw PackageError(s"Collision between exception and template name ${name.toString}")
+    )
+  }


Once putting the invariant check inside GenModule, we do not need the constructor to be private

Nice. Question: should there be more collision checks here? E.g. between templates and interfaces, exceptions and interfaces.

Oh actually, they can't collide anyway because Interface has a DataInterface as underlying, not a DataRecord.

remyhaemmerle-da · 2021-10-06T19:50:36Z

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

@@ -9,7 +9,7 @@ object StablePackages {
  // Based on compiler/damlc/tests/src/stable-packages.sh
  // TODO: Use this map to generate the mapping used in the test,
  // or generate both from a single source.
-  private val nameToIdMap = Map[PackageName, PackageId](
+  private[this] def nameToIdMap: Map[PackageName, PackageId] = Map(


This is used only once to create Ids, no need to keep it in memory.

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

sofiafaro-da

Nice, thank you!

sofiafaro-da · 2021-10-07T10:27:28Z

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

+  ) {
+    templates.keysIterator.foreach(name =>
+      if (exceptions.keySet.contains(name))
+        throw PackageError(s"Collision between exception and template name ${name.toString}")
+    )
+  }


Nice. Question: should there be more collision checks here? E.g. between templates and interfaces, exceptions and interfaces.

nickchapman-da

Thanks!

nickchapman-da · 2021-10-07T11:16:45Z

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

@@ -586,18 +584,18 @@ object Ast {
      isTest: Boolean,
  ) extends GenDefinition[E]

-  class GenDValueCompanion[E] {
+  final class GenDValueCompanion[E] private[Ast] {


What does private[Ast] mean/do here?

you cannot allocate a new GenDValueCompanion outside Ast object.

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

remyhaemmerle-da added the component/daml-engine DAML-LF Engine & Interpreter label Oct 6, 2021

remyhaemmerle-da requested a review from nickchapman-da October 6, 2021 19:45

remyhaemmerle-da requested a review from cocreature as a code owner October 6, 2021 19:45

remyhaemmerle-da commented Oct 6, 2021

View reviewed changes

daml-lf/language/src/main/scala/com/digitalasset/daml/lf/language/Ast.scala Show resolved Hide resolved

sofiafaro-da approved these changes Oct 7, 2021

View reviewed changes

nickchapman-da approved these changes Oct 7, 2021

View reviewed changes

remyhaemmerle-da merged commit dd233ef into main Oct 7, 2021

remyhaemmerle-da deleted the remy-lf-language-cleanup branch October 7, 2021 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LF: clean daml-lf/language module #11152

LF: clean daml-lf/language module #11152

remyhaemmerle-da commented Oct 6, 2021

remyhaemmerle-da Oct 6, 2021

remyhaemmerle-da Oct 6, 2021

remyhaemmerle-da Oct 6, 2021

sofiafaro-da Oct 7, 2021

sofiafaro-da Oct 7, 2021

remyhaemmerle-da Oct 6, 2021

sofiafaro-da left a comment

sofiafaro-da Oct 7, 2021

nickchapman-da left a comment

nickchapman-da Oct 7, 2021

remyhaemmerle-da Oct 7, 2021 •

edited

Loading

LF: clean daml-lf/language module #11152

LF: clean daml-lf/language module #11152

Conversation

remyhaemmerle-da commented Oct 6, 2021

Pull Request Checklist

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

sofiafaro-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickchapman-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remyhaemmerle-da Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

remyhaemmerle-da Oct 7, 2021 •

edited

Loading