Skip to content

Commit

Permalink
TM-23 Fail build on compiler warnings (corda#5453)
Browse files Browse the repository at this point in the history
* java compile respects compilation.allWarningsAsErrors

* suppress or cleanup warnings

* suppress warning

* use non-deprecated kotlin dependency

* rename property

* handle property existence check

* Deal with warnings
  • Loading branch information
zkiss authored and r3domfox committed Sep 11, 2019
1 parent 4e6edd0 commit f171de7
Show file tree
Hide file tree
Showing 75 changed files with 223 additions and 143 deletions.
9 changes: 8 additions & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ buildscript {
//
// TODO: Sort this alphabetically.
ext.kotlin_version = constants.getProperty("kotlinVersion")
ext.warnings_as_errors = project.hasProperty("compilation.warningsAsErrors") ? project.property("compilation.warningsAsErrors").toBoolean() : false

ext.quasar_group = 'co.paralleluniverse'
ext.quasar_version = constants.getProperty("quasarVersion")
Expand Down Expand Up @@ -214,6 +215,12 @@ allprojects {

tasks.withType(JavaCompile) {
options.compilerArgs << "-Xlint:unchecked" << "-Xlint:deprecation" << "-Xlint:-options" << "-parameters"
if (warnings_as_errors) {
// We cannot fail the build on compiler warnings because we have java warnings that you cannot disable:
// Signal is internal proprietary API and may be removed in a future release
// otherwise we should have the following line here:
// options.compilerArgs << "-Werror"
}
options.encoding = 'UTF-8'
}

Expand All @@ -224,7 +231,7 @@ allprojects {
jvmTarget = "1.8"
javaParameters = true // Useful for reflection.
freeCompilerArgs = ['-Xjvm-default=compatibility']
allWarningsAsErrors = project.hasProperty('compilation.allWarningsAsErrors') ? project.property('compilation.allWarningsAsErrors').toBoolean() : false
allWarningsAsErrors = warnings_as_errors
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ open class StringToMethodCallParser<in T : Any> @JvmOverloads constructor(
@Throws(UnparseableCallException::class)
fun validateIsMatchingCtor(methodNameHint: String, parameters: List<Pair<String, Type>>, args: String) {
val tree = createJsonTreeAndValidate(methodNameHint, parameters, args)
val inOrderParams: List<Any?> = parameters.mapIndexed { _, (argName, argType) ->
parameters.mapIndexed { _, (argName, _) ->
tree[argName] ?: throw UnparseableCallException.MissingParameter(methodNameHint, argName, args)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class NodeMonitorModel : AutoCloseable {
vaultUpdates.startWith(initialVaultUpdate).subscribe(vaultUpdatesSubject::onNext)

// Transactions
val (transactions, newTransactions) = rpc.internalVerifiedTransactionsFeed()
val (transactions, newTransactions) = @Suppress("DEPRECATION") rpc.internalVerifiedTransactionsFeed()
newTransactions.startWith(transactions).subscribe(transactionsSubject::onNext)

// SM -> TX mapping
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ open class CordaRPCClientConfiguration @JvmOverloads constructor(
if (trackRpcCallSites != other.trackRpcCallSites) return false
if (reapInterval != other.reapInterval) return false
if (observationExecutorPoolSize != other.observationExecutorPoolSize) return false
@Suppress("DEPRECATION")
if (cacheConcurrencyLevel != other.cacheConcurrencyLevel) return false
if (connectionRetryInterval != other.connectionRetryInterval) return false
if (connectionRetryIntervalMultiplier != other.connectionRetryIntervalMultiplier) return false
Expand All @@ -212,6 +213,7 @@ open class CordaRPCClientConfiguration @JvmOverloads constructor(
result = 31 * result + trackRpcCallSites.hashCode()
result = 31 * result + reapInterval.hashCode()
result = 31 * result + observationExecutorPoolSize
@Suppress("DEPRECATION")
result = 31 * result + cacheConcurrencyLevel
result = 31 * result + connectionRetryInterval.hashCode()
result = 31 * result + connectionRetryIntervalMultiplier.hashCode()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ object IdentitySyncFlow {
.filter { serviceHub.networkMapCache.getNodesByLegalIdentityKey(it.owningKey).isEmpty() }
.toList()
return confidentialIdentities
.map { Pair(it, serviceHub.identityService.certificateFromKey(it.owningKey)) }
.map {
@Suppress("DEPRECATION")
Pair(it, serviceHub.identityService.certificateFromKey(it.owningKey))
}
// Filter down to confidential identities of our well known identity
// TODO: Consider if this too restrictive - we perhaps should be checking the name on the signing certificate in the certificate path instead
.filter { it.second?.name == ourIdentity.name }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import co.paralleluniverse.fibers.Suspendable
import net.corda.core.CordaInternal
import net.corda.core.crypto.DigitalSignature
import net.corda.core.crypto.verify
import net.corda.core.flows.*
import net.corda.core.flows.FlowException
import net.corda.core.flows.FlowLogic
import net.corda.core.flows.FlowSession
import net.corda.core.flows.InitiatingFlow
import net.corda.core.identity.AnonymousParty
import net.corda.core.identity.CordaX500Name
import net.corda.core.identity.Party
Expand Down Expand Up @@ -41,7 +44,7 @@ private constructor(private val otherSideSession: FlowSession?,

@Deprecated("It is unsafe to use this constructor as it requires nodes to automatically vend anonymous identities without first " +
"checking if they should. Instead, use the constructor that takes in an existing FlowSession.")
constructor(otherParty: Party, revocationEnabled: Boolean, progressTracker: ProgressTracker) : this(null, otherParty, progressTracker)
constructor(otherParty: Party, @Suppress("UNUSED_PARAMETER") revocationEnabled: Boolean, progressTracker: ProgressTracker) : this(null, otherParty, progressTracker)

@Deprecated("It is unsafe to use this constructor as it requires nodes to automatically vend anonymous identities without first " +
"checking if they should. Instead, use the constructor that takes in an existing FlowSession.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class IdentitySyncFlowTests {
val issueFlow = charlieNode.services.startFlow(CashIssueAndPaymentFlow(1000.DOLLARS, ref, charlie, anonymous, notary))
val issueTx = issueFlow.resultFuture.getOrThrow().stx
val confidentialIdentity = issueTx.tx.outputs.map { it.data }.filterIsInstance<Cash.State>().single().owner
val confidentialIdentCert = charlieNode.services.identityService.certificateFromKey(confidentialIdentity.owningKey)!!
val confidentialIdentCert = @Suppress("DEPRECATION") charlieNode.services.identityService.certificateFromKey(confidentialIdentity.owningKey)!!

// Manually inject this identity into Alice's database so the node could leak it, but we prove won't
aliceNode.database.transaction { aliceNode.services.identityService.verifyAndRegisterIdentity(confidentialIdentCert) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ import net.corda.core.contracts.ContractState

@Suppress("unused")
object StateContractValidationEnforcementRule {
fun shouldEnforce(state: ContractState): Boolean = true
fun shouldEnforce(@Suppress("UNUSED_PARAMETER") state: ContractState): Boolean = true
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ package net.corda.core.internal
/**
* Stubbing out non-deterministic method.
*/
fun <T: Any> createInstancesOfClassesImplementing(classloader: ClassLoader, clazz: Class<T>): Set<T> {
fun <T: Any> createInstancesOfClassesImplementing(@Suppress("UNUSED_PARAMETER") classloader: ClassLoader, @Suppress("UNUSED_PARAMETER") clazz: Class<T>): Set<T> {
return emptySet()
}
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ class ConstraintsPropagationTests {
whenever(attachmentSigned.allContracts).thenReturn(setOf(propagatingContractClassName))

// network parameters
val netParams = testNetworkParameters(minimumPlatformVersion = 4,
testNetworkParameters(minimumPlatformVersion = 4,
packageOwnership = mapOf("net.corda.core.contracts" to ALICE_PARTY.owningKey))

ledgerServices.attachments.importContractAttachment(attachmentIdSigned, attachmentSigned)
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/kotlin/net/corda/core/Utils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import rx.Observer
// TODO Delete this file once the Future stuff is out of here

fun <A> CordaFuture<out A>.toObservable(): Observable<A> {
return Observable.create { subscriber ->
return Observable.unsafeCreate { subscriber ->
thenMatch({
subscriber.onNext(it)
subscriber.onCompleted()
Expand Down
20 changes: 16 additions & 4 deletions core/src/main/kotlin/net/corda/core/contracts/ContractsDSL.kt
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,14 @@ fun <C : CommandData> Collection<CommandWithParties<CommandData>>.select(klass:
party: AbstractParty? = null) =
mapNotNull { if (klass.isInstance(it.value)) uncheckedCast<CommandWithParties<CommandData>, CommandWithParties<C>>(it) else null }.
filter { if (signer == null) true else signer in it.signers }.
filter { if (party == null) true else party in it.signingParties }.
map { CommandWithParties(it.signers, it.signingParties, it.value) }
filter {
@Suppress("DEPRECATION")
if (party == null) true else party in it.signingParties
}.
map {
@Suppress("DEPRECATION")
CommandWithParties(it.signers, it.signingParties, it.value)
}

/** Filters the command list by type, parties and public keys all at once. */
inline fun <reified T : CommandData> Collection<CommandWithParties<CommandData>>.select(signers: Collection<PublicKey>?,
Expand All @@ -56,8 +62,14 @@ fun <C : CommandData> Collection<CommandWithParties<CommandData>>.select(klass:
parties: Collection<Party>?) =
mapNotNull { if (klass.isInstance(it.value)) uncheckedCast<CommandWithParties<CommandData>, CommandWithParties<C>>(it) else null }.
filter { if (signers == null) true else it.signers.containsAll(signers) }.
filter { if (parties == null) true else it.signingParties.containsAll(parties) }.
map { CommandWithParties(it.signers, it.signingParties, it.value) }
filter {
@Suppress("DEPRECATION")
if (parties == null) true else it.signingParties.containsAll(parties)
}.
map {
@Suppress("DEPRECATION")
CommandWithParties(it.signers, it.signingParties, it.value)
}

/** Ensures that a transaction has only one command that is of the given type, otherwise throws an exception. */
inline fun <reified T : CommandData> Collection<CommandWithParties<CommandData>>.requireSingleCommand() = requireSingleCommand(T::class.java)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,15 @@ val ContractState.requiredContractClassName: String? get() {
*/
fun AttachmentConstraint.canBeTransitionedFrom(input: AttachmentConstraint, attachment: ContractAttachment): Boolean {
val output = this

@Suppress("DEPRECATION")
fun AttachmentConstraint.isAutomaticHashConstraint() =
this is AutomaticHashConstraint

return when {
// These branches should not happen, as this has been already checked.
input is AutomaticPlaceholderConstraint || output is AutomaticPlaceholderConstraint -> throw IllegalArgumentException("Illegal constraint: AutomaticPlaceholderConstraint.")
input is AutomaticHashConstraint || output is AutomaticHashConstraint -> throw IllegalArgumentException("Illegal constraint: AutomaticHashConstraint.")
input.isAutomaticHashConstraint() || output.isAutomaticHashConstraint() -> throw IllegalArgumentException("Illegal constraint: AutomaticHashConstraint.")

// Transition to the same constraint.
input == output -> true
Expand Down
10 changes: 6 additions & 4 deletions core/src/main/kotlin/net/corda/core/internal/TransactionUtils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,12 @@ class TransactionDeserialisationException(groupEnum: ComponentGroupEnum, index:
*
* This method used the [deserialiseComponentGroup] method.
*/
fun deserialiseCommands(componentGroups: List<ComponentGroup>,
forceDeserialize: Boolean = false,
factory: SerializationFactory = SerializationFactory.defaultFactory,
context: SerializationContext = factory.defaultContext): List<Command<*>> {
fun deserialiseCommands(
componentGroups: List<ComponentGroup>,
forceDeserialize: Boolean = false,
factory: SerializationFactory = SerializationFactory.defaultFactory,
@Suppress("UNUSED_PARAMETER") context: SerializationContext = factory.defaultContext
): List<Command<*>> {
// TODO: we could avoid deserialising unrelated signers.
// However, current approach ensures the transaction is not malformed
// and it will throw if any of the signers objects is not List of public keys).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ interface IdentityService {
*/
@Throws(UnknownAnonymousPartyException::class)
fun assertOwnership(party: Party, anonymousParty: AnonymousParty) {
@Suppress("DEPRECATION")
val anonymousIdentity = certificateFromKey(anonymousParty.owningKey)
?: throw UnknownAnonymousPartyException("Unknown $anonymousParty")
val issuingCert = anonymousIdentity.certPath.certificates[1]
Expand Down Expand Up @@ -79,7 +80,9 @@ interface IdentityService {
* @param key The owning [PublicKey] of the [Party].
* @return Returns a [Party] with a matching owningKey if known, else returns null.
*/
fun partyFromKey(key: PublicKey): Party? = certificateFromKey(key)?.party
fun partyFromKey(key: PublicKey): Party? =
@Suppress("DEPRECATION")
certificateFromKey(key)?.party

/**
* Resolves a party name to the well known identity [Party] instance for this name. Where possible well known identity
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -390,5 +390,7 @@ private constructor(
privacySalt: PrivacySalt = this.privacySalt,
sigs: List<TransactionSignature> = this.sigs,
networkParameters: NetworkParameters = this.networkParameters
) = ContractUpgradeLedgerTransaction(inputs, notary, legacyContractAttachment, upgradedContractClassName, upgradedContractAttachment, id, privacySalt, sigs, networkParameters)
) =
@Suppress("DEPRECATION")
ContractUpgradeLedgerTransaction(inputs, notary, legacyContractAttachment, upgradedContractClassName, upgradedContractAttachment, id, privacySalt, sigs, networkParameters)
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,11 @@ data class NotaryChangeWireTransaction(
* TODO - currently this uses the main classloader.
*/
@CordaInternal
internal fun resolveOutputComponent(services: ServicesForResolution, stateRef: StateRef, params: NetworkParameters): SerializedBytes<TransactionState<ContractState>> {
internal fun resolveOutputComponent(
services: ServicesForResolution,
stateRef: StateRef,
@Suppress("UNUSED_PARAMETER") params: NetworkParameters
): SerializedBytes<TransactionState<ContractState>> {
return services.loadState(stateRef).serialize()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,9 @@ open class TransactionBuilder(
* TODO also on the versions of the attachments of the transactions generating the input states. ( after we add versioning)
*/
private fun selectContractAttachmentsAndOutputStateConstraints(
services: ServicesForResolution, serializationContext: SerializationContext?): Pair<Collection<SecureHash>, List<TransactionState<ContractState>>> {
services: ServicesForResolution,
@Suppress("UNUSED_PARAMETER") serializationContext: SerializationContext?
): Pair<Collection<SecureHash>, List<TransactionState<ContractState>>> {

// Determine the explicitly set contract attachments.
val explicitAttachmentContracts: List<Pair<ContractClassName, SecureHash>> = this.attachments
Expand Down Expand Up @@ -297,7 +299,10 @@ open class TransactionBuilder(
return Pair(attachments, resolvedOutputStatesInTheOriginalOrder)
}

private val automaticConstraints = setOf(AutomaticPlaceholderConstraint, AutomaticHashConstraint)
private val automaticConstraints = setOf(
AutomaticPlaceholderConstraint,
@Suppress("DEPRECATION") AutomaticHashConstraint
)

/**
* Selects an attachment and resolves the constraints for the output states with [AutomaticPlaceholderConstraint].
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,14 @@ class WireTransaction(componentGroups: List<ComponentGroup>, val privacySalt: Pr
val hashToResolve = it ?: services.networkParametersService.defaultHash
services.networkParametersService.lookup(hashToResolve)
},
resolveContractAttachment = { services.loadContractAttachment(it) },
// `as?` is used due to [MockServices] not implementing [ServiceHubCoreInternal]
isAttachmentTrusted = { (services as? ServiceHubCoreInternal)?.attachmentTrustCalculator?.calculate(it) ?: true }
)
}

// Helper for deprecated toLedgerTransaction
// TODO: revisit once Deterministic JVM code updated
@Suppress("UNUSED") // not sure if this field can be removed safely??
private val missingAttachment: Attachment by lazy {
object : AbstractAttachment({ byteArrayOf() }, DEPLOYED_CORDAPP_UPLOADER ) {
override val id: SecureHash get() = throw UnsupportedOperationException()
Expand Down Expand Up @@ -143,8 +143,6 @@ class WireTransaction(componentGroups: List<ComponentGroup>, val privacySalt: Pr
resolveAttachment,
{ stateRef -> resolveStateRef(stateRef)?.serialize() },
{ null },
// Returning a dummy `missingAttachment` Attachment allows this deprecated method to work and it disables "contract version no downgrade rule" as a dummy Attachment returns version 1
{ resolveAttachment(it.txhash) ?: missingAttachment },
{ it.isUploaderTrusted() }
)
}
Expand All @@ -161,7 +159,6 @@ class WireTransaction(componentGroups: List<ComponentGroup>, val privacySalt: Pr
resolveAttachment,
{ stateRef -> resolveStateRef(stateRef)?.serialize() },
resolveParameters,
{ resolveAttachment(it.txhash) ?: missingAttachment },
{ true } // Any attachment loaded through the DJVM should be trusted
)
}
Expand All @@ -171,7 +168,6 @@ class WireTransaction(componentGroups: List<ComponentGroup>, val privacySalt: Pr
resolveAttachment: (SecureHash) -> Attachment?,
resolveStateRefAsSerialized: (StateRef) -> SerializedBytes<TransactionState<ContractState>>?,
resolveParameters: (SecureHash?) -> NetworkParameters?,
resolveContractAttachment: (StateRef) -> Attachment,
isAttachmentTrusted: (Attachment) -> Boolean
): LedgerTransaction {
// Look up public keys to authenticated identities.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import com.nhaarman.mockito_kotlin.mock
import org.assertj.core.api.Assertions.assertThat
import org.assertj.core.api.Assertions.assertThatThrownBy
import org.junit.Test
import java.lang.IllegalArgumentException
import java.lang.RuntimeException

class ClassLoadingUtilsTest {

Expand Down Expand Up @@ -33,7 +31,7 @@ class ClassLoadingUtilsTest {

@Test(expected = IllegalArgumentException::class)
fun throwsExceptionWhenClassDoesNotContainProperConstructors() {
val classes = createInstancesOfClassesImplementing(BaseInterface::class.java.classLoader, BaseInterface2::class.java)
createInstancesOfClassesImplementing(BaseInterface::class.java.classLoader, BaseInterface2::class.java)
}

@Test
Expand Down
Loading

0 comments on commit f171de7

Please sign in to comment.