-
Notifications
You must be signed in to change notification settings - Fork 23
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
Semantic analysis (typed ASTs, type checking, etc.) #3333
Conversation
Plot.numericFn(fn: (Number) => Number, params?: {xScale?: Scale, yScale?: Scale, title?: String, xPoints?: List(Number)}) => Plot | ||
Was given arguments: ((x,y) => internal code)` | ||
Plot.numericFn(fn: (Number) => Number, params?: {xScale?: Scale, yScale?: Scale, xPoints?: List(Number)}) => Plot | ||
Was given arguments: ((x: any, y: any) => Duration|Number|Dist))` |
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.
Note how lambdas are now serialized to their inferred signatures.
(limitation: parameter annotations don't affect parameter types yet)
TypedASTNode, | ||
} from "./types.js"; | ||
|
||
export abstract class Node<T extends string> { |
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 have the separate class hierarchy for typed AST, while the old AST is still represented as basic JS objects.
I'm not sure that this is optimal; it causes more code duplication than I'd like.
But fixing this is another 1-2 days task, and there are some details why it's not entirely straightforward, so I'll do it some time later.
type: FrType<T>; | ||
}; | ||
|
||
export class FrInput<IsOptional extends boolean, Unpacked = unknown> { |
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.
The name is annoying because we also had frInput
which is "fr type for VInput". Because of this, I renamed frInput
to frFormInput
, and we probably should rename VInput
to VFormInput
too.
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.
Really looking forward to LLM systems that could do broader refactors for gradually improving things like these.
@@ -0,0 +1,158 @@ | |||
/** | |||
* An "IR" is an intermediate representation of a Squiggle code. |
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 renamed "Expression" to "IR", since "Expression" was also used in context of AST nodes.
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 a bit conflicted that this is so short (hard to search for, can be confusing). But IR is a standard term, so seems fine.
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.
Most changes in the compiler are because I renamed expression/
to compiler/
and reorganized stuff into multiple files, but I also make use typed AST data, e.g. pre-resolved identifiers.
kind: "Program", | ||
imports: imports.map((imp) => assertKind(imp, "Import")), | ||
statements: statements.map(assertStatement), | ||
result: result ? assertExpression(result) : null, |
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.
All these assertions are not necessary if Peggy grammar is correct, but we don't have type safety in Peggy, and the performance hit is negligible.
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.
Another techincally unrelated big change in this PR.
I needed to throw errors during analysis stage that are similar to runtime errors and I didn't want to implement several new classes. So instead we now have a single ErrorMessage
class.
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 removed frTypes
first and implemented /types/*
, but then decided to bring frTypes back because I didn't want to keep pack/unpack on type classes (generics were getting too complicated).
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.
Moved to reducer/lambda/FnDefinition.ts
and heavily refactored.
@@ -53,4 +55,6 @@ export class VLambda extends BaseValue<"Lambda", number> implements Indexable { | |||
// deserialization is implemented in ./serialize.ts, because of circular import issues. | |||
} | |||
|
|||
export const vLambda = (v: Lambda) => new VLambda(v); | |||
export function vLambda(v: Lambda) { |
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.
Btw, this is an interesting case where function vNambda
instead of arrow function helped with recursive import issues.
* don't want to implement strict type checks, we just want to make sure that | ||
* there's a chance that the code won't fail. | ||
*/ | ||
export function typeCanBeAssigned(type1: Type, type2: Type): boolean { |
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.
This function is the heart of our type checking system. High priority for reviewing!
No description provided.