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.

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

Jump to bottom

feat(compiler)!: foreign module syntax #1873

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

feat(compiler)!: foreign module syntax #1873

wants to merge 1 commit into from

Conversation

alex-snezhko
Copy link
Member

Closes #1052 with modified syntax as discussed in Discord

module A

// use as a submodule
foreign module Namespace binds "namespace" {
  foreign value myFn binds "my_fn": WasmI32 => WasmI32
}

// use as a toplevel module
foreign module Namespace binds "namespace"

foreign value myFn binds "my_fn": WasmI32 => WasmI32

spotandjake
spotandjake reviewed Jun 26, 2023
View reviewed changes
Copy link
Member

@spotandjake spotandjake left a comment •
edited

Choose a reason for hiding this comment

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

Looks mostly good, I did have a few small questions though.

Great job on this 👏 , insane turn around time.

compiler/src/parsing/parser.mly
@@ -37,7 +37,8 @@ module Grain_parsing = struct end
%token <string> PREFIX_150
%token <string> INFIX_ASSIGNMENT_10

%token ENUM RECORD TYPE MODULE INCLUDE USE PROVIDE ABSTRACT FOREIGN WASM PRIMITIVE
%token FOREIGN_VALUE FOREIGN_MODULE
%token ENUM RECORD TYPE MODULE INCLUDE USE PROVIDE ABSTRACT FOREIGN PRIMITIVE BINDS
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this pr get rid of the FOREIGN token in the lever, so shouldn't we remove it here?

Copy link
Member

Choose a reason for hiding this comment

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

Haven't read through the implementation yet, but at least what we discussed was keeping that token to help with error messages.

Copy link
Member

Choose a reason for hiding this comment

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

Haven't read through the implementation yet, but at least what we discussed was keeping that token to help with error messages.

well need to keep it in the lexar than as well right?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ospencer How could we use it in error messages? Wouldn't that token need to show up in a parse rule?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. What we want to enable is a proper error message when someone writes, for example, foreign foo. The compiler should tell you that after the foreign keyword, there should be the keyword module or value.

compiler/src/parsing/parser.mly
foreign_binds:
| lseparated_nonempty_list(eos, foreign_bind) eos? { $1 }

foreign_module_stmt:
Copy link
Member

Choose a reason for hiding this comment

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

could we change

FOREIGN_MODULE UIDENT BINDS str lbrace foreign_binds

to

foreign_module_header lbrace foreign_binds

so we ensure the pattern is the same for both?

If this makes sense we should probably do the same thing for normal modules in a separate pr.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say that's reasonable. I'll also just go ahead and change normal modules in this PR since it's basically a one line change anyways

Copy link
Member

Choose a reason for hiding this comment

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

Alright, my thoughts are if we ever go back and change the syntax its now one change instead of two.

stdlib/runtime/debug.gr
@@ -1,4 +1,6 @@
/* grainc-flags --compilation-mode=runtime */
module Debug

provide foreign wasm debug: a => Void from "console"
provide foreign module Console binds "console" {
foreign value debug binds "debug": a => Void
Copy link
Member

Choose a reason for hiding this comment

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

Should we be allowing an a type on foreigns?

I thought the only types we wanted to allow are WasmI32, WasmI64, WasmF32, WasmF64 and Void

It looks like we did before with the old syntax but Im wondering if we should change that with the new syntax. I don't know where the best place to change that would be, maybe typed_well_formedness, should enforce that you are only using the primitive unsafe types. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm actually not even sure why this file exists as it doesn't appear to be used anywhere; what you're suggesting sounds like an interesting idea, though I think this would require some nontrivial changes to the compiler to be able to recognize a function call to a foreign vs a normal function. What are your thoughts @ospencer @phated

Copy link
Member

Choose a reason for hiding this comment

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

The old Grain js runner had a debug thing and this is how you'd use it. I don't remember if we merged that PR yet.

Should we be allowing an a type on foreigns?

No, and we discussed that (as a thing that we will do/warn against), but that work does not need to happen in this PR (and needs some more thought).

stdlib/runtime/wasi.gr
@@ -1,47 +1,36 @@
/* grainc-flags --no-gc */
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to have this set to --no-gc?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but I figured I'd just keep this file (as well as the other wasi files) like this for now since it will have to be rectified with #1804 anyways

stdlib/runtime/wasiConstants.gr
@@ -0,0 +1,36 @@
/* grainc-flags --no-gc */
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this module just be marked as unsafe, rather than --no-gc

stdlib/runtime/wasiErrors.gr
@@ -0,0 +1,184 @@
/* grainc-flags --no-gc */
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spotandjake spotandjake spotandjake left review comments

@ospencer ospencer Awaiting requested review from ospencer ospencer is a code owner

@peblair peblair Awaiting requested review from peblair peblair is a code owner

@jozanza jozanza Awaiting requested review from jozanza jozanza is a code owner

@phated phated Awaiting requested review from phated phated is a code owner

@marcusroberts marcusroberts Awaiting requested review from marcusroberts marcusroberts is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@alex-snezhko alex-snezhko

Labels
compiler syntax
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New foreign syntax
3 participants
@alex-snezhko @ospencer @spotandjake