refactor: remove inClassProperty parser state

[JLHwung]

Q	A
Test is passed	yes
License	MIT

This PR refactors the new.target detection logic, so we can remove unnecessary inClassProperty parser state.

We are now going to prove that !this.scope.inNonArrowFunction && !this.state.inClassProperty is equivalent of !this.scope.inNonArrowFunction && !this.scope.inClass.

By definition of state.inClassProperty, scope.inClass: true implies that state.inClassProperty: true, therefore !state.inClassProperty implies !scope.inClass.

Now we asserts that under the condition of this.scope.inNonArrowFunction: false, !scope.inClass implies !state.inClassProperty.

If scope.inClass: false and scope.inClassProperty: true, we know that thisScope has been changed from SCOPE_CLASS to another scope type. Now by the definition of thisScope

babel/packages/babel-parser/src/util/scope.js

Lines 211 to 212 in 416ce35

	(scope.flags & SCOPE_VAR || scope.flags & SCOPE_CLASS) &&
	!(scope.flags & SCOPE_ARROW)

and the definition of SCOPE_VAR

babel/packages/babel-parser/src/util/scopeflags.js

Line 16 in 416ce35

SCOPE_VAR = SCOPE_PROGRAM | SCOPE_FUNCTION | SCOPE_TS_MODULE;

thisScope can only be one of SCOPE_PROGRAM, SCOPE_FUNCTION, SCOPE_TS_MODULE, since SCOPE_PROGRAM and SCOPE_TS_MODULE can not be nested inside class properties, thisScope must be SCOPE_FUNCTION and it also satisfies !SCOPE_ARROW, which means this.scope.inNonArrowFunction: true, but it contradicts the condition.

Q.E.D.

[mheiber]

I enjoyed the proof and think this is a really cool approach to making changes!

I'm asking the below because I'm interested in the role of verification in transpilers. It's not a criticism, and please feel free to disregard:

What is the advantage of this change? Is the change for readability or for performance?

If the change is for readability, I wonder if the explicitness of the extra parser state might help readability or maintainability, even if a proof shows that the state is redundant due to interactions between other features.

For example, after this change, it seems code maintainers must go through the proof again to see if the conclusion still holds when there is a language feature or Babel refactor that affects the scopes, function types, or class properties.

[JLHwung]

@mheiber Thank you for your insightful comments!

What is the advantage of this change? Is the change for readability or for performance?

The goal of this PR is to remove the usage of state.inClassProperty because its definition is vague. While the name looks straightforward, it is reset to false when parsing function declaration but, for example, not reset when parsing class method.

class C { prop = (function() { /* inClassProperty is false here */ })() }
class C { prop = class () { constructor() { /* inClassProperty is true here */ } } }

IMO inClassProperty should mean that there exists a ClassProperty node in the ancestors of current parsing node. It turns out when inClassProperty was used in the parser, it is more correct to check the scope flags instead of AST ancestry queries because various nodes can be nested in the class properties. (See #10771 for an example where inClassProperty is an inaccurate implementation)

For example, after this change, it seems code maintainers must go through the proof again to see if the conclusion still holds when there is a language feature or Babel refactor that affects the scopes, function types, or class properties.

The current SCOPE_CLASS flag includes class property initializers and class method/accessor body. We can offer some specialized helpers here

get inClassProperty() {
  thisScope satisfies (SCOPE_CLASS: true, SCOPE_FUNCTION: false)
}
get inClassMethod() {
   thisScope satisfies (SCOPE_CLASS: true, SCOPE_FUNCTION: true)
}

However, if any new language features depends on the ancestry query, we can surely bring back state.inClassProperty for a faithful implementation.

[mheiber]

Thanks @JLHwung, I can see how inClassProperty was confusing