Skip to content
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

Add code generation for bbml #245

Merged
merged 36 commits into from
Dec 12, 2024
Merged

Conversation

NeilKleistGao
Copy link
Member

@NeilKleistGao NeilKleistGao commented Dec 1, 2024

In this PR:

  • Generate code for mutable reference operations in bbml
  • Fix some problems with BbML typing
  • Map builtin operators and floating operators
  • Add predef for BbML
  • Add getter code generation and corresponding infrastructures

However, I'm not sure if we still need to make get a keyword. There are already some functions using get as the name and it seems hard to repick names for them.

@LPTK
Copy link
Contributor

LPTK commented Dec 2, 2024

There are already some functions using get as the name and it seems hard to repick names for them.

Which functions are you talking about? They should all be in this repo and so the migration should be easy.

Perhaps we could just use a naming scheme. Functions called get_XXX generate a getter for XXX and similarly for set_XXX.

fun get_x() = _x
fun set_x(value) = set _x = value

BTW I noticed that in JS get is not actually a keyword. So yet another alternative approach in a similar spirit as JS:

fun (get) x() = _x
fun (set) x(value) = set _x = value

(it looks a bit weird)

Anyway, this should be done in a separate PR.

@LPTK
Copy link
Contributor

LPTK commented Dec 2, 2024

What's the rationale for adding a competing Predef for BbML? Can't we use the existing one, and add all functionality needed by BbML in some separate BbPredef module?

@NeilKleistGao
Copy link
Member Author

What's the rationale for adding a competing Predef for BbML? Can't we use the existing one, and add all functionality needed by BbML in some separate BbPredef module?

So far only checkArgs in the predef is required. I added a new implementation that can be type checked in Bbml (even though the type-checking for importing is still not done). Or do we plan to use the untyped predef everywhere?

@LPTK
Copy link
Contributor

LPTK commented Dec 3, 2024

Ah you mean that the current Predef doesn't type check. Then I guess this design is fine for now.

@NeilKleistGao NeilKleistGao marked this pull request as ready for review December 6, 2024 03:21
Copy link
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, but please see my comment.

Also, we should really now look into using Scope to generate proper TV names without a counter. Currently, the counter makes diffs like this one very noisy.

@LPTK
Copy link
Contributor

LPTK commented Dec 6, 2024

I just realized we must forbid getters from being defined in non-module and non-function scopes. Indeed, they cannot be used correctly if we don't know their symbol.

fun foo(f) =
  f.oops

class Foo(x) with
  fun oops = x

foo(Foo(42))
//│ = [Function: oops]

foo(Foo(42))()
//│ FAILURE: Unexpected runtime error
//│ ═══[RUNTIME ERROR] TypeError: Cannot read properties of undefined (reading 'x')
//│     at oops (REPL25:1:176)
//│     at REPL27:1:80
//│     at ContextifyScript.runInThisContext (node:vm:136:12)
//│     at REPLServer.defaultEval (node:repl:598:22)
//│     at bound (node:domain:432:15)
//│     at REPLServer.runBound [as eval] (node:domain:443:12)
//│     at REPLServer.onLine (node:repl:927:10)
//│     at REPLServer.emit (node:events:520:28)
//│     at REPLServer.emit (node:domain:488:12)
//│     at [_onLine] [as _onLine] (node:internal/readline/interface:416:12)

Note that a property of module methods is that we always know their symbol when calling them.

Copy link
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, sorry that the getter implementation is unexpectedly blocking this PR. It would have been better to make it in a separate PR. Anyway, let's fix it now.

There are also still problems with your hack around globalThis. I still don't know what it achieves/what it's for.

hkmc2/shared/src/main/scala/hkmc2/semantics/Symbol.scala Outdated Show resolved Hide resolved
hkmc2/shared/src/main/scala/hkmc2/semantics/Term.scala Outdated Show resolved Hide resolved
hkmc2/shared/src/test/mlscript/codegen/FunctionsThis.mls Outdated Show resolved Hide resolved
hkmc2/shared/src/main/scala/hkmc2/codegen/Lowering.scala Outdated Show resolved Hide resolved
@NeilKleistGao NeilKleistGao requested a review from LPTK December 12, 2024 04:01
Copy link
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great changes, thanks! Please fix the few remaining issues and I'll make the merge.

hkmc2/shared/src/main/scala/hkmc2/semantics/Symbol.scala Outdated Show resolved Hide resolved
hkmc2/shared/src/test/mlscript/codegen/Getters.mls Outdated Show resolved Hide resolved
@LPTK LPTK merged commit d427826 into hkust-taco:hkmc2 Dec 12, 2024
1 check passed
@LPTK LPTK deleted the bbml-codegen🧃 branch December 12, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants