Skip to content

Fix let (requires prelude to be merged first) (BREAKING CHANGE) #321

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

Merged
merged 15 commits into from
Aug 6, 2019

Conversation

openorclose
Copy link
Contributor

image

Problem

Program:

let a = 1;
function f() {
  return a;
}

roughly becomes:

let a = 1;
function f() {
  return a;
}
variables.a = {value: a, kind: 'let'};
variables.f = {value: f, kind: 'const'};

In the same context, if you do:

a = 2;
f();

it gets roughly transformed into

{
// put back the old values
let a = variables.a.value;
const f = variables.f.value;
{// new scope
  a = 2;
  f();
}
variables.a.value = a;
}

Problem the a referenced in f is still unchanged!

Fix

Program:

let a = 1;
function f() {
  return a;
}

roughly becomes:

let a = 1;
function f() {
  return a;
}
variables.a = {value: a, kind: 'let', assignNewValue: function (newValue) {
  return a = this.value = newValue;
}
};
variables.f = {value: f, kind: 'const'};

In the same context, if you do:

a = 2;
f();

it gets roughly transformed into

{
// put back the old values
let a = variables.a.value;
const f = variables.f.value;
{// new scope
  a = variables.a.assignNewValue(2);
  f();
}
}

so that the actual a referenced by f does indeed get updated correctly.
🎉

@coveralls
Copy link

coveralls commented Aug 4, 2019

Pull Request Test Coverage Report for Build 1525

  • 113 of 120 (94.17%) changed or added relevant lines in 8 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.06%) to 91.623%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/stdlib/stream.ts 10 12 83.33%
src/transpiler.ts 56 61 91.8%
Files with Coverage Reduction New Missed Lines %
src/interop.ts 1 94.85%
src/interpreter.ts 2 91.73%
Totals Coverage Status
Change from base Build 1517: -0.06%
Covered Lines: 1709
Relevant Lines: 1844

💛 - Coveralls

@martin-henz martin-henz self-requested a review August 4, 2019 09:54
Copy link
Member

@martin-henz martin-henz left a comment

Choose a reason for hiding this comment

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

This works now. Took a glance at the implementation. Makes sense. I trust it all works.

@openorclose openorclose changed the title Fix let (requires prelude to be merged first) Fix let (requires prelude to be merged first) (BREAKING CHANGE) Aug 6, 2019
@openorclose
Copy link
Contributor Author

Note that this includes breaking changes:

const f = () => x;

is now now longer a valid program, since x is undefined. Previously this was valid, and only threw an error when f was called.

@martin-henz
Copy link
Member

Note that this includes breaking changes:

const f = () => x;

is now now longer a valid program, since x is undefined. Previously this was valid, and only threw an error when f was called.

Will take note of this for the staff briefing next week.

@martin-henz martin-henz merged commit 492e486 into master Aug 6, 2019
@thomastanck thomastanck deleted the fix-let 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.

3 participants