LF: Log internal errors (#10471)

part of #9974 CHANGELOG_BEGIN CHANGELOG_END
digital-asset · Aug 4, 2021 · dc47b10 · dc47b10
1 parent 7bcec46
commit dc47b10
Show file tree

Hide file tree

Showing 15 changed files with 139 additions and 51 deletions.
diff --git a/daml-lf/archive/src/main/scala/com/digitalasset/daml/lf/archive/DecodeV1.scala b/daml-lf/archive/src/main/scala/com/digitalasset/daml/lf/archive/DecodeV1.scala
@@ -133,14 +133,15 @@ private[archive] class DecodeV1(minor: LV.Minor) {
     if (internedList.nonEmpty)
       assertSince(LV.Features.internedDottedNames, "interned dotted names table")
 
-    def outOfRange(id: Int) =
-      Error.Parsing(s"invalid string table index $id")
-
     internedList
       .map(idn =>
         decodeSegments(
           idn.getSegmentsInternedStrList.asScala
-            .map(id => internedStrings.lift(id).getOrElse(throw outOfRange(id)))
+            .map(id =>
+              internedStrings
+                .lift(id)
+                .getOrElse(throw Error.Parsing(s"invalid string table index $id"))
+            )
         )
       )
       .to(ImmArraySeq)

diff --git a/daml-lf/archive/src/main/scala/com/digitalasset/daml/lf/archive/Error.scala b/daml-lf/archive/src/main/scala/com/digitalasset/daml/lf/archive/Error.scala
@@ -1,15 +1,18 @@
 // Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-package com.daml.lf.archive
+package com.daml.lf
+package archive
 
 import java.io.File
 
 sealed abstract class Error(val msg: String) extends RuntimeException(msg)
 
 object Error {
 
-  final case class Internal(location: String, message: String) extends Error(s"IO error: $message")
+  final case class Internal(location: String, message: String)
+      extends Error(s"IO error: $message")
+      with InternalError
 
   final case class IO(location: String, cause: java.io.IOException)
       extends Error(s"IO error: $cause")

diff --git a/daml-lf/data/BUILD.bazel b/daml-lf/data/BUILD.bazel
@@ -35,6 +35,7 @@ da_scala_library(
         "//libs-scala/logging-entries",
         "@maven//:com_google_guava_guava",
         "@maven//:com_google_protobuf_protobuf_java",
+        "@maven//:org_slf4j_slf4j_api",
     ],
 )
 

diff --git a/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/InternalError.scala b/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/InternalError.scala
@@ -0,0 +1,32 @@
+// Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package com.daml.lf
+
+import org.slf4j.LoggerFactory
+
+private[lf] object InternalError {
+  private[this] val logger = LoggerFactory.getLogger("com.daml.lf")
+
+  def log(location: String, message: String): Unit =
+    logger.error(s"LF internal error in $location: $message")
+
+  @throws[IllegalArgumentException]
+  def illegalArgumentException(location: String, message: String): Nothing = {
+    log(location, message)
+    throw new IllegalArgumentException(message)
+  }
+
+  @throws[RuntimeException]
+  def runtimeException(location: String, message: String): Nothing = {
+    log(location, message)
+    throw new RuntimeException(message)
+  }
+}
+
+trait InternalError {
+  def location: String
+  def message: String
+
+  InternalError.log(location, message)
+}
diff --git a/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/Utf8.scala b/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/Utf8.scala
@@ -11,7 +11,6 @@ import com.google.protobuf.ByteString
 import scalaz.Order
 
 import scala.annotation.tailrec
-import scala.collection.compat._
 import scala.jdk.CollectionConverters._
 
 // The Daml-LF strings are supposed to be UTF-8 while standard java strings are UTF16
@@ -71,12 +70,16 @@ object Utf8 {
   }
 
   def unpack(s: String): ImmArray[Long] =
-    ImmArray(s.codePoints().iterator().asScala.map(_.toLong).iterator.to(Iterable))
+    s.codePoints().iterator().asScala.map(_.toLong).to(ImmArray)
 
-  @throws[IllegalArgumentException]
-  def pack(codePoints: ImmArray[Long]): String = {
+  // Converts the List of Unicode code points into a String if all code points are legal.
+  // Returns the first illegal code point as Left otherwise.
+  def pack(codePoints: ImmArray[Long]): Either[Long, String] = {
     val builder = new StringBuilder()
-    for (cp <- codePoints) {
+    var illegalCodePoint = Option.empty[Long]
+    val iterator = codePoints.iterator
+    while (iterator.nonEmpty && illegalCodePoint.isEmpty) {
+      val cp = iterator.next()
       if (
         Character.MIN_VALUE <= cp && cp < Character.MIN_SURROGATE ||
         Character.MAX_SURROGATE < cp && cp <= Character.MAX_VALUE
@@ -86,14 +89,15 @@ object Utf8 {
         builder += cp.toChar
       } else if (Character.MAX_VALUE < cp && cp <= Character.MAX_CODE_POINT) {
         // cp is from one of the Supplementary Plans,
-        // then it needs 2 UTF16 Char to be encoded.
+        // then it needs 2 UTF16 Chars to be encoded.
         builder += Character.highSurrogate(cp.toInt)
         builder += Character.lowSurrogate(cp.toInt)
       } else {
-        throw new IllegalArgumentException(s"invalid code point 0x${cp.toHexString}.")
+        // cp is not a legal Unicode code point
+        illegalCodePoint = Some(cp)
       }
     }
-    builder.result()
+    illegalCodePoint.toLeft(builder.result())
   }
 
 }
diff --git a/daml-lf/data/src/test/scala/com/digitalasset/daml/lf/data/Utf8Spec.scala b/daml-lf/data/src/test/scala/com/digitalasset/daml/lf/data/Utf8Spec.scala
@@ -139,12 +139,14 @@ class Utf8Spec extends AnyWordSpec with Matchers with ScalaCheckDrivenPropertyCh
         cp <- (Character.MIN_CODE_POINT until Character.MIN_SURROGATE) ++
           ((Character.MAX_SURROGATE + 1) to Character.MAX_CODE_POINT)
       )
-        Utf8.pack(makeImmArray(cp.toLong)) shouldBe "-" + new String(Character.toChars(cp)) + "-"
+        Utf8.pack(makeImmArray(cp.toLong)) shouldBe Right(
+          "-" + new String(Character.toChars(cp)) + "-"
+        )
     }
 
     "reject any surrogate code point" in {
       for (cp <- Character.MIN_SURROGATE to Character.MAX_SURROGATE)
-        an[IllegalArgumentException] should be thrownBy Utf8.pack(makeImmArray(cp.toLong))
+        Utf8.pack(makeImmArray(cp.toLong)) shouldBe a[Left[_, _]]
     }
 
     "reject too small or too big code points" in {
@@ -160,25 +162,25 @@ class Utf8Spec extends AnyWordSpec with Matchers with ScalaCheckDrivenPropertyCh
       )
 
       for (cp <- testCases)
-        an[IllegalArgumentException] should be thrownBy Utf8.pack(makeImmArray(cp))
+        Utf8.pack(makeImmArray(cp)) shouldBe a[Left[_, _]]
     }
 
     "packs properly" in {
-      Utf8.pack(ImmArray.empty) shouldBe ""
-      Utf8.pack(ImmArray(0x00061, 0x000b6, 0x02031, 0x1f602)) shouldBe "a¶‱😂"
+      Utf8.pack(ImmArray.empty) shouldBe Right("")
+      Utf8.pack(ImmArray(0x00061, 0x000b6, 0x02031, 0x1f602)) shouldBe Right("a¶‱😂")
     }
   }
 
   "unpack" should {
     "unpacks properly" in {
-      Utf8.pack(ImmArray.empty) shouldBe ""
+      Utf8.pack(ImmArray.empty) shouldBe Right("")
       Utf8.unpack("a¶‱😂") shouldBe ImmArray(0x00061, 0x000b6, 0x02031, 0x1f602)
     }
   }
 
   "pack and unpack" should {
     "form an isomorphism between strings and sequences of legal code points" in {
-      forAll(strings)(s => Utf8.pack(Utf8.unpack(s)) shouldBe s)
+      forAll(strings)(s => Utf8.pack(Utf8.unpack(s)) shouldBe Right(s))
     }
   }
 

diff --git a/daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/Error.scala b/daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/Error.scala
@@ -27,9 +27,10 @@ object Error {
     }
 
     final case class Internal(
-        nameOfFunc: String,
+        location: String,
         override val message: String,
     ) extends Error
+        with InternalError
 
     final case class Validation(validationError: validation.ValidationError) extends Error {
       def message: String = validationError.pretty
@@ -84,9 +85,10 @@ object Error {
     }
 
     final case class Internal(
-        nameOfFunc: String,
+        location: String,
         override val message: String,
     ) extends Error
+        with InternalError
 
     final case class Lookup(lookupError: language.LookupError) extends Error {
       override def message: String = lookupError.pretty
@@ -131,9 +133,10 @@ object Error {
     }
 
     final case class Internal(
-        nameOfFunc: String,
+        location: String,
         override val message: String,
     ) extends Error
+        with InternalError
 
     final case class DamlException(error: interpretation.Error) extends Error {
       // TODO https://github.com/digital-asset/daml/issues/9974

diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/ledger/BlindingTransaction.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/ledger/BlindingTransaction.scala
@@ -1,20 +1,19 @@
 // Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-package com.daml.lf.ledger
+package com.daml.lf
+package ledger
 
 import com.daml.lf.data.Ref.Party
 import com.daml.lf.data.Relation.Relation
 import com.daml.lf.transaction.BlindingInfo
 import com.daml.lf.transaction.Node
 import com.daml.lf.transaction.{NodeId, Transaction => Tx}
 import com.daml.lf.value.Value.ContractId
+import com.daml.nameof.NameOf
 
 object BlindingTransaction {
 
-  def crash(reason: String) =
-    throw new IllegalArgumentException(reason)
-
   private object BlindState {
     val Empty =
       BlindState(Map.empty, Map.empty)
@@ -31,7 +30,10 @@ object BlindingTransaction {
         nid: NodeId,
     ): BlindState = {
       if (disclosures.contains(nid))
-        crash(s"discloseNode: nodeId already processed '$nid'.")
+        InternalError.illegalArgumentException(
+          NameOf.qualifiedNameOfCurrentFunc,
+          s"discloseNode: nodeId already processed '$nid'.",
+        )
       // Each node should be visible to someone
       copy(
         disclosures = disclosures.updated(nid, witnesses)
@@ -107,8 +109,9 @@ object BlindingTransaction {
             )
           }
         case None =>
-          throw new IllegalArgumentException(
-            s"processNode - precondition violated: node $nodeId not present"
+          InternalError.illegalArgumentException(
+            NameOf.qualifiedNameOfCurrentFunc,
+            s"processNode - precondition violated: node $nodeId not present",
           )
       }
 

diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SBuiltin.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SBuiltin.scala
@@ -433,7 +433,12 @@ private[lf] object SBuiltin {
   final case object SBCodePointsToText extends SBuiltinPure(1) {
     override private[speedy] def executePure(args: util.ArrayList[SValue]): SText = {
       val codePoints = getSList(args, 0).map(_.asInstanceOf[SInt64].value)
-      SText(Utf8.pack(codePoints.toImmArray))
+      Utf8.pack(codePoints.toImmArray) match {
+        case Right(value) =>
+          SText(value)
+        case Left(cp) =>
+          crash(s"invalid code point 0x${cp.toHexString}.")
+      }
     }
   }
 

diff --git a/.../interpreter/src/main/scala/com/digitalasset/daml/lf/transaction/PartialTransaction.scala b/.../interpreter/src/main/scala/com/digitalasset/daml/lf/transaction/PartialTransaction.scala
@@ -18,6 +18,7 @@ import com.daml.lf.transaction.{
   TransactionVersion => TxVersion,
 }
 import com.daml.lf.value.Value
+import com.daml.nameof.NameOf
 
 import scala.collection.immutable.HashMap
 import scala.Ordering.Implicits.infixOrderingOps
@@ -67,14 +68,20 @@ private[lf] object PartialTransaction {
         case Some(Some(value)) =>
           value
         case _ =>
-          throw new RuntimeException(s"seed for ${idx}th root node not provided")
+          InternalError.runtimeException(
+            NameOf.qualifiedNameOfCurrentFunc,
+            s"seed for ${idx}th root node not provided",
+          )
       }
     }
   }
 
   private[PartialTransaction] object NoneSeededTransactionRootContext extends RootContextInfo {
     val actionChildSeed: Any => Nothing = { _ =>
-      throw new IllegalStateException(s"the machine is not configure to create transaction")
+      InternalError.runtimeException(
+        NameOf.qualifiedNameOfCurrentFunc,
+        s"the machine is not configure to create transaction",
+      )
     }
   }
 
@@ -605,7 +612,11 @@ private[lf] case class PartialTransaction(
             ec.parent.addActionChild(nodeId, exerciseNode.version min context.minChildVersion),
           nodes = nodes.updated(nodeId, exerciseNode),
         )
-      case _ => throw new RuntimeException("endExercises called in non-exercise context")
+      case _ =>
+        InternalError.runtimeException(
+          NameOf.qualifiedNameOfCurrentFunc,
+          "endExercises called in non-exercise context",
+        )
     }
 
   /** Close a abruptly an exercise context du to an uncaught exception.
@@ -624,7 +635,11 @@ private[lf] case class PartialTransaction(
           actionNodeSeeds =
             actionNodeSeeds :+ actionNodeSeed, //(NC) pushed by 'beginExercises'; why push again?
         )
-      case _ => throw new RuntimeException("abortExercises called in non-exercise context")
+      case _ =>
+        InternalError.runtimeException(
+          NameOf.qualifiedNameOfCurrentFunc,
+          "abortExercises called in non-exercise context",
+        )
     }
 
   private[this] def makeExNode(ec: ExercisesContextInfo): ExerciseNode = {
@@ -672,7 +687,10 @@ private[lf] case class PartialTransaction(
           )
         )
       case _ =>
-        throw new RuntimeException("endTry called in non-catch context")
+        InternalError.runtimeException(
+          NameOf.qualifiedNameOfCurrentFunc,
+          "endTry called in non-catch context",
+        )
     }
 
   /** Close abruptly a try context, due to an uncaught exception,
@@ -702,7 +720,11 @@ private[lf] case class PartialTransaction(
             .addRollbackChild(info.nodeId, context.minChildVersion, context.nextActionChildIdx),
           nodes = nodes.updated(info.nodeId, rollbackNode),
         ).resetActiveState(info.beginState)
-      case _ => throw new RuntimeException("rollbackTry called in non-catch context")
+      case _ =>
+        InternalError.runtimeException(
+          NameOf.qualifiedNameOfCurrentFunc,
+          "rollbackTry called in non-catch context",
+        )
     }
   }
 

diff --git a/daml-lf/language/BUILD.bazel b/daml-lf/language/BUILD.bazel
@@ -20,6 +20,7 @@ da_scala_library(
     visibility = ["//visibility:public"],
     deps = [
         "//daml-lf/data",
+        "//libs-scala/nameof",
     ],
 )
 

diff --git a/daml-lf/language/src/main/scala/com/digitalasset/daml/lf/language/TypeOrdering.scala b/daml-lf/language/src/main/scala/com/digitalasset/daml/lf/language/TypeOrdering.scala
@@ -5,6 +5,7 @@ package com.daml.lf
 package language
 
 import Ast._
+import com.daml.nameof.NameOf
 import data.Ref
 
 object TypeOrdering extends Ordering[Type] {
@@ -100,7 +101,10 @@ object TypeOrdering extends Ordering[Type] {
       case Ast.TStruct(_) => 3
       case Ast.TApp(_, _) => 4
       case Ast.TVar(_) | Ast.TForall(_, _) | Ast.TSynApp(_, _) =>
-        throw new IllegalArgumentException(s"cannot compare types $typ")
+        InternalError.illegalArgumentException(
+          NameOf.qualifiedNameOfCurrentFunc,
+          s"cannot compare types $typ",
+        )
     }
 
 }