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

Parse using declaration (explicit resource management) #14968

Merged
merged 14 commits into from Oct 26, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Sep 22, 2022

Q                       A
Fixed Issues? Implement parsing/printing support of the stage-2 Explicit Resource Management proposal
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link babel/website#2684
Any Dependency Changes?
License MIT

This PR parses / generates the using declaration

{
  using handler = getHandler()
}

introduced by the Explicit Resource Management proposal. We also create the new syntax plugin and add it to the babel-standalone.

I suggest reviewing this PR by commits.

/cc @rbuckton

@JLHwung JLHwung added PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Explicit Resource Management labels Sep 22, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Sep 22, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/53239/

lookaheadAhead.type === tt._of &&
lookaheadAhead.end - lookaheadAhead.start === 2
) {
// using of of ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the worst scenario, we have to do triple lookahead to differentiate between using of of of and using of of.

@rbuckton Maybe we can forbid of as a using binding in for-of? So for (using of must be a for-LHS and we don't have to peek more.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it the same as let of of of? Or is of disallowed in that case?

Copy link
Contributor Author

@JLHwung JLHwung Sep 22, 2022

Choose a reason for hiding this comment

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

for ( [lookahead ∉ { let, async of }] LeftHandSideExpression[?Yield, ?Await] of AssignmentExpression[+In, ?Yield, ?Await] ) Statement[?Yield, ?Await, ?Return]

for-of has disabled let and async as LHS so for(let of of must start a for-of with a let-binding of. But using comes after for-of so we have to recognize for(using of of) as for-of with using as LHS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per tc39/proposal-explicit-resource-management#107, I will disallow of as a for-using-of binding name, hopefully we will get rid of extra lookahead, too.

Copy link

Choose a reason for hiding this comment

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

There is now a PR that addresses this in the proposal repository.

@liuxingbaoyu liuxingbaoyu added this to the 7.20.0 milestone Sep 28, 2022
// Ensure no line break between `using` and the first declarator
iterator = (_, i: number) => {
if (i === 0) {
this._noLineTerminator = _noLineTerminator;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also unset _noLineTerminator after printing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean unset _noLineTerminator after printing the using keyword? The variable _noLineTerminator here stores the value of this.noLineTerminator before we print using, and then we restore this._noLineTerminator after the first declarator is printed.

Comment on lines 1 to 4
{
using /* 1 */a = foo(),

/* 2 */
b = foo();
/* 2 */b = foo();
}
Copy link
Member

Choose a reason for hiding this comment

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

The comments and indentation look fine.😃

Copy link
Member

@liuxingbaoyu liuxingbaoyu left a comment

Choose a reason for hiding this comment

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

Just a small question: I noticed that the newly added containsEsc property is only doing reads and no writes, is this expected?

@JLHwung
Copy link
Contributor Author

JLHwung commented Oct 15, 2022

Just a small question: I noticed that the newly added containsEsc property is only doing reads and no writes, is this expected?

The containsEsc will be set by the tokenizer by readWord

this.state.containsEsc = false;

So if we see an identifier, lookahead.containsEsc will be a boolean, otherwise lookahead will not have a containsEsc property.

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Oct 16, 2022
@nicolo-ribaudo nicolo-ribaudo changed the title Parse using declaration Parse using declaration (explicit resource management) Oct 26, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit 8300960 into babel:main Oct 26, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the parse-using-declaration branch October 26, 2022 19:36
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release Spec: Explicit Resource Management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants