Kilian Vounckx (Aug 20 2022 at 15:19): Kilian Vounckx (Aug 20 2022 at 15:19):Hi all,
Newcomer to Roc here (also to zulip, so sorry if this is the wrong place or stream or whatever to ask)
I am trying to translate this md5 implementation to roc, just to try it out. It relies heavily on bitwise operators. When trying these out, I discovered that these operators are implemented in an unintuitive way. For example: Num.shiftLeftBy x y corresponds to python's (or c's, or any other language) y << x. This argument swap is the same for all operators.
Is this intended, or is this a bug somewhere? There is a chance I am just plain wrong since I don't often work with these operators. However I don't think so.
It would be nice if someone took a look at this!
Folkert de Vries (Aug 20 2022 at 15:31):I'm guessing the idea is to use the |> operator so we x |> Num.shiftLeftBy y
Folkert de Vries (Aug 20 2022 at 15:31):which reads intuitively
Ayaz Hafiz (Aug 20 2022 at 15:36):I think that's a bit off because right now you would have 4 |> Num.shiftLeftBy 1 be 1 << 4. Seems like it should be 1 |> Num.shiftLeftBy 4
Kilian Vounckx (Aug 20 2022 at 15:36):Even then it is weird to me. I read x |> Num.shiftLeftBy y as "shift x to the left by y places", but maybe that's just me
Kilian Vounckx (Aug 20 2022 at 15:37):Oh Ayaz was there with my point
Kilian Vounckx (Aug 20 2022 at 15:37):So not just me :sweat_smile:
Folkert de Vries (Aug 20 2022 at 15:39):I read x |> Num.shiftLeftBy y as "shift x to the left by y places",
that is what it _should_ be, is that not what happens?
Kilian Vounckx (Aug 20 2022 at 15:39):No, it is the other way around
Folkert de Vries (Aug 20 2022 at 15:39):in roc, the |> operator desugas as x |> f y => f x y
Folkert de Vries (Aug 20 2022 at 15:39):so this desugars to Num.shiftLeftBy x y in this case
Folkert de Vries (Aug 20 2022 at 15:40):which corresponds to y << x right?
Folkert de Vries (Aug 20 2022 at 15:41):hmm
Kilian Vounckx (Aug 20 2022 at 15:41):Right
Folkert de Vries (Aug 20 2022 at 15:41):nope that reasoning is flawed somewhere
Folkert de Vries (Aug 20 2022 at 15:42):Num.shiftLeftBy 2 0b0000_0011 today evaluates to 0b0000_1100, (resp 2 |> Num.shiftLeftBy 0b11) so that is equivalent to ob11 << 2
Folkert de Vries (Aug 20 2022 at 15:43):and yeah that is not the order that you actually want, probably
Kilian Vounckx (Aug 20 2022 at 15:43):Intuitively to me both Num.shiftLeftBy x y and x |> Num.shiftLeftBy y should correspond to x << y
Folkert de Vries (Aug 20 2022 at 15:43):@Richard Feldman I think we discussed this at some point?
Richard Feldman (Aug 20 2022 at 15:47):Kilian Vounckx said:
Intuitively to me both
Num.shiftLeftBy x yandx |> Num.shiftLeftBy yshould correspond tox << y
yes, this is how it ought to work - if that's not how it works currently, that's a bug!
Richard Feldman (Aug 20 2022 at 15:48):my guess is that the original implementation followed Elm's implementation, and didn't switch the arguments
Richard Feldman (Aug 20 2022 at 15:48):but the arguments need to be switched in Roc becuase |> is flipped
Kilian Vounckx (Aug 20 2022 at 15:49):I think it is wrong, but probably someone should check that
Kilian Vounckx (Aug 20 2022 at 15:50):I have never contributed to Roc (or any open source project), but would like to. Would this be a good first issue to work on?
Richard Feldman (Aug 20 2022 at 15:51):sure!
Richard Feldman (Aug 20 2022 at 15:51):(I just checked - yeah, the arguments are currently backwards, and should be flipped!)
Kilian Vounckx (Aug 20 2022 at 15:54):Cool. I'll read the CONTRIBUTING.md file on github and ask any questions here if (probably when) I have them
Richard Feldman (Aug 20 2022 at 15:54):awesome, thank you so much! :smiley:
Richard Feldman (Aug 20 2022 at 15:54):there's a #contributing channel dedicated to that topic
Kilian Vounckx (Aug 20 2022 at 15:55):No problem!
I will look there as well
Kilian Vounckx (Aug 20 2022 at 16:14):Before changing any code, I built from source with Nix and tried to run cargo test. It failed however with the message `failed to run custom build command for 'wasm3-sys v0.5.0 ...'. Is this normal? Or should I do something else before running the tests?
Folkert de Vries (Aug 20 2022 at 16:15):hmm, nix should install bindgen right?
Folkert de Vries (Aug 20 2022 at 16:15):or, if not, you can try cargo install bindgen maybe?
Kilian Vounckx (Aug 20 2022 at 16:19):I just tried it after cargo install bindgen
Same error
Folkert de Vries (Aug 20 2022 at 16:19):is bindgen available in the shell now?
Kilian Vounckx (Aug 20 2022 at 16:20):Yes
Folkert de Vries (Aug 20 2022 at 16:20):ok, what does the error say exactly?
Kilian Vounckx (Aug 20 2022 at 16:21):error: failed to run custom build command for `wasm3-sys v0.5.0 (https://github.com/roc-lang/wasm3-rs?rev=f0f807d1fc0a50d1d68e5799e54ee62c05af00f5#f0f807d1)`
Caused by:
process didn't exit successfully: `/home/kilianv/.roc/target/debug/build/wasm3-sys-8d9c9821d14427cb/build-script-build` (exit status: 1)
--- stderr
/home/kilianv/.roc/target/debug/build/wasm3-sys-8d9c9821d14427cb/build-script-build: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by /nix/store/j132k1ncfn6gjfd2f5s1gz37170rch4h-gcc-11.3.0-lib/lib/libgcc_s.so.1)
warning: build failed, waiting for other jobs to finish...
hmm, we saw that earlier when trying to use bindgen as a library
Folkert de Vries (Aug 20 2022 at 16:24):but not when it is used from the command line
Kilian Vounckx (Aug 20 2022 at 16:25):This is the error I get when running cargo test. If I run bindgen in the command line, it is just an error saying I used the bindgen command wrong
Kilian Vounckx (Aug 20 2022 at 16:26):error: The following required arguments were not provided:
<header>
USAGE:
bindgen [FLAGS] [OPTIONS] <header> -- <clang-args>...
For more information try --help
but that just means bindgen is installed correctly no?
Folkert de Vries (Aug 20 2022 at 16:26):yes, what does bindgen --version say?
Kilian Vounckx (Aug 20 2022 at 16:26):0.59.2
Folkert de Vries (Aug 20 2022 at 16:27):that's fine
Folkert de Vries (Aug 20 2022 at 16:27):could you try cloning https://github.com/roc-lang/wasm3-rs and running cargo build there?
Kilian Vounckx (Aug 20 2022 at 16:28):Does it matter where I clone it relative to my Roc clone?
Folkert de Vries (Aug 20 2022 at 16:29):no
Kilian Vounckx (Aug 20 2022 at 16:30):Lots of warnings about compiling with optimization, but no errors
Folkert de Vries (Aug 20 2022 at 16:31):this is still using the roc compiler's nix environment?
Kilian Vounckx (Aug 20 2022 at 16:31):yes
Kilian Vounckx (Aug 20 2022 at 16:31):should I exit that?
Folkert de Vries (Aug 20 2022 at 16:31):no, no keep that
Kilian Vounckx (Aug 20 2022 at 16:31):okay
Folkert de Vries (Aug 20 2022 at 16:32):so, you just compiled the thing that fails to compile when compiling the roc compiler tests
Folkert de Vries (Aug 20 2022 at 16:32):and I'm really not sure what would cause that ...
Folkert de Vries (Aug 20 2022 at 16:33):something to try: in crates/compiler/test_gen/Cargo.toml, change the wasm3 dependency to point to your local clone, so that becomes wasm3 = { path = "/some/path/to/wasm3-rs" }
Folkert de Vries (Aug 20 2022 at 16:33):and see what that does
Kilian Vounckx (Aug 20 2022 at 16:39):now there is another error:
error: failed to run custom build command for `inkwell v0.1.0 (https://github.com/roc-lang/inkwell?branch=master#accd4068)`
Caused by:
process didn't exit successfully: `/home/kilianv/.roc/target/debug/build/inkwell-a557c6ea40c637ca/build-script-build` (exit status: 1)
--- stderr
/home/kilianv/.roc/target/debug/build/inkwell-a557c6ea40c637ca/build-script-build: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by /nix/store/j132k1ncfn6gjfd2f5s1gz37170rch4h-gcc-11.3.0-lib/lib/libgcc_s.so.1)
Apart from that, a lot of the same warnings as when building wasm3-rs
Folkert de Vries (Aug 20 2022 at 16:42):is this maybe relevant https://www.reddit.com/r/NixOS/comments/tzvrjm/help_with_glibc_234_not_found/?
Kilian Vounckx (Aug 20 2022 at 16:45):I don't get the question in that subreddit. When I run cargo inside the nix environment, it works fine
Kilian Vounckx (Aug 20 2022 at 16:48):I'm really sorry. Since I'm a student living with my parents during summer, I can't decide when to eat. So I have to go get dinner. I will try to get back asap
Brendan Hansknecht (Aug 20 2022 at 16:48):A general note, sometimes cleaning and rebuilding fixes nix glibc errors for me.
Kilian Vounckx (Aug 20 2022 at 17:05):I'm back :smile:
what do you mean by that? Rebuilding the nix environment or something else?
Brendan Hansknecht (Aug 20 2022 at 17:11):No, just cargo clean and then cargo build
Brendan Hansknecht (Aug 20 2022 at 17:11):While in the nix environment
Kilian Vounckx (Aug 20 2022 at 17:16):Oh trying that,
I am not really familiar enough yet with cargo (except build, run and test)
Kilian Vounckx (Aug 20 2022 at 17:32):alright! tests are running!
Kilian Vounckx (Aug 20 2022 at 17:32):Thanks @Brendan Hansknecht and @Folkert de Vries for the help!
Folkert de Vries (Aug 20 2022 at 17:33):nice!
Folkert de Vries (Aug 20 2022 at 17:34):then, to fix the original problem: try looking at occurences of
NumShiftLeftBy,
NumShiftRightBy,
NumShiftRightZfBy,
Yeah, I found the problem in the source already. However there is a note in the comments explicitly stating that the arguments are swapped. So it seems deliberate
Folkert de Vries (Aug 20 2022 at 17:38):well yes but we've now established the order that we want
Folkert de Vries (Aug 20 2022 at 17:38):so, try and make the output line up with that
Kilian Vounckx (Aug 20 2022 at 17:38):That means I have to only change 3 lines (swap 2 variables on each line, for each function), and it should work if I understand correctly. However I think I should also add documentation to those functions to tell other users what the order is with some examples
Folkert de Vries (Aug 20 2022 at 17:38):and, to check that, try cargo test-gen-llvm gen_num::shift_left_by
Folkert de Vries (Aug 20 2022 at 17:39):and test-gen-dev and test-gen-wasm for the other backends
Kilian Vounckx (Aug 20 2022 at 17:39):right now, I am running all tests which takes a while :sweat_smile:
so when that is done I will make the change on a different branch and only test the changes
Kilian Vounckx (Aug 20 2022 at 17:40):I'm a first time open source contributor. Does it matter if I fork or clone the repo when making a pull request with the change?
Folkert de Vries (Aug 20 2022 at 17:40):yes, you will need a fork now
Kilian Vounckx (Aug 20 2022 at 17:41):okay, and then I just make a new branch and make the necessary changes?
Folkert de Vries (Aug 20 2022 at 17:43):yup
Kilian Vounckx (Aug 20 2022 at 17:47):Great, when that is done, I will probably have some questions about how to do the pull requests and signing commits since I have never done this. For now I know what to do, thanks a lot!
Brian Carroll (Aug 20 2022 at 19:18):Commit signing will probably be easier if you set it up before making the commits themselves.
https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits
Kilian Vounckx (Aug 20 2022 at 19:25):ah well You were literally two minutes too late :pensive:
Is there a way to still sign even after making the commits? I haven't made the pull request yet
Folkert de Vries (Aug 20 2022 at 19:27):would git commit --amend work?
Folkert de Vries (Aug 20 2022 at 19:27):after you set up the signing
Kilian Vounckx (Aug 20 2022 at 19:28):I can't seem to make a gpg key in the nix environment. For just the key I don't have to be in it right?
Folkert de Vries (Aug 20 2022 at 19:29):not sure but I'd guess git is not specific to the nix environment?
Kilian Vounckx (Aug 20 2022 at 19:29):my guess as well
Richard Feldman (Aug 20 2022 at 19:32):also btw you can now write test for this directly in Num.roc using the expect keyword - this is very new, so we don't have many such tests yet
Richard Feldman (Aug 20 2022 at 19:32):Str.roc has a couple of examples of this
Kilian Vounckx (Aug 20 2022 at 19:33):Oh I will do a couple of those as well then
Can you also do this in doc comments, like in rust? Or is it something planned? Or not at all?
Richard Feldman (Aug 20 2022 at 19:40):just the expect keyword - I think there's value in doing some checking on examples in documentation (so you can get a notification if they break), but I want to stop short of doctests because I don't think it's ideal to create an incentive for example in docs to be both good for teaching and also good tests - I think it's better to make an incentive to optimize examples in docs for teaching, and to optimize tests outside docs for being good tests
Kilian Vounckx (Aug 20 2022 at 19:40):Folkert de Vries said:
would
git commit --amendwork?
the amend option worked
Kilian Vounckx (Aug 20 2022 at 19:42):Richard Feldman said:
just the
expectkeyword - I think there's value in doing some checking on examples in documentation (so you can get a notification if they break), but I want to stop short of doctests because I don't think it's ideal to create an incentive for example in docs to be both good for teaching and also good tests - I think it's better to make an incentive to optimize examples in docs for teaching, and to optimize tests outside docs for being good tests
I understand. I was just wondering. Now I have to be extra careful to make my doc examples correct :sweat_smile:
Kilian Vounckx (Aug 20 2022 at 19:44):Is there a way to filter the expect tests with cargo? Or do I just run them in roc?
Folkert de Vries (Aug 20 2022 at 19:45):target/debug/roc test crates/compiler/builtins/roc/Str.roc runs expects in Str.roc
Kilian Vounckx (Aug 20 2022 at 20:00):when I run the test in roc I get the following error:
thread 'main' panicked at 'way too many specialization passes!', crates/compiler/load_internal/src/file.rs:867:26
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
hmm, maybe just skip those tests for now?
Kilian Vounckx (Aug 20 2022 at 20:01):Yeah I was planning that too. Maybe I'll get back to it later, as well as some other tests
Richard Feldman (Aug 20 2022 at 20:02):cc @Ayaz Hafiz - you mentioned something to me last week about that error
Kilian Vounckx (Aug 20 2022 at 20:06):Alright, I made the pull request!
Thanks for the help everyone!
I now have a better understanding of the process, so next pull requests should go way faster :smiling_face:
Ayaz Hafiz (Aug 20 2022 at 20:07):Kilian Vounckx said:
when I run the test in roc I get the following error:
thread 'main' panicked at 'way too many specialization passes!', crates/compiler/load_internal/src/file.rs:867:26 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Do you have a commit I could reproduce this on?
Ayaz Hafiz (Aug 20 2022 at 20:08):as the error suggests that should never really happen haha
Kilian Vounckx (Aug 20 2022 at 20:16):The exact commit is one in my fork: 6d776c6624a84ae0fa710c85df25732a35102a03
The commit I forked from is: 334bc7174fb91c8ca00d9612ba92dc653bce0aa3
Kilian Vounckx (Aug 20 2022 at 20:22):woops, I see the language barrier has made some spelling mistakes creep in. @Ayaz Hafiz Will you fix them, or do I do it. If so, how do I do it?
Ayaz Hafiz (Aug 20 2022 at 20:26):You can push new commits with the needed changes and then mark the suggested changes as "resolved"
Last updated: Jul 06 2025 at 12:14 UTC