Pauan/rollup-plugin-purs

Should mutations inside the ST monad be removed by assumePureVars?

cyrbon opened this issue · 6 comments

I'm wondering whether this is an expected behavior of the assumePureVars optimization. One example is using ST monad from StrMap. Lets say we have the following code:

import Control.Monad.Eff.Console (log)
import Data.Maybe (fromMaybe)
import Data.StrMap (pureST, lookup)
import Data.StrMap.ST as M

efficientlyMutateMap = do
  s <- M.new
  _ <- M.poke s "first val" "foo"
  _ <- M.poke s "second val" "foo"
  _ <- M.poke s "third val" "foo"
  pure s

main = do
  let strMap = pureST efficientlyMutateMap
  log $ fromMaybe "Nothing!" (lookup "third val" strMap)

With assumePureVars, which is turned on by default, it will be transpiled to:

var _efficientlyMutateMap = function _do() {
  var _v13 = {};

  return _v13;
};

without assumePureVars it transpiles correctly:

var _efficientlyMutateMap = function _do() {
  var _v13 = {};

  _poke2_uncurried(_v13, "first val", "foo")();

  _poke2_uncurried(_v13, "second val", "foo")();

  _poke2_uncurried(_v13, "third val", "foo")();

  return _v13;
};

It seems to be possible to use ST monad with assumePureVars, when its used like so:

efficientlyMutateMap = do
  s <- M.new
  s2 <- M.poke s "first val" "foo"
  s3 <- M.poke s2 "second val" "foo"
  s4 <- M.poke s3 "third val" "foo"
  pure s4

Result:

var _efficientlyMutateMap = function _do() {
  var _v13 = {};
  var _v14 = _poke2_uncurried(_v13, "first val", "foo")();
  var _v15 = _poke2_uncurried(_v14, "second val", "foo")();
  var _v16 = _poke2_uncurried(_v15, "third val", "foo")();
  return _v16;
};

However, some core common libraries like purescript-maps don't use it in this way. So, for example, a singleton function from there:

-- | Create a map with one key/value pair
singleton :: forall a. String -> a -> StrMap a
singleton k v = pureST do
  s <- SM.new
  _ <- SM.poke s k v
  pure s

will transpile incorrectly to:

var _singleton$$_uncurried = function (_k5, _v13) {
  return function _do() {
    var _v14 = {};

    return _v14;
  }();
};

So, everybody using a Map / dictionary in purescript can be burned by this. Of course, in case of purescript-maps, it should be simple to modify a couple functions to use the design I've described above.

Pauan commented

That sounds like a bug, it shouldn't be removing those function calls.

I'll look into this, thanks for the report!

Pauan commented

Okay, this is really tricky. I might have to remove the assumePureVars option and just try to improve the dead code elimination algorithm instead.

Just curious if this has been addressed yet.

Pauan commented

@born2defy Sorry, I haven't had any time to work on this.

For now the solution is to set assumePureVars to false

You'll still get dead code elimination, it just won't be as good.

Pauan commented

By the way, there is talk about adding dead code elimination to PureScript. If it gets added, then dead code elimination won't be needed in rollup-plugin-purs anymore.

So because there's a good chance of dead code elimination being added to PureScript, even if I had time to work on this my motivation would be quite low.