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.

Sign up for GitHub

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

Jump to bottom

break: Add support for labeled statements, breaks, and continues #2891

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

break: Add support for labeled statements, breaks, and continues #2891

wants to merge 3 commits into from

Conversation

CountBleck
Copy link
Member

@CountBleck CountBleck commented Dec 16, 2024 •
edited
Loading

Fixes #2889.

Changes proposed in this pull request:
⯈ Support labeled statements in the parser/AST
⯈ Support labeled break/continue.

This change is breaking solely because it modifies the Node.createXXX APIs, which are used by transforms.

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@CountBleck CountBleck changed the title Add support for labeled statements, breaks, and continues BREAKING CHANGE: Add support for labeled statements, breaks, and continues Dec 16, 2024
@CountBleck CountBleck changed the title BREAKING CHANGE: Add support for labeled statements, breaks, and continues break: Add support for labeled statements, breaks, and continues Dec 16, 2024
@CountBleck CountBleck force-pushed the labeled-statements branch 3 times, most recently from 60e57da to 3a29149 Compare December 16, 2024 23:05
@CountBleck CountBleck marked this pull request as ready for review December 16, 2024 23:05
@CountBleck
Copy link
Member Author

I just need to add some tests; then, this will be ready to merge.

@JairusSW Heads up: this change definitely breaks transforms. Do you have anything to comment on this PR?

@JairusSW
Copy link
Contributor

You can define multiple constructors

@CountBleck CountBleck marked this pull request as draft December 17, 2024 00:40
@CountBleck
Copy link
Member Author

It looks like I forgot about if...

@CountBleck
Copy link
Member Author

CountBleck commented Dec 17, 2024 •
edited
Loading

Okay, it appears that my code breaks something related to Flow flags, so more work on this PR is needed.

One issue is that breaking out of a labeled block sets the Breaks flag on the outer flow and messes with the function not appearing to always terminate.

Another issue is that a continue (and likely break as well) from inside an inner loop to an outer loop won't set the Continues/ConditionallyContinues flow flag on the outer loop body's flow. This is a much more significant issue (since nested loops are the most common use of labels) and seems to be more difficult to fix...

@CountBleck
Add support for labeled statements to the parser/AST
66a29fc 
This is a prerequisite for supporting labeled breaks/continues. Clearly
unusable labels, such as `x: let foo = 1;` report an error by default,
similar to TS's behavior.
@CountBleck
Add support for labeled break/continue
b47dc99 
This requires an additional field to Flow that maps user-defined
statement labels to the internal Binaryen labels passed to module.br().
Thanks to the existing logic to handle unlabeled break/continue, adding
support for labeled break/continue is a breeze.

Fixes AssemblyScript#2889.
@CountBleck
Add tests for labeled break/continue
692b388
@CountBleck CountBleck removed the request for review from HerrCai0907 December 17, 2024 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support labeled statements
2 participants
@CountBleck @JairusSW