Skip to content

🏗🚮 Inline private const class properties#24027

Closed
jridgewell wants to merge 2 commits intoampproject:masterfrom
jridgewell:inline-const-class-props
Closed

🏗🚮 Inline private const class properties#24027
jridgewell wants to merge 2 commits intoampproject:masterfrom
jridgewell:inline-const-class-props

Conversation

@jridgewell
Copy link
Copy Markdown
Contributor

Since we've disabled closure's inlineProperties transform, we lose the ability to DCE based on constant properties:

class Ex {
  constructor() {
    /** @private @const {boolean} */
    this.foo_ = false;
  }

  test() {
    if (this.foo_) {
      // We expect this to be DCE'd (since foo_ is always false)
      // But closure won't do it.
      alert('foo');
    }
  }
}

This inlines the private const properties before closure sees them, so that they can be DCE'd:

// Input
class Ex {
  constructor() {
    /** @private @const {boolean} */
    this.foo_ = false;
  }

  test() {
    if (this.foo_) {
      alert('foo');
    }
  }
}

// Output
class Ex {
  constructor() {
  }

  test() {
    if (false) {
      alert('foo');
    }
  }
}

Since we've [disabled](https://github.com/ampproject/amphtml/blob/33ca73f014a388cca2d94ec19cd868d62334de2a/build-system/runner/src/org/ampproject/AmpCommandLineRunner.java#L78) closure's `inlineProperties` transform, we lose the ability to DCE based on constant properties:

```js
class Ex {
  constructor() {
    /** @Private @const {boolean} */
    this.foo_ = false;
  }

  test() {
    if (this.foo_) {
      // We expect this to be DCE'd (since foo_ is always false)
      // But closure won't do it.
      alert('foo');
    }
  }
}
```

This inlines the private const properties _before_ closure sees them, so that they can be DCE'd:

```js
// Input
class Ex {
  constructor() {
    /** @Private @const {boolean} */
    this.foo_ = false;
  }

  test() {
    if (this.foo_) {
      alert('foo');
    }
  }
}

// Output
class Ex {
  constructor() {
  }

  test() {
    if (false) {
      alert('foo');
    }
  }
}
```
Copy link
Copy Markdown

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Why not enable inlineProperties in Closure instead of adding yet another Babel transform?

jridgewell added a commit to jridgewell/amphtml that referenced this pull request Aug 19, 2019
Supersedes ampproject#24027.

This allows Closure to inline known-constant properties into any accesses, which allows it to DCE if-statement branches that are known to never run.
@jridgewell jridgewell closed this Aug 20, 2019
@jridgewell jridgewell deleted the inline-const-class-props branch August 20, 2019 00:22
jridgewell added a commit that referenced this pull request Aug 20, 2019
* Enable property inlining

Supersedes #24027.

This allows Closure to inline known-constant properties into any accesses, which allows it to DCE if-statement branches that are known to never run.

* Use constant split comment
@jridgewell jridgewell restored the inline-const-class-props branch August 20, 2019 04:31
@jridgewell
Copy link
Copy Markdown
Contributor Author

Agh, it's still needed.

@jridgewell jridgewell reopened this Aug 20, 2019
@jridgewell
Copy link
Copy Markdown
Contributor Author

Not needed for #23968 anymore.

@jridgewell jridgewell closed this Aug 20, 2019
@jridgewell jridgewell deleted the inline-const-class-props branch August 20, 2019 23:12
@jridgewell jridgewell restored the inline-const-class-props branch August 20, 2019 23:12
@jridgewell jridgewell deleted the inline-const-class-props branch August 20, 2019 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants