Desugar pattern guards *after* type checking

[alexbiehl]

@paf31

This is a fix for #2795

It's not working properly currently:

purs.exe: An internal error occurred during compilation: "Binders should have been desugared"
Please report this at https://github.com/purescript/purescript/issues
CallStack (from HasCallStack):
  error, called at src\Language\PureScript\Crash.hs:24:3 in purescript-0.11.1-inplace:Language.PureScript.Crash
  internalError, called at src\Language\PureScript\Sugar\BindingGroups.hs:112:33 in purescript-0.11.1-inplace:Language.PureScript.Sugar.BindingGroups
purs.exe: thread blocked indefinitely in an MVar operation

I am using everywhereOnValuesM traversal to traverse the list of Declarations. It seems to miss the expressions in there. Please take a look at desugarCaseGuards for a hint. Any pointer on how to fix this?

[alexbiehl]

(Ah I discovered binding groups are a part of Purescript.Sugar, so this explains it)

[alexbiehl]

I have discovered another issue. For a test I wrote:

f x
  | 1 <- x = x
  | true = 1234

This gets correctly translated to:

f x = let 
          v _ = 1234
        in case x of 
               1 -> x
               _ -> v true

Now the codegen, still correct:

var g = function (x) {
    var v = function(x0) { return 1234; }

    if (x === 1) {
        return x;
    };
    return v(true);
};

It rightfully spots the very small function v (btw. this seems to be new, when I wrote the pattern guards patch it didn't do such things) and inlines it:

var g = function (x) {
    if (x === 1) {
        return x;
    };
    return 1234(true);
};

It still applies true to 1234 which is of course not correct!

[paf31]

@alexbiehl are you able to verify that the issue is in the optimizer? You could turn the optimizer off and see if the generated code is correct.

[alexbiehl]

Using

-- | Apply a series of optimizer passes to simplified JavaScript code
optimize :: MonadSupply m => AST -> m AST
optimize js = pure js

I get

"use strict";
var f = function (x) {
    return (function () {
        var v = (function () {
            var $1 = x;
            var x1 = $1;
            if (true) {
                return 1234;
            };
            throw new Error("Failed pattern match at X line 3, column 1 - line 5, column 12: " + [ $1.constructor.name ]);
        })();
        return (function () {
            var $2 = x;
            var x1 = $2;
            return (function () {
                var $3 = x1;
                if ($3 === 1) {
                    return x1;
                };
                return v(true);
                throw new Error("Failed pattern match at X line 3, column 1 - line 5, column 12: " + [ $3.constructor.name ]);
            })();
            throw new Error("Failed pattern match at X line 3, column 1 - line 5, column 12: " + [ $2.constructor.name ]);
        })();
    })();
};

This looks correct. Variablev is only bound once. This makes it susceptible for inlining and then maybe etaConvert does the rest?

[alexbiehl]

Ah! No, we can see v is being invoked immediately. It doesn't reduce to a function, it reduces to 1234 which of course is replaced by vs occurrence. Somewhere a bug is lurking...

[paf31]

Is it possible to run your pass between the exhaustivity and type checking passes? Would that help?

[alexbiehl]

This was the culprit. After desugaring we need to generate code which obeys the invariants of desugared code..

[alexbiehl]

@paf31 Please take a look; I think this is ready for review now. Test suite succeeds locally.

[paf31]

Looks great, thanks very much!