Unshadowing (with avoid list) confusion in JavaScript code-generation

[jeromesimeon]

There seem to be a bit of confusion in the way the avoid list collides with the variable or constant names. During code generation, variables get prefixed with v. If you have a global constant with name e.g., this, you end up with inconsistent JavaScript code.

bash-3.2$ cat oql/this.oql
this
bash-3.2$ ../bin/qcert -source oql -target javascript oql/this.oql 
Compiling from oql to js:
  oql -> nraenv -> nraenv -> nnrc -> nnrc -> js
bash-3.2$ cat oql/this.js 
function query(constants) {
  var vthis_0 = deref(constants, "this");
  return vthis;
}

[shinnar]

This problem is not restricted to things like this -- it appears that every time we suffix with a number we do so only on the declaration, not on the use!
For example:

/* Returns a record (struct in OQL terminology) */
select this
from c in Marketers,
     d in c->clients,
     e in Marketers

This happens to Marketers as well!
This shows up more obviously with this, since then we always suffix (which is probably also a bug, albeit one that has no ill-effects except on code readability).

[shinnar]

Actually, looking into this more resulted in noticing #82 as well

[shinnar]

to be clear, we are talking about free variables here. I think the fix here is to

• avoid emitting duplicate let bindings for the same free variable.
• stop worrying about avoidance (this is related to Javascript backend does not avoid keywords correctly #82, and a solution to it)

[shinnar]

4ae6a5f addresses the duplication problem.

[shinnar]

97e51e4 fixes this particular example (almost as a side-effect), since the variable is first rewritten to "c$" as part of the let (which is a bit hacky, incidentally). However, the output is a bit nicer in that it distinguishes the variables that refer to constants from other ones.

However, it does not really fix the problem, so I am leaving this issue open.
The basic problem is that unshadow does not rewrite NNRCGetConstants, since they are supposed to be free. However, we call unshadow on something that in fact binds these variables with NNRCLets. So the unshadow renames the let variables, but leaves the NNRCGetConstants alone.

A proper fix probably involves one of these options:

• Fixing the unshadow call
  • Either not calling unshadow on that Let expression (call unshadow on the body instead),
    and deal with potential renaming separately. Note that simply prepending "vc$" is not sufficient, since the name of the string could have invalid javascript characters that need to be mangled, which itself could cause two variables to conflate. We could either substitute in the GetConstants explicitly/seperately, or (better), pass in a map of substitutions, and use it when translating the getconstants.
• Providing a modified (paramaterized?) version of unshadow which renames the constants.
  This is problematic, since we also need to rename the binding for them.
  Of course, if we close over the lets first, then this could happen for free, but it makes the correctness
  property of unshadow somewhat subtle.
• when we "close" over the lets, we can also change the getconstants into vars. Then the unshadow would work normally. We would need to be careful to avoid shadows when doing this, of course. Note that this even if we don't use it, this substitution function might be useful as part of the correctness specification for a modified unshadow as per the previous option.

I lean towards the first option, as it leaves unshadow alone, and acknowledges that the NNRCLet bindings being added are really a convenience to reuse some of the translator, rather then being actually semantically equivalent NNRC.

[shinnar]

also, see #82

[jeromesimeon]

I agree with the analysis (nice btw!), and I think I like the first option as well.