Add PureScript implementation

[mrsekut]

This is a PureScript implementaion.
Passed all tests.
Dockerfile etc. are not ready yet.

[mrsekut]

Is there anything else I need to do?

[kanaka]

Overall, the code itself looks good. One issue I noticed is that step2 uses Env which is part of step3. Can you adjust step2 to not use the Env module?

The build doesn't work out of the box. The Makefile doesn't quite work and is missing some dependencies on source files (to trigger rebuild when sources change) and on the npm dependency. Also, the Makefile needs a clean target.

I suggest the following changes to the Dockerfile and Makefile:

diff --git a/impls/purs/Dockerfile b/impls/purs/Dockerfile
index 935e655b..73f46925 100644
--- a/impls/purs/Dockerfile
+++ b/impls/purs/Dockerfile
@@ -30,8 +30,9 @@ RUN curl -sL https://deb.nodesource.com/setup_12.x | bash -
 # Install nodejs
 RUN apt-get -y install nodejs
 
-# Install purescript
-RUN apt-get install -y libtinfo5
+# Install purescript and deps
+RUN apt-get install -y git libtinfo5
 RUN npm install -g --unsafe-perm purescript spago
 
-ENV NPM_CONFIG_CACHE /mal/.npm
\ No newline at end of file
+ENV NPM_CONFIG_CACHE /mal/.npm
+ENV HOME /mal
diff --git a/impls/purs/Makefile b/impls/purs/Makefile
index b97196b7..2488a981 100644
--- a/impls/purs/Makefile
+++ b/impls/purs/Makefile
@@ -1,6 +1,8 @@
-src/step%:
-       spago bundle-app --main ${${@F}} --to $(@F:%.purs=%.js)
+step%.js: src/step%.purs $(OTHER_SRCS) node_modules/readline-sync
+       spago bundle-app --main $($(<:src/%=%)) --to $@
 
+node_modules/readline-sync:
+       npm install
 
 #####################
 
@@ -14,4 +16,10 @@ step6_file.purs = Mal.Step6
 step7_quote.purs = Mal.Step7
 step8_macros.purs = Mal.Step8
 step9_try.purs = Mal.Step9
-stepA_mal.purs = Mal.StepA
\ No newline at end of file
+stepA_mal.purs = Mal.StepA
+
+OTHER_SRCS = src/Readline.js src/Readline.purs src/Types.purs src/Reader.purs \
+            src/Printer.purs src/Env.purs src/Core.purs
+
+clean:
+       rm -rf step*.js output/*

You can test that everything works for the build and CI like this:

make "docker-build^purs"
make DOCKERIZE=1 "clean^purs"
make DOCKERIZE=1 "test^purs"

[kanaka]

A couple other things:

• With the Dockerfile and Makefile changes, the tests pass locally for me. So things are close.
• I've pushed a docker image based on the Dockerfile changes I suggested. That will enable your implementation to be tested in CI.
• There is an extraneous file ".psc-ide-port" at the top-level. Please remove (and maybe ignore it in .gitignore).
• You'll need to add the implementation to the README.md and update the counts at the top.

With those changes (and the Env change I already mentioned), and assuming CI passes, it should be ready to merge.

[mrsekut]

Fixed README, Dockerfile, Makefile and step2.

[kanaka]

There was a minor conflict with upstream in the README.md for the counts update. I've fixed that. I forgot one more thing that needs to be done which is to add purs to the IMPLS.yml so that it gets tested by CI (Actions).

[mrsekut]

Added to IMPLS.yml.
Performance testing seems to be taking a long time ..

[kanaka]

The performance test is failing because the (time-ms) function appears to be returning the same value every time it is called rather than the current milliseconds time value.

I also noticed a couple of other issues while tracking that down:

• The REPL doesn't seem to handle exiting on Ctrl-D. Rather it gives an EOF error and then continues.
• The makefile doesn't have an all target. Having is really helpful for testing and CI efficiency (no need to create the docker container for each step to build). Here is a suggested change that addresses that:

diff --git a/impls/purs/Makefile b/impls/purs/Makefile
index 6e44cd4c..18e9991d 100644
--- a/impls/purs/Makefile
+++ b/impls/purs/Makefile
@@ -1,4 +1,10 @@
-step%.js: src/step%.purs $(OTHER_SRCS) node_modules/readline-sync
+BINS = step0_repl.js step1_read_print.js step2_eval.js step3_env.js \
+       step4_if_fn_do.js step5_tco.js step6_file.js step7_quote.js \
+       step8_macros.js step9_try.js stepA_mal.js
+
+all: $(BINS)
+
+$(BINS): %.js: src/%.purs $(OTHER_SRCS) node_modules/readline-sync
        spago bundle-app --main $($(<:src/%=%)) --to $@
 
 

[mrsekut]

Fixed makefile, time-ms and ctrl-d

[kanaka]

I'm now getting the following error when I try and run the performance test:

~/impls/purs$ ../purs/run ../tests/perf1.mal
/mal/impls/purs/stepA_mal.js:410
      throw e;
      ^

Error: invalid operator
    at Object.exports.error (/mal/impls/purs/stepA_mal.js:401:12)
    at Object.$$throw [as throw] (/mal/impls/purs/stepA_mal.js:437:47)
    at Object.fn (/mal/impls/purs/stepA_mal.js:21302:43)
    at Bound.value1 (/mal/impls/purs/stepA_mal.js:22038:173)
    at Bound.value0 (/mal/impls/purs/stepA_mal.js:1068:49)
    at /mal/impls/purs/stepA_mal.js:1116:35
    at go (/mal/impls/purs/stepA_mal.js:1138:21)
    at __do (/mal/impls/purs/stepA_mal.js:859:39)
    at /mal/impls/purs/stepA_mal.js:867:25
    at __do (/mal/impls/purs/stepA_mal.js:871:21)

[mrsekut]

sorry, there was an omission of confirmation.
Fixed an error to make the type name easier to understand.

[kanaka]

The perf tests still don't work. The (time-ms) function now returns different values if I run it twice in a row, but the time values aren't numbers (or they aren't being coerced everywhere they need to be) and so mathematical operations don't work correctly.

user> (def! t1 (time-ms))
1639585323644.0
user> (def! t2 (time-ms))
1639585328289.0
user> (- t2 t1)
0
user> (number? t2)
false
user> (number? 0)
true
user> (+ t2 t1)
-2

Also, I found another bug in the Makefile. The order of variable definitions is significant in Makefiles and so things weren't rebuilding correctly when dependent sources changes. Here is a suggested patch (to move the OTHER_SRCS variable earlier):

diff --git a/impls/purs/Makefile b/impls/purs/Makefile
index 59dbe0f3..4dd0582b 100644
--- a/impls/purs/Makefile
+++ b/impls/purs/Makefile
@@ -2,6 +2,9 @@ BINS = step0_repl.js step1_read_print.js step2_eval.js step3_env.js \
        step4_if_fn_do.js step5_tco.js step6_file.js step7_quote.js \
        step8_macros.js step9_try.js stepA_mal.js
 
+OTHER_SRCS = src/Readline.js src/Readline.purs src/Types.purs src/Reader.purs \
+             src/Printer.purs src/Env.purs src/Core.purs
+
 all: $(BINS)
 
 $(BINS): %.js: src/%.purs $(OTHER_SRCS) node_modules/readline-sync
@@ -26,9 +29,6 @@ step8_macros.purs = Mal.Step8
 step9_try.purs = Mal.Step9
 stepA_mal.purs = Mal.StepA
 
-OTHER_SRCS = src/Readline.js src/Readline.purs src/Types.purs src/Reader.purs \
-             src/Printer.purs src/Env.purs src/Core.purs
-
 
 clean:
        rm -rf step*.js output/*
\ No newline at end of file

[mrsekut]

Thank you for the repeated reviews.

Fixed Makefile and time related functions.
Now the tests for perf1, perf2 and perf3 have been completed successfully.

[kanaka]

Okay, the perf tests works now. A couple more things before merging:

• The comparison operators (<, >=, etc) don't work with time values either. There is actually a stepA test that reports this although it's optional (although it is mandatory for merging to upstream). I would recommend converting the time values to regular numeric types. You don't have to keep the most significant part of the epoch milliseconds value. For example, you could subtract the epoch millisecond value of Jan 1, 2000 if you wanted before returning the value as an integer.
• Some earlier merged PRs have caused conflicts with Makefile.impls and README.md. Can you merge those? The counts will need to be re-updated again too.

[mrsekut]

The comparison function also supports MalTime and MalInt operations, eliminating conflicts.

[kanaka]

It's still failing: https://github.com/kanaka/mal/runs/4549501254?check_suite_focus=true#step:4:1342

I would recommend converting the value of time-ms to a regular number/integer rather than coercing for every math operation. If more numerical operations are added in the future, there is less to change if time-ms returns a native number rather than having to add coercion to every numeric function.

[mrsekut]

Fixed the comparison function.

MalTime Type is an alias for Number Type.
Int Type is used in other parts.
When using Int, the epoch millisecond value does not fit. Therefore, we use Number for time.

I thought about unifying all the numeric types in the implementation to Number Type, but I continue to use Int Type because it makes it difficult to understand the intention.

Therefore, it is necessary to perform calculations that combine Number Type and Int Type in numerical operations and comparison functions. The implementation of this part was wrong and it failed the test.

[kanaka]

Looks good! I had to do a little manual rebasing and cleanup, but I've merged it. Congrats! Do you have a twitter account? If so, do you want to tweet something about this implementation (and I'll retweet it)? Otherwise, I'll just tweet it directly to announce it.

BTW, I notice during the build a bunch of non-fatal warnings that looked like this:

...
Warning 18 of 18:

  in module Data.CatList
  at .spago/catenable-lists/v6.0.1/src/Data/CatList.purs:210:1 - 210:47 (line 210, column 1 - line 210, column 47)

    A custom warning occurred while solving type class constraints:

      'MonadZero' is deprecated, use 'Monad' and 'Alternative' constraints instead


  in value declaration monadZeroCatList

  See https://github.com/purescript/documentation/blob/master/errors/UserDefinedWarning.md for more information,
  or to contribute content related to this warning.

[mrsekut]

Thank you for your kind review.
My Twitter account is @mrsekut. I tweeted earlier.

I will check the warning later, and if I can fix it, I will make another PR.