Panic (correctly now?) on missing else for exhaustive sealed when#452
Conversation
Signed-off-by: Tom <tom@temper.systems>
Signed-off-by: Tom <tom@temper.systems>
Signed-off-by: Tom <tom@temper.systems>
Signed-off-by: Tom <tom@temper.systems>
Signed-off-by: Tom <tom@temper.systems>
Signed-off-by: Tom <tom@temper.systems>
Signed-off-by: Tom <tom@temper.systems>
| | if (e == 2) { | ||
| | m = new MapBuilder<String, Int>() | ||
| | m = new MapBuilder<String, Int>(); | ||
| | } |
There was a problem hiding this comment.
Without the semicolon, there was complaint about the void in the secret else block. Also some new statements below. The change is from no longer throwing a void after the if/else if chain as a whole. More on that later.
But it would be nice at least to improve error messaging for things like missing semicolon here.
| |[interactive#0:1+22-41]@G: Cannot assign to Int32 from Void | ||
| |1: when (i) { 1 -> 1 } } | ||
| | ⇧ | ||
| |[interactive#0:1+41]@G: Cannot assign to Int32 from Void |
There was a problem hiding this comment.
This also is from no longer putting the void after the if chain. The secret else block got and still gets the trailing position. But the trailing void got a larger range. If I put the whole range on the added else { void }, then other tests break that expect the trailing pos. I went with trailing pos between the two, so this test had to change.
| } | ||
|
|
||
| // With type info, we can finalize `else` values. | ||
| replaceVoidishPanics(root) |
There was a problem hiding this comment.
Helper just added to this file below because it's small.
There was a problem hiding this comment.
We've got other micro passes in the benchmark and snapshot pattern. Could you add an entry to logger-names.json, run gradle kcodegen:updateGeneratedCode, and use that pattern here?
There was a problem hiding this comment.
We've got other micro passes in the benchmark and snapshot pattern. Could you add an entry to logger-names.json, run
gradle kcodegen:updateGeneratedCode, and use that pattern here?
I'll add it. I haven't checked, but I really hope our snapshotter is null for the cli-facing compiler.
| TreeVisit.startingAt(root).forEach tree@{ tree -> | ||
| tree is CallTree || return@tree VisitCue.Continue | ||
| tree.size == expectedTreeSize || return@tree VisitCue.Continue | ||
| tree.child(0).functionContained == VoidishPanicFn || return@tree VisitCue.Continue |
There was a problem hiding this comment.
I more and more like this guard chain style of coding. I hope others don't find it obtuse. To me, it reads as a chain of conditions.
There was a problem hiding this comment.
It's an idiom I've adapted to.
I'm also used to break if condition; and die unless condition; style from Perl, so having a one-line idiom for this kind of thing seems reasonable to me.
| // We don't support subtyping the same interface under different type actuals, | ||
| // so simple subtyping is fine here. Any broken type actuals will cause static | ||
| // type errors elsewhere. | ||
| isSimpleSubtype(foundType, expectedType) || return@exhaustive null |
There was a problem hiding this comment.
This also is a very simple function below. We really don't need all the extra logic from TypeContext or such, if I've thought through it correctly, as mentioned in the comment.
| | errors: [ | ||
| | "Cannot assign to String from Void!", | ||
| | "Expected subtype of String, but got Void!", | ||
| | "Void expressions cannot be used as values!", |
There was a problem hiding this comment.
These from the void case. Before fixing things, I got this trio twice, once for each of the functions above. Feel free to review commit history if interested.
| | let t#0; | ||
| | t#0 = (fn g)(s__1); | ||
| | return__1 = is(t#0, NoStringIndex) | ||
| | return__1 = is((fn g)(s__1), NoStringIndex) |
There was a problem hiding this comment.
This is from the more thorough truthiness, but seems to be the only case we get of improvement from that. This is generate code stage, and I think the later simplify rounds get most cases.
| override val sigs = nullaryNeverReturnsSigs( | ||
| requiredInputTypes = listOf(WKT.anyValueType2, WKT.typeType2), | ||
| ) { it } | ||
| } |
There was a problem hiding this comment.
Put in a reified type for the expected type so I wouldn't have to mess with the expected type formals of nullary functions. But I didn't try it in other forms, so maybe it would have been fine. In any case, this works, too.
| } || return null | ||
| // Yep, got it. | ||
| return !value | ||
| } |
There was a problem hiding this comment.
I didn't add a type stage test for this, but it gets a fair amount of if/else chain simplification done at type stage with this, because we have lots of if (!false) instead of if (true) at that point.
I was thinking that I might try some very simple post-typer flow typing, and the simplified chain from this made it look easier to do. I ended up not doing anything with it, but if we theoretically want function macro stage to see simpler code, this helps.
If we want tests of before and after type stage, let me know. Or if we want to remove this adjustment from this pr, also let me know.
There was a problem hiding this comment.
For reference, here's a manual capture of just after simplify 2 in type stage both before the above change:
`test//`.describeShape = (@stay fn describeShape(s__13 /* aka s */: Shape) /* return__7 */: String {
var t#23 ⦂ Boolean, t#24 ⦂ Boolean, t#25 ⦂ String;
fn__14: do {
if (!false) {
t#23 = is(s__13, Circle)
} else {
t#23 = false
};
if (t#23) {
t#25 = "circle"
} else {
if (!false) {
t#24 = is(s__13, Square)
} else {
t#24 = false
};
if (t#24) {
t#25 = "square"
} else {
t#25 = panic ⋖ String ⋗()
}
};
return__7 = t#25
}
})And after:
`test//`.describeShape = (@stay fn describeShape(s__13 /* aka s */: Shape) /* return__7 */: String {
var t#23 ⦂ Boolean, t#24 ⦂ Boolean, t#25 ⦂ String;
fn__14: do {
t#23 = is(s__13, Circle);
if (t#23) {
t#25 = "circle"
} else {
t#24 = is(s__13, Square);
if (t#24) {
t#25 = "square"
} else {
t#25 = panic ⋖ String ⋗()
}
};
return__7 = t#25
}
})| }.edge(0) | ||
| macroCursor.referenceTo(panicCall) | ||
| } | ||
| }.let { ControlFlow.Stmt(it) } |
There was a problem hiding this comment.
This is fairly close to the logic in #439 but refines some. Especially with the "voidish panic" for later cleanup.
| | if (e == 2) { | ||
| | m = new MapBuilder<String, Int>() | ||
| | m = new MapBuilder<String, Int>(); | ||
| | } |
| } | ||
|
|
||
| // With type info, we can finalize `else` values. | ||
| replaceVoidishPanics(root) |
There was a problem hiding this comment.
We've got other micro passes in the benchmark and snapshot pattern. Could you add an entry to logger-names.json, run gradle kcodegen:updateGeneratedCode, and use that pattern here?
| TreeVisit.startingAt(root).forEach tree@{ tree -> | ||
| tree is CallTree || return@tree VisitCue.Continue | ||
| tree.size == expectedTreeSize || return@tree VisitCue.Continue | ||
| tree.child(0).functionContained == VoidishPanicFn || return@tree VisitCue.Continue |
There was a problem hiding this comment.
It's an idiom I've adapted to.
I'm also used to break if condition; and die unless condition; style from Perl, so having a one-line idiom for this kind of thing seems reasonable to me.
| private fun isSimpleSubtype(sub: NominalType, sup: NominalType): Boolean { | ||
| sub.definition == sup.definition && return true | ||
| return sub.definition.superTypes.any { isSimpleSubtype(it, sup) } | ||
| } |
There was a problem hiding this comment.
Ok, so given a sub from <T extends Foo> and sup = Foo, this'll work.
There was a problem hiding this comment.
Ok, so given a sub from
<T extends Foo>and sup = Foo, this'll work.
I've manually checked this now with export let describeShape<T extends Shape>(s: T): String instead of export let describeShape(s: Shape): String, and it still claims compatibility and provides the panic instead of void, because of how TypeFormal.supertypes works, which I think is correct. But we also get errors like "Unrelated types cannot be targeted with is or as runtime type checks: \u003c[Circle]\u003e from T__0!", which I think also is correct.
I think I won't push the adjusted test, though.
Signed-off-by: Tom <tom@temper.systems>
elsebecause that's harder now, but that affects positions and also makes semicolon more vitaltruthinesssmarter for simplify control flow, but not really taking advantage of that here