From 5458aa890ccb1da23b5e44f09d512d338074d5a6 Mon Sep 17 00:00:00 2001 From: nickchapman-da <49153372+nickchapman-da@users.noreply.github.com> Date: Tue, 28 Sep 2021 16:34:31 +0100 Subject: [PATCH] Switch on NonUnitStatements warning in daml-lf/transaction (#11048) * Add NonUnitStatements warning in daml-lf/transaction & fix using discard or similar. CHANGELOG_BEGIN CHANGELOG_END * address comment * prefer parens to braces for single statement map/foreach bodies --- daml-lf/transaction/BUILD.bazel | 7 +- .../digitalasset/daml/lf/crypto/Hash.scala | 12 ++-- .../digitalasset/daml/lf/ledger/EventId.scala | 10 +-- .../lf/transaction/TransactionCoder.scala | 66 +++++++++---------- .../daml/lf/value/CidContainer.scala | 6 +- .../digitalasset/daml/lf/value/Value.scala | 3 +- .../daml/lf/value/ValueCoder.scala | 57 ++++++++-------- 7 files changed, 85 insertions(+), 76 deletions(-) diff --git a/daml-lf/transaction/BUILD.bazel b/daml-lf/transaction/BUILD.bazel index 086d92313c55..88122b7b9f35 100644 --- a/daml-lf/transaction/BUILD.bazel +++ b/daml-lf/transaction/BUILD.bazel @@ -10,6 +10,10 @@ load( "silencer_plugin", ) +lf_scalacopts_stricter = lf_scalacopts + [ + "-P:wartremover:traverser:org.wartremover.warts.NonUnitStatements", +] + # # Transaction and value protocol buffers # @@ -53,7 +57,7 @@ da_scala_library( "@maven//:org_scalaz_scalaz_core", "@maven//:org_scala_lang_modules_scala_collection_compat", ], - scalacopts = lf_scalacopts, + scalacopts = lf_scalacopts_stricter, tags = ["maven_coordinates=com.daml:daml-lf-transaction:__VERSION__"], visibility = ["//visibility:public"], deps = [ @@ -62,6 +66,7 @@ da_scala_library( "//daml-lf/data", "//daml-lf/language", "//libs-scala/nameof", + "//libs-scala/scala-utils", "@maven//:com_google_protobuf_protobuf_java", ], ) diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/crypto/Hash.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/crypto/Hash.scala index 4e8b9bfd38ca..3d603687109e 100644 --- a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/crypto/Hash.scala +++ b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/crypto/Hash.scala @@ -10,6 +10,7 @@ import java.util.concurrent.atomic.AtomicLong import com.daml.lf.data.{Bytes, ImmArray, Ref, Time, Utf8} import com.daml.lf.value.Value +import com.daml.scalautil.Statement.discard import scalaz.Order import javax.crypto.Mac import javax.crypto.spec.SecretKeySpec @@ -133,16 +134,16 @@ object Hash { private val intBuffer = ByteBuffer.allocate(java.lang.Integer.BYTES) final def add(a: Int): this.type = { - intBuffer.rewind() - intBuffer.putInt(a).position(0) + discard(intBuffer.rewind()) + discard(intBuffer.putInt(a).position(0)) add(intBuffer) } private val longBuffer = ByteBuffer.allocate(java.lang.Long.BYTES) final def add(a: Long): this.type = { - longBuffer.rewind() - longBuffer.putLong(a).position(0) + discard(longBuffer.rewind()) + discard(longBuffer.putLong(a).position(0)) add(longBuffer) } @@ -172,8 +173,7 @@ object Hash { @throws[HashingError] final def addCid(cid: Value.ContractId): this.type = { val bytes = cid2Bytes(cid) - add(bytes.length) - add(bytes) + add(bytes.length).add(bytes) } // In order to avoid hash collision, this should be used together diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/ledger/EventId.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/ledger/EventId.scala index e7956f25e285..efc2e3ba42da 100644 --- a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/ledger/EventId.scala +++ b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/ledger/EventId.scala @@ -15,11 +15,11 @@ case class EventId( nodeId: NodeId, ) { lazy val toLedgerString: LedgerString = { - val builder = new StringBuilder() - builder += '#' - builder ++= transactionId - builder += ':' - builder ++= nodeId.index.toString + val builder = (new StringBuilder() + += '#' + ++= transactionId + += ':' + ++= nodeId.index.toString) LedgerString.assertFromString(builder.result()) } } diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/TransactionCoder.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/TransactionCoder.scala index df83e07a3d50..6274f339909f 100644 --- a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/TransactionCoder.scala +++ b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/TransactionCoder.scala @@ -11,6 +11,7 @@ import com.daml.lf.data.Ref.{Name, Party} import com.daml.lf.transaction.Node._ import com.daml.lf.value.{Value, ValueCoder, ValueOuterClass} import com.daml.lf.value.ValueCoder.{DecodeError, EncodeError} +import com.daml.scalautil.Statement.discard import com.google.protobuf.{ByteString, GeneratedMessageV3, ProtocolStringList} import scala.Ordering.Implicits.infixOrderingOps @@ -203,7 +204,7 @@ object TransactionCoder { ) = { key match { case Some(key) => - encodeKeyWithMaintainers(encodeCid, version, key).map { k => setKey(k); () } + encodeKeyWithMaintainers(encodeCid, version, key).map(k => discard(setKey(k))) case None => Right(()) } @@ -217,15 +218,9 @@ object TransactionCoder { setUnversioned: ByteString => GeneratedMessageV3.Builder[_], ): Either[EncodeError, Unit] = { if (version < TransactionVersion.minNoVersionValue) { - encodeVersionedValue(encodeCid, version, value).map { v => - setVersioned(v); - {} - } + encodeVersionedValue(encodeCid, version, value).map(v => discard(setVersioned(v))) } else { - encodeValue(encodeCid, version, value).map { v => - setUnversioned(v) - () - } + encodeValue(encodeCid, version, value).map(v => discard(setUnversioned(v))) } } @@ -255,7 +250,7 @@ object TransactionCoder { node match { case NodeRollback(children) => val builder = TransactionOuterClass.NodeRollback.newBuilder() - children.foreach { id => builder.addChildren(encodeNid.asString(id)); () } + children.foreach(id => discard(builder.addChildren(encodeNid.asString(id)))) for { _ <- Either.cond( test = enclosingVersion >= TransactionVersion.minExceptions || disableVersionCheck, @@ -274,7 +269,7 @@ object TransactionCoder { ) else { if (enclosingVersion >= TransactionVersion.minNodeVersion) - nodeBuilder.setVersion(nodeVersion.protoValue) + discard(nodeBuilder.setVersion(nodeVersion.protoValue)) node match { @@ -282,7 +277,7 @@ object TransactionCoder { val builder = TransactionOuterClass.NodeCreate.newBuilder() nc.stakeholders.foreach(builder.addStakeholders) nc.signatories.foreach(builder.addSignatories) - builder.setContractIdStruct(encodeCid.encode(nc.coid)) + discard(builder.setContractIdStruct(encodeCid.encode(nc.coid))) for { _ <- if (nodeVersion < TransactionVersion.minNoVersionValue) { @@ -295,11 +290,12 @@ object TransactionCoder { ) .map(builder.setContractInstance) } else { - encodeValue(encodeCid, nodeVersion, nc.arg).map { arg => - builder.setTemplateId(ValueCoder.encodeIdentifier(nc.templateId)) - builder.setArgUnversioned(arg) - builder.setAgreement(nc.agreementText) - } + encodeValue(encodeCid, nodeVersion, nc.arg).map(arg => + builder + .setTemplateId(ValueCoder.encodeIdentifier(nc.templateId)) + .setArgUnversioned(arg) + .setAgreement(nc.agreementText) + ) } _ <- encodeAndSetContractKey( encodeCid, @@ -311,12 +307,12 @@ object TransactionCoder { case nf @ NodeFetch(_, _, _, _, _, _, _, _) => val builder = TransactionOuterClass.NodeFetch.newBuilder() - builder.setTemplateId(ValueCoder.encodeIdentifier(nf.templateId)) + discard(builder.setTemplateId(ValueCoder.encodeIdentifier(nf.templateId))) nf.stakeholders.foreach(builder.addStakeholders) nf.signatories.foreach(builder.addSignatories) - builder.setContractIdStruct(encodeCid.encode(nf.coid)) + discard(builder.setContractIdStruct(encodeCid.encode(nf.coid))) if (nodeVersion >= TransactionVersion.minByKey) { - builder.setByKey(nf.byKey) + discard(builder.setByKey(nf.byKey)) } nf.actingParties.foreach(builder.addActors) for { @@ -330,17 +326,20 @@ object TransactionCoder { case ne @ NodeExercises(_, _, _, _, _, _, _, _, _, _, _, _, _, _) => val builder = TransactionOuterClass.NodeExercise.newBuilder() - builder.setContractIdStruct(encodeCid.encode(ne.targetCoid)) - builder.setChoice(ne.choiceId) - builder.setTemplateId(ValueCoder.encodeIdentifier(ne.templateId)) - builder.setConsuming(ne.consuming) + discard( + builder + .setContractIdStruct(encodeCid.encode(ne.targetCoid)) + .setChoice(ne.choiceId) + .setTemplateId(ValueCoder.encodeIdentifier(ne.templateId)) + .setConsuming(ne.consuming) + ) ne.actingParties.foreach(builder.addActors) - ne.children.foreach { id => builder.addChildren(encodeNid.asString(id)); () } + ne.children.foreach(id => discard(builder.addChildren(encodeNid.asString(id)))) ne.signatories.foreach(builder.addSignatories) ne.stakeholders.foreach(builder.addStakeholders) ne.choiceObservers.foreach(builder.addObservers) if (nodeVersion >= TransactionVersion.minByKey) { - builder.setByKey(ne.byKey) + discard(builder.setByKey(ne.byKey)) } for { _ <- Either.cond( @@ -382,12 +381,14 @@ object TransactionCoder { case nlbk @ NodeLookupByKey(_, _, _, _) => val builder = TransactionOuterClass.NodeLookupByKey.newBuilder() - builder.setTemplateId(ValueCoder.encodeIdentifier(nlbk.templateId)) - nlbk.result.foreach(cid => builder.setContractIdStruct(encodeCid.encode(cid))) + discard(builder.setTemplateId(ValueCoder.encodeIdentifier(nlbk.templateId))) + nlbk.result.foreach(cid => + discard(builder.setContractIdStruct(encodeCid.encode(cid))) + ) for { encodedKey <- encodeKeyWithMaintainers(encodeCid, nlbk.version, nlbk.key) } yield { - builder.setKeyWithMaintainers(encodedKey) + discard(builder.setKeyWithMaintainers(encodedKey)) nodeBuilder.setLookupByKey(builder).build() } } @@ -567,7 +568,7 @@ object TransactionCoder { nodeVersion, protoExe.getResultVersioned, protoExe.getResultUnversioned, - ).map { v => Some(v) } + ).map(v => Some(v)) } keyWithMaintainers <- decodeOptionalKeyWithMaintainers(decodeCid, nodeVersion, protoExe.getKeyWithMaintainers) @@ -673,10 +674,7 @@ object TransactionCoder { val builder = TransactionOuterClass.Transaction .newBuilder() .setVersion(transaction.version.protoValue) - transaction.roots.foreach { nid => - builder.addRoots(encodeNid.asString(nid)) - () - } + transaction.roots.foreach(nid => discard(builder.addRoots(encodeNid.asString(nid)))) transaction .fold[Either[EncodeError, TransactionOuterClass.Transaction.Builder]]( diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/CidContainer.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/CidContainer.scala index b8b46406040d..03933bb3e352 100644 --- a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/CidContainer.scala +++ b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/CidContainer.scala @@ -5,6 +5,7 @@ package com.daml.lf package value import com.daml.lf.data.Bytes +import com.daml.scalautil.Statement.discard import Value.ContractId import scala.util.control.NoStackTrace @@ -16,11 +17,10 @@ trait CidContainer[+A] { def mapCid(f: ContractId => ContractId): A def foreachCid(f: ContractId => Unit) = { - mapCid(cid => { + discard(mapCid(cid => { f(cid) cid - }) - () + })) } // We cheat using exceptions, to get a cheap implementation of traverse using the `map` function above. diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/Value.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/Value.scala index 3ada4c353ee9..770913fcceb1 100644 --- a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/Value.scala +++ b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/Value.scala @@ -9,6 +9,7 @@ import com.daml.lf.data.Ref.{Identifier, Name} import com.daml.lf.data._ import com.daml.lf.language.Ast import com.daml.lf.transaction.TransactionVersion +import com.daml.scalautil.Statement.discard import data.ScalazEqual._ import scalaz.{@@, Equal, Order, Tag} @@ -76,7 +77,7 @@ sealed abstract class Value extends CidContainer[Value] with Product with Serial def cids[Cid2 >: ContractId] = { val cids = Set.newBuilder[Cid2] - foreach1(cids += _)(this) + foreach1(x => discard(cids += x))(this) cids.result() } diff --git a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/ValueCoder.scala b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/ValueCoder.scala index 9104e990f7ee..d61032464778 100644 --- a/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/ValueCoder.scala +++ b/daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/value/ValueCoder.scala @@ -9,6 +9,7 @@ import com.daml.lf.data._ import com.daml.lf.transaction.TransactionVersion import com.daml.lf.value.Value._ import com.daml.lf.value.{ValueOuterClass => proto} +import com.daml.scalautil.Statement.discard import com.google.protobuf import com.google.protobuf.{ByteString, CodedInputStream} @@ -93,10 +94,12 @@ object ValueCoder { * @return wire format identifier */ def encodeIdentifier(id: Identifier): proto.Identifier = { - val builder = proto.Identifier.newBuilder().setPackageId(id.packageId) - builder.addAllModuleName((id.qualifiedName.module.segments.toSeq: Seq[String]).asJava) - builder.addAllName((id.qualifiedName.name.segments.toSeq: Seq[String]).asJava) - builder.build() + proto.Identifier + .newBuilder() + .setPackageId(id.packageId) + .addAllModuleName((id.qualifiedName.module.segments.toSeq: Seq[String]).asJava) + .addAllName((id.qualifiedName.name.segments.toSeq: Seq[String]).asJava) + .build() } /** Decode identifier from wire format @@ -173,7 +176,7 @@ object ValueCoder { private[this] def parseValue(bytes: ByteString): Either[DecodeError, proto.Value] = Try { val cis = CodedInputStream.newInstance(bytes.asReadOnlyByteBuffer()) - cis.setRecursionLimit(MAXIMUM_PROTO_RECURSION_LIMIT) + discard(cis.setRecursionLimit(MAXIMUM_PROTO_RECURSION_LIMIT)) proto.Value.parseFrom(cis) } match { case Failure(exception: Error) => @@ -438,8 +441,7 @@ object ValueCoder { case ValueList(elems) => val listBuilder = proto.List.newBuilder() elems.foreach(elem => { - listBuilder.addElements(go(newNesting, elem)) - () + discard(listBuilder.addElements(go(newNesting, elem))) }) builder.setList(listBuilder).build() @@ -447,19 +449,20 @@ object ValueCoder { val recordBuilder = proto.Record.newBuilder() fields.foreach { case (fieldName, field) => val b = proto.RecordField.newBuilder() - if (valueVersion < TransactionVersion.minTypeErasure) fieldName.map(b.setLabel) - b.setValue(go(newNesting, field)) - recordBuilder.addFields(b) - () + if (valueVersion < TransactionVersion.minTypeErasure) + discard(fieldName.map(b.setLabel)) + discard(b.setValue(go(newNesting, field))) + discard(recordBuilder.addFields(b)) } if (valueVersion < TransactionVersion.minTypeErasure) id.foreach(i => recordBuilder.setRecordId(encodeIdentifier(i))) builder.setRecord(recordBuilder).build() case ValueVariant(id, con, arg) => - val protoVar = proto.Variant.newBuilder() - protoVar.setConstructor(con) - protoVar.setValue(go(newNesting, arg)) + val protoVar = proto.Variant + .newBuilder() + .setConstructor(con) + .setValue(go(newNesting, arg)) if (valueVersion < TransactionVersion.minTypeErasure) id.foreach(i => protoVar.setVariantId(encodeIdentifier(i))) builder.setVariant(protoVar).build() @@ -480,13 +483,14 @@ object ValueCoder { case ValueTextMap(map) => val protoMap = proto.Map.newBuilder() map.toImmArray.foreach { case (key, value) => - protoMap.addEntries( - proto.Map.Entry - .newBuilder() - .setKey(key) - .setValue(go(newNesting, value)) + discard( + protoMap.addEntries( + proto.Map.Entry + .newBuilder() + .setKey(key) + .setValue(go(newNesting, value)) + ) ) - () } builder.setMap(protoMap).build() @@ -494,13 +498,14 @@ object ValueCoder { assertSince(TransactionVersion.minGenMap, "Value.SumCase.MAP") val protoMap = proto.GenMap.newBuilder() entries.foreach { case (key, value) => - protoMap.addEntries( - proto.GenMap.Entry - .newBuilder() - .setKey(go(newNesting, key)) - .setValue(go(newNesting, value)) + discard( + protoMap.addEntries( + proto.GenMap.Entry + .newBuilder() + .setKey(go(newNesting, key)) + .setValue(go(newNesting, value)) + ) ) - () } builder.setGenMap(protoMap).build()