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

Enforce strict field initialization #1349

Merged
merged 10 commits into from Jun 30, 2020
Merged

Enforce strict field initialization #1349

merged 10 commits into from Jun 30, 2020

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Jun 22, 2020

As a follow-up to #1155, this PR implements strict field initialization and makes it the default, so we always get the safety guarantees we need in AOT. I expect this to be somewhat controversial because it's stricter than TS in potentially unsafe scenarios.

To give a typical example:

class Foo {
  bar: Bar;
  constructor() {
    this.bar = new Bar(this);
    // illegal because ^ `Bar#constructor` can do anything with
    // the partially initialized `this` before `.bar` is assigned
  }
}
class Bar {
  constructor(public foo: Foo) {
  }
}

must instead be written like

class Foo {
  private _bar: Bar | null; // initialized with zero
  constructor() {
    this._bar = new Bar(this);
  }
  get bar(): Bar {
    return assert(this._bar);
  }
}
...

or using definite assignment syntax to automatically inject the assert:

class Foo {
  bar!: Bar;
  constructor() {
    this.bar = new Bar(this);
    // ^ valid because `.bar` is ignored and checked at runtime
  }
}
...

essentially enforcing an actual safety check for the case where Foo#bar is being accessed too early in Bar#constructor.

What this PR doesn't do compared to TS, however, is to enforce strict initialization of fields that can be trivially initialized with zero (which is our undefined), like of type i32, f64 or a nullable type like string | null, so one can leave these uninitialized instead of adding boilerplate assignments, similar to locals or globals.

For more examples of what's allowed and what's not, see:

  • I've read the contributing guidelines
  • Fix object literals

fixes #1155

@dcodeIO dcodeIO requested a review from MaxGraey June 30, 2020 14:26
src/compiler.ts Outdated Show resolved Hide resolved
src/flow.ts Outdated Show resolved Hide resolved
Copy link
Member

@MaxGraey MaxGraey left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good for me

dcodeIO and others added 2 commits June 30, 2020 19:02
Co-authored-by: Max Graey <maxgraey@gmail.com>
Co-authored-by: Max Graey <maxgraey@gmail.com>
src/compiler.ts Outdated Show resolved Hide resolved
@dcodeIO dcodeIO merged commit 57a55d3 into master Jun 30, 2020
@github-actions
Copy link

github-actions bot commented Jul 1, 2020

🎉 This PR is included in version 0.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jedisct1
Copy link

jedisct1 commented Jul 1, 2020

Looks like if a constructor has the @inline attribute, fields it initializes are considered as not being initialized.

Is that intentional?

@dcodeIO
Copy link
Member Author

dcodeIO commented Jul 1, 2020

Sounds like a bug to me. Need to check.

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

Successfully merging this pull request may close these issues.

None yet

3 participants