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

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

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

Conversation

CountBleck
Copy link
Member

@CountBleck CountBleck commented Dec 16, 2024

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

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...

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.
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 CountBleck removed the request for review from HerrCai0907 December 17, 2024 05:47
@HerrCai0907
Copy link
Member

In TS, there are new ASTNode LabeledStatement to identifier this labeled statement, could we use the same concept?

@CountBleck
Copy link
Member Author

@HerrCai0907 I considered that before, and it might be a pretty good idea. It would be better for transform users for sure.

Still, there's the issue of setting the proper FlowFlags for a continue to an outer loop...maybe the class I use to keep track of labels should contain the flow, and I can use something like do flow.set(...) while ((flow = flow.parent) != targetFlow). (Of course, the actual code would look nicer than that.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support labeled statements
3 participants