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 a general purpose console.log() import. #11

Closed
wants to merge 1 commit into from

Conversation

Terr
Copy link

@Terr Terr commented Nov 10, 2023

Many WebAssembly tutorials, including the example code on MDN, use a "console" "log" import. I thought it would be good to add this import to avoid confusion when people copy/paste example code into their Exercism exercise.

WasmRunner has plenty of alternative logging functions (including log_i32_u() that is also just a console.log() call) but it's not obvious they exist if you don't look into this source file. And if you're a WASM beginner (like me) you might not even realize that the "console" "log" import has to be defined somewhere ahead of time.

Many WebAssembly tutorials, including the example code on MDN, use a
`"console" "log"` import. I thought it would be good to add this import
to avoid confusion when people copy/paste example code into their
Exercism exercise.

`WasmRunner` has plenty of alternative logging functions (including
`log_i32_u()` that is also just a `console.log()` call) but it's not
obvious they exist if you don't look into this source file. And if
you're a WASM beginner (like me) you might not even realize that the
`"console" "log"` import has to be defined somewhere ahead of time.
Copy link

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Nov 10, 2023
@iHiD iHiD reopened this Nov 11, 2023
@iHiD
Copy link
Member

iHiD commented Nov 11, 2023

@bushidocodes
Copy link
Contributor

Hi @Terr, I appreciate the PR!

Unfortunately, we can't accept this. I understand that this might be frustrating to you, but I would like to express that it is great that you are trying to give back. I'd like to explain my rationale for why I don't think we should accept this and offer a suggestion for an alternative area to redirect your effort.

Why we can't accept this:

Your new console::log import is a function that takes an i32 and returns void. This likely works for the examples that you have been working on, but it will not work for the other wasm types. Specifically, you can pass i32, but you will not be able to compile if you try to pass your function an i64, f32, or f64. In addition, the integer types i32 and i64 are ambiguous as to if they are signed or not. This is why there are signed and unsigned instructions for these types. I actually do not know what the signed versus unsigned default would be for your instruction (You can test this by trying to log 0xFFFFFFFF and seeing if you get 4294967295 or -1). Finally, because i32, i64, f32, and f32 are the only wasm types, our Exercism challenges need to come up with our own custom object model for things like strings. We've adopted spans (base offset type i32, length i32) over the wasm linear memory containing UTF-8 encoded text as our standard.

You might be surprised by this given that you are modeling off the examples on MDN. Looking at the examples there, they only deal with a single instruction and log is only able to log a single type.

For example, if you go to https://developer.mozilla.org/en-US/docs/WebAssembly/Reference/Numeric/Subtraction

and you replace the WAT in the demo with the following:

(module
  (import "console" "log" (func $log (param i32)))
  (func $main
    i64.const 10
    call $log ;; log the result
  )
  (start $main)
)

you will error out with the following:

> Error: validate failed:
5:5: error: type mismatch in call, expected [i32] but got [i64]
    call $log ;; log the result
    ^^^^

You can change the type in the import

(module
  (import "console" "log" (func $log (param i64)))
  (func $main
    i64.const 10
    call $log ;; log the result
  )
  (start $main)
)

And now this seems to work:

> 10n

There are still some problems here though. For example, signedness.

(module
  (import "console" "log" (func $log (param i32)))
  (func $main
    i32.const 4294967295
    call $log ;; log the result
    i32.const -1
    call $log ;; log the result
  )
  (start $main)
)

Both values are logged as signed integers (likely because JS doesn't really support first-class unsigned integers):

> -1
> -1

Also, how would one log strings? These DX concerns motivated why wasmRunner.mjs has so many variants of log that append the type as a suffix.

You mention that "WasmRunner has plenty of alternative logging functions (including log_i32_u() that is also just a console.log() call) but it's not obvious they exist if you don't look into this source file."

At least in the web editor, this is not the case. There is a very lengthy "How to debug" section under the "Instructions" section of each problem that describes all of the logging functions, shows examples, and provides a rationale. The markdown that populates this is in the track repo at (https://github.com/exercism/wasm/blob/main/exercises/shared/.docs/debug.md). My suggestion is that we decline this PR and we work to determine how the existing documentation broke down in your workflow / improve the documentation if anything is unclear.

Because it's so prominent in the Web editor, I assume you're using the CLI, so that's not surfaced to you?

@Terr
Copy link
Author

Terr commented Nov 12, 2023

Thanks for your extensive reply. I just intended this as a simple addition and I have no idea how to make it work for some of the types you mention, so I guess we should just close this PR.

Regarding the documentation bit: I do indeed use the Exercism CLI tool. https://exercism.org/docs/tracks/wasm/learning is the page I got my information from, I truly had no idea the web editor had its own documentation. I don't know if that's also the case on other tracks but it does seem like a waste to not link that more prominently somewhere.

@bushidocodes
Copy link
Contributor

Please know that even though we didn't merge this, your feedback is extremely helpful! I hadn't realized that there was such a gap between the web UI and the CLI tool. I suspect we may need to add something like a boilerplate comment to the *.wat source file of each exercise linking to the debug markdown file.

Have a good remainder of your weekend!

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.

None yet

3 participants