-
Notifications
You must be signed in to change notification settings - Fork 40
Disable subtyping, introduce coercions, and type check core #1219
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.
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
Conversation
|
The following commutation also works: // [...]
val thn = box { block("first") }
val els = box { block("second") }
def res() /* at raise */ = if (true) {
thn()
} else {
els()
}
box res
}
// [...] |
|
I disabled subtyping in contravariant positions, since there we cannot (easily) insert coercions. In particular, we do not want to eta-expand functions to insert coercions. |
|
Current plan to insert coercions:
|
|
btw, in case we'd want other names than |
|
Quick status report: yesterday night, I tried to thread the expected type through the So I am currently tempted to go back to my original idea where Typer detects coercions and annotates them in the DB (yes... more side channeling---broken window). Ideally, typer would perform elaboration. However, if we have unification based (HM-like) type inference, this is a bit tricky to achieve in general. How do other languages (like Koka) implement elaboration if things might become known only much later (and non-locally)? It seems an elaborating typer is easier to implement in a bidirectional setting. |
|
Note: of course we can collect coercions also by we infer a coercion from |
|
TODOs
|
48d17b3 to
6005a10
Compare
…other debug utility, fix bugs related to result type modifying transformations
|
Explicitly annotating match with the motif together with limited checks of type-correctness in core surfaced a few bugs: A lot of times, transformations like |
|
The VMTests look roughly ok with one exception: branches = 210,
- pushedFrames = 68,
- poppedFrames = 67,
+ pushedFrames = 1,
+ poppedFrames = 1,
allocations = 0,
variableWrites = 88,
- resets = 2,
- shifts = 68,
- resumes = 66
+ resets = 1,
+ shifts = 1,
+ resumes = 0Before we generated this code in core: while now we generate The former is arguably better, since it is trivially tail resumptive (empty match) in any continuation other than |
| def unify(c1: Capture, c2: Capture, ctx: ErrorContext): Unit = unify(CaptureSet(Set(c1)), CaptureSet(Set(c2)), ctx) | ||
| def unifyCapture(c1: Capture, c2: Capture, ctx: ErrorContext): Unit = unifyCaptures(CaptureSet(Set(c1)), CaptureSet(Set(c2)), ctx) | ||
|
|
||
| def isZeroLike(tpe: ValueType): Boolean = tpe match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can delete those
| case (s: ValueType, ValueTypeRef(t: UnificationVar), Invariant) => requireEqual(t, s, ctx) | ||
| // coercing is the last resort... | ||
| case (TBottom, to, Covariant, Some(coerce)) => coerce(Coercion.FromNothing(to)) | ||
| case (from, TUnit, Covariant, Some(coerce)) => coerce(Coercion.ToUnit(from)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this
phischu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We fixed many things, this needs to be merged.
| pushedFrames = 1, | ||
| poppedFrames = 1, | ||
| pushedFrames = 68, | ||
| poppedFrames = 67, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge but should investigate this.
| enum Coercion { | ||
| case ToAny(from: ValueType) | ||
| case FromNothing(to: ValueType) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, let's merge but also investigate later whether we want to have coercions at all.
| tpesAndTerms.foreach { case (tpe, term) => | ||
| Context.requireSubtype(tpe, result, ErrorContext.Expected(tpe, result, term.getOrElse(Context.focus), | ||
| term.map(t => c => Context.coerce(t, c)))) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do not insert coercions, we can also delete this duplicate check here, which improves errors.
| * - each sequence contains a list of conditions that all have to match (conjunction). | ||
| */ | ||
| def compile(clauses: List[Clause]): core.Stmt = { | ||
| def compile(clauses: List[Clause], motif: ValueType): core.Stmt = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like "motif" but of course we can also use the english term "motive" that Conor used.
| } | ||
|
|
||
| @targetName("preserveTypesStmt") | ||
| inline def preserveTypes(before: Stmt)(inline f: Stmt => Stmt): Stmt = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep these in "production" as well? This might be costly.
|
|
||
| // These are always never traversed (accept when checking on the module level, so we use the free structure) | ||
| case Empty | ||
| case Append(lhs: Constraints, rhs: Constraints) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not yet done here (but can complete this in a follow up PR)! We need to actually check whether the constraints hold when encountering the declarations.
| // // TODO factor out callLike things | ||
| // shouldEqual(data, retType) | ||
| // val Typing(argTypes, argCapt, argFree) = all(vargs, typecheck) | ||
| // valuesShouldEqual(paramTypes, argTypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaving this here, since we will need a very similar logic when checking constraints at the declaration site.
| } | ||
| def setParent(u: UnionFind, child: Int, parent: Int) = { | ||
| with on[OutOfBounds].default { do raise(MissingValue(), "No node " ++ child.show) } | ||
| with default[OutOfBounds, Unit] { do raise(MissingValue(), "No node " ++ child.show) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit sad that we need the type annotations here.
|
|
|
I retract. The LLVM tests using files are still failing on my Mac M1. I would be nice to have an ARM runner; maybe we could set up one at some point. |
There is a small caveat: I only enabled these for the PR CI, not for the `master` branch CI. If interested in the latter, feel free to request that too. --- To be used in #1219. - run all IO tests on macOS with LLVM - run with `--debug` flag - takes some 3m30s (like JS tests) Here's the log: <img width="645" height="274" alt="Screenshot 2025-12-13 at 17 50 31" src="https://github.com/user-attachments/assets/df402a2c-bbb3-400f-94d4-da9ec511418d" />
b-studios
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should cherrypick / rebase the CI test added to master
There is a small caveat: I only enabled these for the PR CI, not for the `master` branch CI. If interested in the latter, feel free to request that too. --- To be used in #1219. - run all IO tests on macOS with LLVM - run with `--debug` flag - takes some 3m30s (like JS tests) Here's the log: <img width="645" height="274" alt="Screenshot 2025-12-13 at 17 50 31" src="https://github.com/user-attachments/assets/df402a2c-bbb3-400f-94d4-da9ec511418d" />

See the PR comments.