[code_builder] Add control-flow support

[one23four56]

TL;DR: Adds full support for control-flow constructs in code_builder:

• if / else if / else (+ if-case) (resolves Add ability to generate if-else blocks #834)
• while, do-while (resolves Add support for while #939)
• for, for-in, await for
• try / catch / finally
• switch statements and expressions
• collection control-flow: if, else, for, for-in (resolves LiteralMapExpression needs support for collection-if expression #1145, resolves Add support for control flow expressions #890)

Background

The implementation is loosely based around discussions in #834. Several ideas were proposed in that thread, but it appears none have been implemented. The primary issue with control-flow implementation, as summed up by @matanlurey, is that:

The problem with builders for statements is there are so many, and it is hard to cover everything.

This implementation works around that issue by using shared visitor logic, similar to the "compound statement builder" idea proposed by @eredo, taking advantage of the fact that all control-flow blocks in Dart are essentially formatted the same. Loosely, every control block can be broken down into:

expression {
  body
}

Going off of that, we really only have to implement visitors for three logical "levels" of control blocks, those being:

• ControlExpression, a control-flow expression
• ControlBlock, a ControlExpression followed by a bracketed body
• ControlTree, a "tree" of multiple ControlBlocks (ie if/else if/else)

For (almost) every concrete implementation of a control-flow block, all it takes is then implementing ControlBlock or ControlTree and the visiting logic comes free of charge.

Control Expressions

Control Expressions are implemented via the internal-only ControlExpression class, a new Expression subtype. The visitor logic for this class is the most complex due to the amount of variation in the formatting of control-flow expressions. It provides numerous constructors, covering each type of control-flow expression (if, for, while, etc.), and should (hopefully) be extendable enough to easily accommodate new control expression types if/when the language is updated.

The class is internal-only, as all of its functionality is exposed via friendlier public builders. Making it public would only add scope noise and potential confusion.

Similar to the way ExpressionVisitor and ExpressionEmitter are structured, this adds ControlBlockVisitor and ControlBlockEmitter as abstract mixin classes mixed into DartEmitter. Visitor logic for ControlExpression is in these control block mixins (rather than the expression ones) for maintainability, as it is easier to have everything in one place instead of arguably the most important part being in a separate file.

Control Blocks

Control Blocks are implemented via the internal-only ControlBlock mixin class. Rather than just exporting that, I decided on something similar to what @TekExplorer proposed, where each specific type of control block implements a public "helper" builder on top of ControlBlock to make the API more user-friendly. In total, there are 5 of these builders:

• ForLoop (for)
• ForInLoop (for-in, await for)
• WhileLoop* (while, do-while)
• Condition (if, else if, else) (single) (see conversation)
  • Supports if-case via ConditionBuilder.ifCase BranchBuilder.ifCase
• CatchBlock (catch)

WhileLoop requires a custom visitor due to the existence of the do-while loop, which does not follow the "standard" control block format. All others simply mix in ControlBlock.

I opted not to support single-line control blocks (with no brackets). These are entirely stylistic; functionally it does not matter whether if (value) return; has brackets or not, and if (value) return; is really the only place where this would be used, as it is bad form to omit the braces if the statement body overflows onto the next line. That being said, I did add Expression.ifThenReturn which is shorthand to generate such an expression and which, out of simplicity, omits the braces (see Expression Helpers).

You may have noticed that I didn't include switch statements in this section. No surprise, they also required a separate visitor, among other unique implementation details, so they are in their own section, see Switches.

Control Trees

Control Trees are implemented via the internal-only ControlTree mixin class, using the same helper builder concept as control blocks. In total there are two such builders:

• IfTree Conditional for creating if-else trees (see conversation)
• TryCatch for try/catch/finally trees.

IfTree was named that way instead of something like ConditionalTree in order to avoid autocomplete conflicts with Condition when using the API, which proved to be sufficiently annoying during development. (see conversation)

As try and finally blocks don't have any parameters, TryCatch implements them as a Block for simplicity, hence the lack of a TryBlock or FinallyBlock. CatchBlock is not implemented this way to support customizing the error/stacktrace parameter names as well as on clauses.

Switches

The implementation for switch statements and expressions is fairly complex, largely due to the existence of two different switch types. Because of this, most functionality is located in the internal-only Switch and SwitchBuilder classes, which both switch subtypes (SwitchStatement and SwitchExpression) then extend.

Rather than confusingly exposing two Case types (switch is bad enough), generic Case<T> is public, with the switch-specific implementations being in two separate internal classes, CaseStatement and CaseExpression. Case<T> is not visitable; the aforementioned specific implementations each implement Code and have their own visitors.

The two switch types each provide a private getter _cases, which converts the Case<T>s from the switch's public cases field into their visitable, switch-specific counterparts. When visited, the switch's ControlExpression is generated, and _cases is used to create a Block. These two values are then passed into a BuildableSwitch, yet another internal-only class, which implements ControlBlock.

After all that (admittedly convoluted) conversion, the BuildableSwitch is passed into the control block visitor and from then on generated the same way as any other ControlBlock.

Expression Helpers

Several helper methods were also added to Expression to facilitate easier usage of control-flow blocks. Rather than adding them to Expression directly, they were added to ControlFlow, an extension on Expression. This was done primarily for ease of maintenance due to the unwieldy nature of expression.dart, but also to allow users of the package to easily be able opt-out of these specific helpers via hide if desired.

Added the following helpers:

• Expression.yielded, Expression.yieldStarred
• Expression.ifThen
  • Expression.ifThenReturn
• Expression.loopWhile, Expression.loopDoWhile
• Expression.loopForIn

Additionally added some static helpers to ControlFlow:

• ControlFlow.breakVoid, ControlFlow.breakLabel
• ControlFlow.continueVoid, ControlFlow.continueLabel
• ControlFlow.returnVoid
• ControlFlow.rethrowVoid
• ControlFlow.ifCase

Finally, static Expression.wildcard was also added. It was added to Expression rather than ControlFlow as it felt more idiomatic (a wildcard isn't really a control flow statement like the others on ControlFlow are)

Collection Control-Flow

Collection control-flow support was implemented as static methods on the ControlFlow extension, particularly:

• ControlFlow.collectionIf
• ControlFlow.collectionElse
• ControlFlow.collectionFor
• ControlFlow.collectionForIn

These are fairly simple; however, they did require some slight changes to visitAll and the various collection literal visitors in order to support chaining, ie:

[if (value) item else other] // ✅ works
[if (value) item, else other] // ❌ doesn't work

{ if (value) key: item else otherKey: other } // ✅ works
{ if (value) key: item, else otherKey: other } // ❌ doesn't work

I tried to implement this via a child/nesting system, but that ended up becoming prohibitively complicated (primarily due to map literals). A chaining-based system proved to be much simpler.

Other Changes

• The current .g.dart files were generated with an older version of built_value and contained some redundant checks and new keyword usage. When I ran the built script it updated all those files, so I've also pushed those versions.
• Fixed some typos in the expression_test.dart file and on the changelog (also, apologies, I guess my editor automatically formatted the changelog and replaced all - with *, something which I didn't realize until just now as I am writing this)
• Rewrote part of the README to be more beginner friendly (I'll take this out if it isn't allowed (I haven't contributed here before so I'm not sure), it was just a personal gripe I had from when I used this the first time)
• Bumped the version from 4.10.2-wip to 4.11.0-wip in pubspec.yaml
• Added relevant changelog entries

Tests

Added comprehensive unit tests for all changes. Per package:coverage and LCOV, all touched files (excluding generated .g.dart) have at least 97% test coverage.

Happy to answer any questions or make any changes requested, I've got lots of free time at the moment and this is something I'd love to see code_builder support.

---

• [✅] I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.
• Most changes should add an entry to the changelog and may need to rev the pubspec package version.
• Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

[TekExplorer]

Woah! My little helper method grew up!

Didn't expect to see this when I woke up today!

Anyway, I notice (looking at the tests) that the catch blocks have a default name for the exception - i'd honestly just put a wildcard if its empty.

We don't always want the value, since we can always rethrow after doing whatever. (plus we dont want code_builder to be the reason for shadowing, right? Who knows what local variables devs make?)

Additionally, if neither exception nor stack are specified but the on type is, then it should omit the catch entirely; on FormatException catch (_) {...} vs on FormatException {...}

How you handle switch statements/expressions seems to be pretty good, at least on the surface.

The Condition/IfTree separation kind of bothers me. then again, there's no reason we cant have extensions that let us sort of ignore IfTree exists by letting us do Condition().elseIf, so maybe thats not a problem? The overlap just feels a bit weird.

Edit: looking further, it does look like you have those extensions, so it may not be a problem at all.

That said, the collection ones seem to be third class citizens. I think that does need improving, because we should effectively have the full power of our conditionals and loops, just with an expression in place of a body.

Edit 2: It may be that we're just missing if-case for that actually, though i could be missing something myself. Something to look into.

[one23four56]

Thanks for the suggestions!

i'd honestly just put a wildcard if its empty.

Fully agree here, updated to fix that. I guess I always just use e so that didn't even cross my mind.

Additionally, if neither exception nor stack are specified but the on type is, then it should omit the catch entirely

Fixed this as well. I knew I forgot something! Totally overlooked this during development.

The Condition/IfTree separation kind of bothers me.

Same here to some degree, but I couldn't really find a better solution. A Condition is meant to be an invididual "block" of an IfTree, and I initially was going to make it private, but that was complicated by it being a builder. A function like IfTreeBuilder.ifThen would then require usage of this private builder, which is undesirable for numerous reasons. I would love to have if/else stuff contained in a single class, but AFAIK that would required abandoning the builder paradigm for conditions. Not the end of the world given that they're pretty simple, but does seem somewhat out of place. I could definitely implement that if desired. If anyone has a better solution LMK.

That said, the collection ones seem to be third class citizens. I think that does need improving, because we should effectively have the full power of our conditionals and loops, just with an expression in place of a body.

This is also something that somewhat bugs me as well. The way I see it, there are three ways to implement collection control-flow:

1. Add new versions of the existing builders for collection purposes or modify the current ones to support it via an option.

   • Having two versions of certain builders (eg ForLoop and CollecionForLoop) bugs me as far as API design goes, and would also require lots of work to essentially re-implement existing logic. I could see something like a flag (ie bool? collection or similar) working, but that would be deceptively complex to implement (or at least, implement well), since collection control-flow bodies are Expression while traditional ones are Block. Perhaps adding another constructor would work (eg ForLoop.collection())?

2. Add helper methods

   • Obviously this is what I ended up doing, but I don't think it is necessarily the "best" option. As mentioned it does inherently reduce the "power" of collection expressions versus their traditional counterparts. I chose this operating under the assumption that collection control-flow is rarely used (in generated code, at least) so doesn't really need a full builder implementation. I may be wrong on that, and in that case we should explore a different option. I felt like this struck somewhat of a balance, ie, they're there if needed, but won't "jump out" if not.

3. Remake literal collection operations to add first-class support

   • This is my personal favorite option, but I didn't implement it because I feel like there should be a community discussion first. Adding some sort of a builder implementation for literal lists/sets/maps (and possibly deprecating the existing one?) would allow "true" first-class support for collection control-flow via something like Map.addIf, and would also make working with spread operators easier. Of course, this would also be overkill for most situations so there would need to be Map.fromLiteral or similar (so maybe leave the existing functions?). I think this is a good idea, but it definitely needs further discussion.

Edit 2: It may be that we're just missing if-case for that actually, though i could be missing something myself. Something to look into.

If-case is actually supported via using ControlFlow.ifCase as the condition, eg:

ControlFlow.collectionIf(
  condition: ControlFlow.ifCase(
    object: // ...
    pattern: // ...
  ),
  value: // ...
);

Maybe I should add a dedicated helper like ControlFlow.collectionIfCase? Or add a reference to it in the docs? Definitely want people to know it exists 😅

[one23four56]

Alright, I thought for a little longer on the IfTree/Condition separation and made some changes.

First off, I renamed IfTree -> Conditional and Condition -> Branch (along with their builders). Branch no longer implements ControlBlock and is no longer visitable, instead using a private intermediary class like Case does (this had the bonus side effect of simplifying else logic, too).

Branch is no longer exported and is no longer part of the public API. Only BranchBuilder is visible, which should help reinforce the idea that Branch is something used with Conditional and not its own thing. The methods on Conditional have been modified to exclusively use BranchBuilder, which also resulted in the elimination of several redundant methods.

Updated the tests, changelog and docs to reflect this. Also, I realized that despite mentioning Expression.ifThenReturn in the PR, I totally forgot to actually implement it (oops 😅), so I also fixed that.

Hopefully these should address the overlap. Now that I'm out of the dev tunnel vision I realize how confusing that was.