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

Change variable storage to a function that returns that variable #332

Merged
merged 4 commits into from
Aug 25, 2019

Conversation

openorclose
Copy link
Contributor

Fixes #331

// first eval:
let counter = 0;
function f() {
  counter = counter + 1;
}
becomes
let counter = 0;
function f() {
  counter = counter + 1;
}
variables.set("counter", getValue: ()=>counter, assignNewValue: newValue => counter = newValue);
// second eval (wrong):
f(); counter;
becomes
let counter = variables.get("counter").getValue(); // set back counter;
{
  f(); // this has a side effect of incrementing counter
  return counter; // that does not get propagated here
}

Let's change ALL identifiers that refer to previous scopes also do a retrieval again to make sure the value is the most updated one.

// second eval (fixed):
f(); counter;
//becomes
let counter = variables.get("counter").getValue(); // set back counter;
{
  f(); // this has a side effect of incrementing counter
  return (counter = variables.get("counter").getValue()); // we have no choice but to
   // always assume that counter is updated through a side effect and always retrieve its
   // real value
}

@coveralls
Copy link

coveralls commented Aug 14, 2019

Pull Request Test Coverage Report for Build 1630

  • 19 of 19 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 87.797%

Totals Coverage Status
Change from base Build 1611: 0.03%
Covered Lines: 2127
Relevant Lines: 2355

💛 - Coveralls

Copy link

@blackening blackening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my first time reviewing js-slang native, thus i'll explain a little more.

We assume that the program given is indeed a valid program, as guaranteed by the static analyzer.
We note that the program can run in either

  • A new context.
  • An existing context.

Furthermore, we have a "unique" environment model.

  • Each REPL call may declare their own variables.
  • Such variables override existing ones... for subsequent calls only.
    • It is required that functions defined in preceding calls function normally.

E.g.

--------------
let bar = () => 'bar';
function foo() {
   return bar();
}
foo(); // returns 'bar' 
--------------
function bar() {
   return 'barred';
}
bar(); // returns 'barred'
--------------
foo(); // returns bar

From what i can tell (and was explained to me), the problem lies in how the transpiler previously translated statements.

Via static analysis, the transpiler first determines which variables are:

  • Declared
  • Used but not declared (Presumed to be global)

If a variable is declared, then it is transpiled to a deferred load/store.
If a variable is used, it is transpiled to a load.
If a variable is written to, it is transpiled to a assignNewValue call.

Storage is done like this previously:

storage.varname = {
   kind: 'const' | 'let'
   value: varname,
   assignNewValue(paramName) {
         return varname = this.value = paramName;
   }
}

The issue seems to be that this allows storage.varname.value to desync from the "closured" version of varname.

The solution is then to remove storage.varname.value, implement a storage.varname.getValue() and allow the closured version to be a single source of truth.


Frankly, my only comment would be to rename previousVariablesToTimesGoneUp since what it stored has changed.

@openorclose
Copy link
Contributor Author

Frankly, my only comment would be to rename previousVariablesToTimesGoneUp since what it stored has changed.

Yup, I'll change this! Thanks for the review

@openorclose openorclose merged commit e1b584e into master Aug 25, 2019
@thomastanck thomastanck deleted the fix-let-again branch August 1, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug with let in NATIVE
3 participants