Skip to content

Add checks that overrides are compatible with overridden signatures #350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
mikesamuel wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add checks that overrides are compatible with overridden signatures #350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions common/src/commonMain/kotlin/lang/temper/common/Collections.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,15 @@ inline fun <T : Any, reified U : T> Iterable<T>.firstOrNullAs(predicate: (U) ->
return null
}

inline fun <T : Any, reified U : T> Iterable<T>.filterAs(predicate: (U) -> Boolean): List<U> =
buildList {
for (x in this@filterAs) {
if (x is U && predicate(x)) {
add(x)
}
}
}

class ConcatenatedListView<T>(
private val left: List<T>,
private val right: List<T>,
Expand Down
166 changes: 133 additions & 33 deletions frontend/src/commonMain/kotlin/lang/temper/frontend/generate/TypeDeclChecker.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ import lang.temper.type.Visibility
import lang.temper.type.WellKnownTypes
import lang.temper.type2.DefinedNonNullType
import lang.temper.type2.MkType2
import lang.temper.type2.Nullity
import lang.temper.type2.Signature2
import lang.temper.type2.SuperTypeTree2
import lang.temper.type2.mapType
import lang.temper.type2.withNullity
import lang.temper.value.connectedSymbol
import lang.temper.value.parameterNameSymbols

Expand All @@ -25,25 +27,31 @@ internal class TypeDeclChecker(val module: Module, val logSink: LogSink) {

fun checkDeclaredTypeShapes() {
for (typeShape in module.declaredTypeShapes) {
when (typeShape.abstractness) {
Abstractness.Abstract -> {}
Abstractness.Concrete -> checkAllMethodsOverridden(typeShape)
}
checkTypeShape(typeShape)
}
}

private fun checkAllMethodsOverridden(typeShape: TypeShape) {
val superTypeShapes = mutableSetOf<TypeShape>()
fun walk(ts: TypeShape) {
if (ts !in superTypeShapes) {
superTypeShapes.add(ts)
for (x in ts.superTypes) {
walk(x.definition as TypeShape)
fun checkTypeShape(typeShape: TypeShape) {
val superTypeShapes = buildSet {
fun walk(ts: TypeShape) {
if (ts !in this) {
add(ts)
for (x in ts.superTypes) {
walk(x.definition as TypeShape)
}
}
}
walk(typeShape)
}

when (typeShape.abstractness) {
Abstractness.Abstract -> {}
Abstractness.Concrete -> checkAllMethodsOverridden(typeShape, superTypeShapes)
}
walk(typeShape)
checkOverridesCompatible(typeShape, superTypeShapes)
}

private fun checkAllMethodsOverridden(typeShape: TypeShape, superTypeShapes: Set<TypeShape>) {
val isProcessingImplicits = module.isEffectivelyImplicits
val isStd = module.isEffectivelyStd
val allAbstractMethodDescriptors = mutableListOf<MethodDescriptor>()
Expand Down Expand Up @@ -77,27 +85,9 @@ internal class TypeDeclChecker(val module: Module, val logSink: LogSink) {
val word = descriptor.word
val example = descriptor.example
val kind = descriptor.methodKind
val description = when (kind) {
MethodKind.Normal -> word.text
MethodKind.Getter -> "get ${word.text}"
MethodKind.Setter -> "set ${word.text}"
MethodKind.Constructor -> "constructor"
}
val sigInContext = run {
val type = MkType2(typeShape).actuals(
typeShape.formals.map { MkType2(it).get() },
).get() as DefinedNonNullType
val superTypeInContext = SuperTypeTree2.of(type)[example.enclosingType].firstOrNull()
var sig = example.descriptor
?: Signature2(WellKnownTypes.voidType2, false, listOf())
if (sig.hasThisFormal) { sig = sig.copy(requiredInputTypes = sig.requiredInputTypes.drop(1)) }
if (superTypeInContext != null) {
val bindings = (example.enclosingType.formals zip superTypeInContext.bindings).associate { it }
sig = sig.mapType(bindings)
}
sig
}

val description = methodDescription(kind, word)
val sigInContext = sigInContext(typeShape, example)
?: Signature2(WellKnownTypes.voidType2, false, listOf())
val skeletonCode = buildString {
append("public $description(")
val parameterNameSymbols = example.parameterNameSymbols?.let {
Expand All @@ -123,6 +113,109 @@ internal class TypeDeclChecker(val module: Module, val logSink: LogSink) {
)
}
}

private fun checkOverridesCompatible(typeShape: TypeShape, superTypeShapes: Set<TypeShape>) {
if (WellKnownTypes.isWellKnown(typeShape)) {
// TODO: cleanup implicits so it runs clean. GeneratorResult.next and SafeGeneratorResult.next
// are problematic because one Bubbles and one does not.
return
}

for (method in typeShape.methods) {
val sig = method.descriptor?.let { adjustOptionalToNullable(it) } ?: continue
val methodKind = method.methodKind
if (methodKind == MethodKind.Constructor) { continue }
for (superTypeShape in superTypeShapes) {
if (superTypeShape == typeShape) { continue }
for (m in superTypeShape.membersMatching(method.symbol)) {
if (m is MethodShape && m.visibility != Visibility.Private && m.methodKind == methodKind) {
if (method.visibility < m.visibility) {
logSink.log(
Log.Error,
MessageTemplate.IncompatibleVisibility,
method.stay?.pos ?: typeShape.pos,
listOf(
typeShape.name,
methodDescription(method.methodKind, method.symbol),
method.visibility.keyword,
m.enclosingType.name,
),
)
}
val superSigInContext = sigInContext(typeShape, m)?.let { adjustOptionalToNullable(it) }
if (superSigInContext != null && superSigInContext != sig) {
logSink.log(
Log.Error,
MessageTemplate.IncompatibleSignature,
method.stay?.pos ?: typeShape.pos,
listOf(
typeShape.name,
methodDescription(method.methodKind, method.symbol),
sigDescription(sig, method),
sigDescription(superSigInContext, m),
m.enclosingType.name,
),
)
}
}
}
}
}
}

private fun sigInContext(subTypeShape: TypeShape, superTypeMethod: MethodShape): Signature2? {
var sig = superTypeMethod.descriptor ?: return null
val subType = MkType2(subTypeShape).actuals(
subTypeShape.formals.map { MkType2(it).get() },
).get() as DefinedNonNullType
val superTypeShape = superTypeMethod.enclosingType
val superTypeInContext = SuperTypeTree2.of(subType)[superTypeShape].firstOrNull()
sig = sig.withoutThisFormal()
if (superTypeInContext != null) {
val bindings = (superTypeShape.formals zip superTypeInContext.bindings).associate { it }
sig = sig.mapType(bindings)
}
return sig
}

private fun adjustOptionalToNullable(sig: Signature2): Signature2 {
var adjusted = sig
adjusted = sig.withoutThisFormal()
if (adjusted.optionalInputTypes.isNotEmpty()) {
adjusted = adjusted.copy(
requiredInputTypes = buildList {
addAll(adjusted.requiredInputTypes)
for (t in adjusted.optionalInputTypes) {
add(t.withNullity(Nullity.OrNull))
}
},
optionalInputTypes = listOf(),
)
}
return adjusted
}

private fun methodDescription(kind: MethodKind, word: Symbol) = when (kind) {
MethodKind.Normal -> word.text
MethodKind.Getter -> "get ${word.text}"
MethodKind.Setter -> "set ${word.text}"
MethodKind.Constructor -> "constructor"
}

private fun sigDescription(sig: Signature2, m: MethodShape): String = buildString {
append(m.symbol.text)
val parameterInfo = m.parameterInfo?.names
append("(")
for ((i, formal) in sig.allValueFormals.withIndex()) {
if (i != 0) { append(", ") }
val name = parameterInfo?.getOrNull(i + 1)?.text ?: "_" // Skip over `this`
append(name)
append(": ")
append(formal.type)
}
append("): ")
append(sig.returnType2)
}
}

private class MethodDescriptor(
Expand All @@ -138,3 +231,10 @@ private class MethodDescriptor(

override fun toString(): String = "MethodDescriptor($word, $methodKind)"
}

fun Signature2.withoutThisFormal(): Signature2 =
if (hasThisFormal) {
copy(requiredInputTypes = requiredInputTypes.drop(1), hasThisFormal = false)
} else {
this
}
48 changes: 47 additions & 1 deletion frontend/src/commonTest/kotlin/lang/temper/frontend/GenerateCodeStageTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3714,7 +3714,7 @@ class GenerateCodeStageTest {
)

@Test
fun pureVirtualMethodInConcreteClass() = assertModuleAtStage(
fun errorMessageOnMissingOverride() = assertModuleAtStage(
stage = Stage.Run,
input = """
|export interface I<T> { f(x: T): Void; }
Expand All @@ -3729,6 +3729,52 @@ class GenerateCodeStageTest {
|}
""".trimMargin(),
)

@Test
fun errorMessageOnBadOverride() = assertModuleAtStage(
stage = Stage.Run,
input = """
|export interface I<T> { f(x: T): Void; }
|export class A extends I<String> {
| public f(x: String): Void {} // OK
|}
|export class B extends I<String> {
| public f(x: Int32): Void {} // Wrong type
|}
|export class C extends I<String> {
| public f(x: String?): Void {} // Nullable. Mismatch on optioning backends
|}
|export class D extends I<String> {
| public f(x: String): String { x } // Return type mismatch
|}
|export class E extends I<String> {
| public f(): Void {} // Too few params
|}
|export class F extends I<String> {
| public f(x: String, y: String): Void {} // Too many params
|}
|export interface G extends I<String> {
| f(x: Int32): Void; // Same as D but not a concrete type.
|}
|export class H extends I<String> {
| private f(x: String): Void {} // Reduced visibility
|}
""".trimMargin(),
want = """
|{
| run: "void: Void",
| errors: [
| "Type B has method f with signature f(x: Int32): Void, but it should be f(x: String): Void to correctly override from I!",
| "Type C has method f with signature f(x: String?): Void, but it should be f(x: String): Void to correctly override from I!",
| "Type D has method f with signature f(x: String): String, but it should be f(x: String): Void to correctly override from I!",
| "Type E has method f with signature f(): Void, but it should be f(x: String): Void to correctly override from I!",
| "Type F has method f with signature f(x: String, y: String): Void, but it should be f(x: String): Void to correctly override from I!",
| "Type G has method f with signature f(x: Int32): Void, but it should be f(x: String): Void to correctly override from I!",
| "Type H has method f but it's visibility is private which is narrower than the method it overrides in I!"
| ]
|}
""".trimMargin(),
)
}

// Provide an extra binding to a function whose call does not inline so does not trigger any
Expand Down
8 changes: 8 additions & 0 deletions log/src/commonMain/kotlin/lang/temper/log/MessageTemplate.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,14 @@ enum class MessageTemplate(
"Cannot initialize an incomplete declaration",
CompilationPhase.Interpreter,
),
IncompatibleVisibility(
"Type %s has method %s but it's visibility is %s which is narrower than the method it overrides in %s",
CompilationPhase.Interpreter,
),
IncompatibleSignature(
"Type %s has method %s with signature %s, but it should be %s to correctly override from %s",
CompilationPhase.Interpreter,
),
MalformedActual(
"Formal argument where actual expected. `:` only applies to function parameters",
CompilationPhase.Interpreter,
Expand Down
Loading