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

Consider replacing some expression instanceof AST_Constant with expression.is_constant() across the codebase #1144

Open
WofWca opened this issue Feb 7, 2022 · 3 comments

Comments

@WofWca
Copy link
Contributor

WofWca commented Feb 7, 2022

After creating #1141, I had an idea that it might not be the only case where such a change is beneficial.
I think mostly it would be good for when there is a constant with one (or more (#1142)) unary prefixes, which, I believe is pretty common because we usually transform true to !0 and undefined to void 0, which makes expression instanceof AST_Constant false.
The main issue here is that I don't quite understand why is_constant() returns false for AST_RegExp:

return !(this instanceof AST_RegExp);

Here's the commit that introduced this: 0610c02
Might just try doing this anyway and just seeing if any tests fail?

And I'd be glad to work on this.

@jridgewell
Copy link
Collaborator

The main issue here is that I don't quite understand why is_constant() returns false for AST_RegExp:

is_constant is badly named, and it'd be better to think of it as is_primitive. Regexs are instances of the RegExp and have state (particularly lastIndex).

@fabiosantoscode
Copy link
Collaborator

Also is_constant doesn't return true for anything that has an identity.

Here "num" can be inlined safely into the return (provided it's the only use case):

const num = 123
export function something() {
  return num
}

And now something() === something() always holds true. But the same wouldn't be the case if "num" was a regex.

@fabiosantoscode
Copy link
Collaborator

is_primitive sounds great!

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

No branches or pull requests

3 participants