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

Variables in case expressions shadow across branches (when source names shadow) #365

Open
bergmark opened this issue Nov 12, 2013 · 7 comments
Labels

Comments

@bergmark
Copy link
Member

Thanks to @bsummer4 for reporting this.

import Prelude

data S = S Int

data M = MI Int | M

apply :: M -> S -> Int
apply op (S a) = case op of
  MI a -> a
  M -> a

main = print (apply M (S 3))
$ fay Test.hs --pretty; and node Test.js
undefined
$ runhaskell Test.hs
3

Relevant generated code:

if (Fay$$_($p2) instanceof Main._S) {
  var a = Fay$$_($p2).slot1;
  var op = $p1;
  return (function($tmp1){
    if (Fay$$_($tmp1) instanceof Main._MI) {
      var a = Fay$$_($tmp1).slot1;
      return a;
    }
    if (Fay$$_($tmp1) instanceof Main._M) {
      return a;
    }
    return (function(){ throw (["unhandled case",$tmp1]); })();
  })(op);
}

Because JS if does not introduce a new scope, a in the M branch refers to the a in the MI branch. We need to either generate a unique name for all bindings like this (but we don't have name tracking so it would become $gen1), or put all cases in a closure. Best would be if we could check if the identifier is in scope and only then rename it.

Workaround: Compile with --Wall and make sure variables aren't shadowed.

@chrisdone
Copy link
Contributor

Yeah, I guess renaming would be more performant. Probably that would be nice for reading the code in general, that shadowed variables are differentiated.

@chrisdone
Copy link
Contributor

Oh, another idea, just use closures, but then make a pass that generally transforms unneeded immediately-applied closures, as an optimization. Such as the return (function($tmp){ … })(op); one right there.

@chrisdone
Copy link
Contributor

Yeah, Closure optimizes redundant closures away:

var op = window.fib(n);
var x = (function($tmp){
  return $tmp*$tmp;
})(op);
console.log(x);

console.log(x);chris@retina:~$ java -jar /tmp/closure-compiler.jar x.js --compilation_level=ADVANCED_OPTIMIZATIONS
var b=window.a(n);console.log(b*tmp);

@bsummer4
Copy link

Since IF blocks usually contain return statements, but sometimes do not. How can we know if we should wrap the block with {return (function(){BLOCK;})();} or with {(function(){ BLOCK })();}?

@bergmark
Copy link
Member Author

We shouldn't do this translation for all if statements, just when pattern matching. Here we know that a branch will always return if the pattern (and guard, if it's present) matches.

@chrisdone
Copy link
Contributor

Yeah, ifs don't introduce bindings, so they're not affected.

@chrisdone
Copy link
Contributor

Oh, misinterpreted. Yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants