Skip to content

Commit

Permalink
Added more useful error when multiple mappers are resolved
Browse files Browse the repository at this point in the history
  • Loading branch information
stefankoppier committed Jul 28, 2024
1 parent 3052199 commit bdb58e6
Show file tree
Hide file tree
Showing 11 changed files with 151 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,10 @@ class ConstructorCallMappingConstructor(
val getter = irCall(source.property.function).apply {
dispatchReceiver = irGet(source.property.holder)
}
return source.via?.let { generateTransformation(generated, it, getter, source, target, file) } ?: getter
return source.transformation
.singleOrNull()
?.let { generateTransformation(generated, it, getter, source, target, file) }
?: getter
}

private fun IrBuilderWithScope.generatePropertyValueArgument(
Expand All @@ -91,7 +94,8 @@ class ConstructorCallMappingConstructor(
?: irGet(parameters.singleOrNull { it.type == source.property.targetType(file) }
?: mappieTerminate("Could not determine value parameters for property reference. Please use a property reference of an object instead of the class", location(file, source.property)))
}
return source.transformation?.let { generateTransformation(generated, it, getter, source, target, file) }
return source.transformation.singleOrNull()
?.let { generateTransformation(generated, it, getter, source, target, file) }
?: getter
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import tech.mappie.util.*

class MappieDefinitions(definition: List<MappieDefinition>) : List<MappieDefinition> by definition {

fun select(source: IrType, target: IrType): MappieDefinition? =
singleOrNull { it.fromType == source && it.toType == target } ?: singleOrNull { it.fits(source, target) }
fun select(source: IrType, target: IrType): List<MappieDefinition> =
singleOrNull { it.fromType == source && it.toType == target }?.let { listOf(it) }
?: filter { it.fits(source, target) }
}

data class MappieDefinition(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,13 @@ private class ObjectBodyStatementCollector(file: IrFileEntry)
IDENTIFIER_TRANSFORM -> {
val mapping = expression.dispatchReceiver!!.accept(data)!!
val transformation = MappieTransformOperator(expression.valueArguments.first()!! as IrFunctionExpression)
mapping.first to (mapping.second as PropertySource).copy(transformation = transformation)
mapping.first to (mapping.second as PropertySource).copy(transformation = listOf(transformation))
}
IDENTIFIER_VIA -> {
val mapping = expression.dispatchReceiver!!.accept(data)!!
val transformation = expression.valueArguments.first()!!.accept(MapperReferenceCollector(file!!), Unit)
mapping.first to (mapping.second as PropertySource).copy(
transformation = transformation
transformation = listOf(transformation)
)
}
else -> {
Expand Down Expand Up @@ -142,7 +142,7 @@ private class SourceValueCollector(file: IrFileEntry) : BaseVisitor<ObjectMappin
override fun visitPropertyReference(expression: IrPropertyReference, data: Unit): ObjectMappingSource {
return PropertySource(
property = expression,
transformation = null,
transformation = emptyList(),
origin = expression,
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,28 +14,33 @@ sealed interface ObjectMappingSource {

data class ResolvedSource(
val property: MappieSource,
val via: MappieTransformation? = null,
val viaType: IrType? = null,
val transformation: List<MappieTransformation>,
val transformationType: IrType? = null,
override val origin: IrElement,
) : ObjectMappingSource {
override val type: IrType get() = viaType ?: property.type
override val type: IrType get() = transformationType ?: property.type
}

data class PropertySource(
val property: IrPropertyReference,
val transformation: MappieTransformation? = null,
val transformation: List<MappieTransformation> = emptyList(),
override val origin: IrElement,
) : ObjectMappingSource {

val getter = property.getter!!

override val type: IrType get() = when (transformation) {
is MappieTransformOperator -> (transformation.type as IrSimpleType).arguments[1].typeOrFail
is MappieViaOperator -> if (getter.owner.returnType.isNullable()) transformation.type.makeNullable() else transformation.type
is MappieViaResolved -> if (getter.owner.returnType.isNullable()) transformation.type.makeNullable() else transformation.type
is MappieViaGeneratedClass -> if (getter.owner.returnType.isNullable()) transformation.type.makeNullable() else transformation.type
null -> getter.owner.returnType
}
override val type: IrType get() =
if (transformation.isEmpty()) {
getter.owner.returnType
} else {
val transformation = transformation.first()
when (transformation) {
is MappieTransformOperator -> (transformation.type as IrSimpleType).arguments[1].typeOrFail
is MappieViaOperator -> if (getter.owner.returnType.isNullable()) transformation.type.makeNullable() else transformation.type
is MappieViaResolved -> if (getter.owner.returnType.isNullable()) transformation.type.makeNullable() else transformation.type
is MappieViaGeneratedClass -> if (getter.owner.returnType.isNullable()) transformation.type.makeNullable() else transformation.type
}
}
}

data class ExpressionSource(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,20 @@ class ObjectMappingsConstructor(
val concreteSource = explicit[target.name]
if (concreteSource != null) {
val source = concreteSource.singleOrNull()
if (source != null && source is PropertySource && source.transformation == null) {
if (source != null && source is PropertySource && source.transformation.isEmpty()) {
if (!target.type.isAssignableFrom(source.type)) {
val via = symbols
val vias = symbols
.select(source.type, target.type)
?.let { via -> when {
target.type.isList() -> via.copy(toType = context.irBuiltIns.listClass.typeWith(listOf(via.toType)))
target.type.isSet() -> via.copy(toType = context.irBuiltIns.setClass.typeWith(listOf(via.toType)))
else -> via
} }
listOf(source.copy(transformation = via?.let{ MappieViaResolved(it) }))
.let { vias ->
vias.map { via ->
when {
target.type.isList() -> via.copy(toType = context.irBuiltIns.listClass.typeWith(listOf(via.toType)))
target.type.isSet() -> via.copy(toType = context.irBuiltIns.setClass.typeWith(listOf(via.toType)))
else -> via
}
}
}
listOf(source.copy(transformation = vias.map { MappieViaResolved(it) }))
} else {
concreteSource
}
Expand All @@ -49,19 +53,21 @@ class ObjectMappingsConstructor(
val mappings = sources.filter { source -> source.name == getterName(target.name) }
.map { getter ->
if (target.type.isAssignableFrom(getter.type)) {
ResolvedSource(getter, null, null, origin)
ResolvedSource(getter, emptyList(), null, origin)
} else {
val transformation = symbols
.select(getter.type, target.type)
?.let { MappieViaResolved(it) }
?: tryGenerateMapper(getter.type, target.type)
?.also { generatedMappers.add(it) }
?.let { MappieViaGeneratedClass(it) }

.map { MappieViaResolved(it) }
.ifEmpty {
tryGenerateMapper(getter.type, target.type)
?.also { generatedMappers.add(it) }
?.let { listOf(MappieViaGeneratedClass(it)) }
?: emptyList()
}
ResolvedSource(
getter,
transformation,
transformation?.let { target.type },
transformation.let { if (transformation.isEmpty()) null else target.type },
origin,
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ fun logAll(problems: List<Problem>, location: CompilerMessageSourceLocation? = n
problems.forEach { log(it, location) }

fun log(problem: Problem, location: CompilerMessageSourceLocation?) =
when(problem.severity) {
Problem.Severity.ERROR -> logError(problem.description, problem.location ?: location)
Problem.Severity.WARNING -> logWarn(problem.description, problem.location ?: location)
when (problem.severity) {
Problem.Severity.ERROR -> logError(messageOf(problem), problem.location ?: location)
Problem.Severity.WARNING -> logWarn(messageOf(problem), problem.location ?: location)
}

fun logInfo(message: String, location: CompilerMessageSourceLocation? = null) =
Expand Down Expand Up @@ -53,3 +53,7 @@ fun location(file: IrFileEntry, element: IrElement) =
fun location(element: IrDeclaration) =
location(element.fileEntry, element)

private fun messageOf(problem: Problem) =
problem.description + System.lineSeparator() + problem.suggestions
.mapIndexed { i, it -> i + 1 to it }
.joinToString(separator = "") { " ${it.first}. ${it.second}" + System.lineSeparator() }
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ interface MappingValidation {
override val problems: List<Problem> =
buildList {
addAll(MultipleSourcesProblems.of(mapping).all())
addAll(MultipleTransformationsProblems.of(mapping).all())
addAll(UnsafeTypeAssignmentProblems.of(file, mapping).all())
addAll(UnsafePlatformTypeAssignmentProblems.of(file, mapping).all())
addAll(UnknownParameterNameProblems.of(mapping).all())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@ import org.jetbrains.kotlin.cli.common.messages.CompilerMessageLocation
data class Problem(
val description: String,
val severity: Severity,
val suggestions: List<String>,
val location: CompilerMessageLocation?
) {
enum class Severity { ERROR, WARNING; }

companion object {
fun error(description: String, location: CompilerMessageLocation? = null) =
Problem(description, Severity.ERROR, location)
fun error(description: String, location: CompilerMessageLocation? = null, suggestions: List<String> = emptyList()) =
Problem(description, Severity.ERROR, suggestions, location)

fun warning(description: String, location: CompilerMessageLocation? = null) =
Problem(description, Severity.WARNING, location)
fun warning(description: String, location: CompilerMessageLocation? = null, suggestions: List<String> = emptyList()) =
Problem(description, Severity.WARNING, suggestions, location)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package tech.mappie.validation.problems

import tech.mappie.resolving.ConstructorCallMapping
import tech.mappie.resolving.classes.*
import tech.mappie.resolving.classes.targets.MappieTarget
import tech.mappie.validation.Problem

class MultipleTransformationsProblems(private val mappings: Map<MappieTarget, List<ObjectMappingSource>>) {
fun all(): List<Problem> =
mappings.map { (_, sources) ->
val source = sources.single()
when (source) {
is ResolvedSource -> source.transformation
is PropertySource -> source.transformation
else -> null
}
}
.filterNotNull()
.map { transformations ->
val names = transformations.map { it as MappieViaResolved }.joinToString { it.definition.clazz.name.asString() }
Problem.error(
"Multiple mappers resolved to be used in an implicit via",
suggestions = listOf(
"Explicitly call one of $names explicitly.",
"Delete all except one of $names."
),
)
}

companion object {
fun of(mapping: ConstructorCallMapping): MultipleTransformationsProblems =
MultipleTransformationsProblems(
mapping.mappings
.filter { it.value.size == 1 }
.filter {
(it.value.single().let { it is ResolvedSource && it.transformation.size > 1 }) ||
(it.value.single().let { it is PropertySource && it.transformation.size > 1 })
}
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,40 @@ class ViaResolvingTest {
}
}

@Test
fun `map without explicit without via and two inner mappers should fail`() {
KotlinCompilation(directory).apply {
sources = buildList {
add(
kotlin("Test.kt",
"""
import tech.mappie.api.ObjectMappie
import tech.mappie.testing.objects.ViaResolvingTest.*
class Mapper : ObjectMappie<Input, Output>() {
override fun map(from: Input) = mapping {
to::text fromProperty from::text
}
}
class InnerMapperA : ObjectMappie<InnerInput, InnerOutput>()
class InnerMapperB : ObjectMappie<InnerInput, InnerOutput>()
"""
)
)
}
}.compile {
assertThat(exitCode).isEqualTo(ExitCode.COMPILATION_ERROR)
assertThat(messages).containsError(
"Multiple mappers resolved to be used in an implicit via",
listOf(
"Explicitly call one of InnerMapperA, InnerMapperB explicitly.",
"Delete all except one of InnerMapperA, InnerMapperB.",
)
)
}
}

@Test
fun `map without implicit without via and two inner mappers should fail`() {
KotlinCompilation(directory).apply {
Expand All @@ -71,8 +105,13 @@ class ViaResolvingTest {
}
}.compile {
assertThat(exitCode).isEqualTo(ExitCode.COMPILATION_ERROR)
assertThat(messages)
.containsError("Target Output::text automatically resolved from Input::text but cannot assign source type InnerInput to target type InnerOutput")
assertThat(messages).containsError(
"Multiple mappers resolved to be used in an implicit via",
listOf(
"Explicitly call one of InnerMapperA, InnerMapperB explicitly.",
"Delete all except one of InnerMapperA, InnerMapperB.",
)
)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,13 @@ import org.assertj.core.api.AbstractStringAssert
import java.util.regex.Pattern
import kotlin.text.Regex.Companion.escape

fun AbstractStringAssert<*>.containsError(message: String): AbstractStringAssert<*> =
containsPattern(Pattern.compile("e: file://.+ ${escape(message)}"))
fun AbstractStringAssert<*>.containsError(message: String, suggestions: List<String> = emptyList()): AbstractStringAssert<*> =
containsPattern(Pattern.compile("e: file://.+ ${escape(messageOf(message, suggestions))}"))

fun AbstractStringAssert<*>.containsWarning(message: String): AbstractStringAssert<*> =
containsPattern(Pattern.compile("w: file://.+ ${escape(message)}"))

private fun messageOf(message: String, suggestions: List<String>) =
message + System.lineSeparator() + suggestions
.mapIndexed { i, it -> i + 1 to it }
.joinToString(separator = "") { " ${it.first}. ${it.second}" + System.lineSeparator() }

0 comments on commit bdb58e6

Please sign in to comment.