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

Forbid impure method calls inside dev().assert() #14683

Closed
jridgewell opened this issue Apr 17, 2018 · 4 comments
Closed

Forbid impure method calls inside dev().assert() #14683

jridgewell opened this issue Apr 17, 2018 · 4 comments
Assignees

Comments

@jridgewell
Copy link
Contributor

jridgewell commented Apr 17, 2018

We have a build step that removes dev().assert calls:

// input:
function hello() {
  dev().assert(ASSERTION, 'some text to describe');
}

// output:
function hello() {
  ASSERTION;
}

// v0.js:
function hello() {
}

Generally, that ASSERTION then gets removed in v0.js, because it's a useless (side-effect free) reference. But, sometimes we assert call expressions...

Now, Closure Compiler sucks pretty badly tries to decide if a method call will have a side effect. So, this will not be eliminated from the final build.

// input:
function hello() {
  dev().assert(array.includes('something'), 'some text to describe');
}

// output:
function hello() {
  array.includes('something');
}

// v0.js:
function hello() {
  array.includes('something');
}

That's no good. We know that call it totally useless, but Closure doesn't. And that useless call stays there, and over the numerous assertions we do, those bytes add up.

Proposal

From now on, any call expression inside dev().assert() must be marked as /*PURE*/:

dev().assert(array./*PURE*/includes())

dev().assert(someCheck/*PURE*/())

Using this, we will now that we can completely remove the assertion:

// input:
function hello() {
  dev().assert(array./*PURE*/includes('something'), 'some text to describe');
}

// output:
function hello() {
  undefined;
}

// v0.js:
function hello() {
}
@jridgewell jridgewell changed the title Forbid method calls inside dev().assert() Forbid impure method calls inside dev().assert() Apr 17, 2018
@jridgewell jridgewell self-assigned this Apr 17, 2018
@ampprojectbot ampprojectbot added this to the Pending Triage milestone Jun 5, 2018
@ampprojectbot
Copy link
Member

This issue seems to be in Pending Triage for awhile. @jridgewell Please triage this to an appropriate milestone.

1 similar comment
@ampprojectbot
Copy link
Member

This issue seems to be in Pending Triage for awhile. @jridgewell Please triage this to an appropriate milestone.

@rsimha
Copy link
Contributor

rsimha commented May 30, 2019

@jridgewell Does this issue remain with the change to @closurePrimitive annotations added in 5ebcee3#commitcomment-33643222 in #22446?

@rsimha
Copy link
Contributor

rsimha commented Apr 30, 2020

Closing as stale. Please reopen if there's anything left to do.

@rsimha rsimha closed this as completed Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants