Skip to content

new control flow keyword: result #732

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

Closed
andrewrk opened this issue Feb 1, 2018 · 13 comments
Closed

new control flow keyword: result #732

andrewrk opened this issue Feb 1, 2018 · 13 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Feb 1, 2018

One awkward situation is this pattern:

    const result_is_to = x: {
        if (parsed_from.kind != parsed_to.kind) {
            break :x true;
        } else switch (parsed_from.kind) {
            WindowsPath.Kind.NetworkShare => {
                break :x !networkShareServersEql(parsed_to.disk_designator, parsed_from.disk_designator);
            },
            WindowsPath.Kind.Drive => {
                break :x asciiUpper(parsed_from.disk_designator[0]) != asciiUpper(parsed_to.disk_designator[0]);
            },
            else => unreachable,
        }
    };

In all of these situations, we just want to break from the current block.

Proposal: result keyword, which is like break but it always breaks from the current block.

The example becomes:

    const result_is_to = {
        if (parsed_from.kind != parsed_to.kind) {
            result true;
        } else switch (parsed_from.kind) {
            WindowsPath.Kind.NetworkShare => {
                result !networkShareServersEql(parsed_to.disk_designator, parsed_from.disk_designator);
            },
            WindowsPath.Kind.Drive => {
                result asciiUpper(parsed_from.disk_designator[0]) != asciiUpper(parsed_to.disk_designator[0]);
            },
            else => unreachable,
        }
    };
@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Feb 1, 2018
@andrewrk andrewrk added this to the 0.3.0 milestone Feb 1, 2018
@thejoshwolfe
Copy link
Contributor

thejoshwolfe commented Feb 1, 2018

None of those break statements are breaking from the "current block". The current block is the inner most set of {}, which is the "then" block and the "case" blocks in your example.

We could fix this with a companion keyword to specify which block the result keyword breaks to. With nearly 0 thought put into choosing the actual keyword, here's an iteration on your proposal using block and result as new keywords.

    const result_is_to = block {
        if (parsed_from.kind != parsed_to.kind) {
            result true;
        } else switch (parsed_from.kind) {
            WindowsPath.Kind.NetworkShare => {
                result !networkShareServersEql(parsed_to.disk_designator, parsed_from.disk_designator);
            },
            WindowsPath.Kind.Drive => {
                result asciiUpper(parsed_from.disk_designator[0]) != asciiUpper(parsed_to.disk_designator[0]);
            },
            else => unreachable,
        }
    };

Now here's the same thing in status quo zig:

    const result_is_to = while (true) : (unreachable) {
        if (parsed_from.kind != parsed_to.kind) {
            break true;
        } else switch (parsed_from.kind) {
            WindowsPath.Kind.NetworkShare => {
                break !networkShareServersEql(parsed_to.disk_designator, parsed_from.disk_designator);
            },
            WindowsPath.Kind.Drive => {
                break asciiUpper(parsed_from.disk_designator[0]) != asciiUpper(parsed_to.disk_designator[0]);
            },
            else => unreachable,
        }
    };

This while (true) : (unreachable) thing doesn't communicate intent very clearly.

@andrewrk
Copy link
Member Author

andrewrk commented Feb 1, 2018

Better example of the common pattern:

            if (windows.GetExitCodeProcess(self.handle, &exit_code) == 0) x: {
                break :x Term { .Unknown = 0 };
            } else x: {
                break :x Term { .Exited = @bitCast(i32, exit_code)};
            }

@raulgrell
Copy link
Contributor

I think the result keyword is the ideal solution, as it would be semantically equivalent to an implicit block return, but... you know, explicit.

Anything that would require more granular control would be done with the named blocks as currently supported.

@thejoshwolfe
Copy link
Contributor

Ok here's the original example with the "current block" semantics done properly:

    const result_is_to = (
        if (parsed_from.kind != parsed_to.kind)
            result true;
        else switch (parsed_from.kind) {
            WindowsPath.Kind.NetworkShare =>
                result !networkShareServersEql(parsed_to.disk_designator, parsed_from.disk_designator),
            WindowsPath.Kind.Drive =>
                result asciiUpper(parsed_from.disk_designator[0]) != asciiUpper(parsed_to.disk_designator[0]),
            else => unreachable,
        }
    );

Or without removing any of the curly braces, and using a minimum number of result keywords:

    const result_is_to = {
        result if (parsed_from.kind != parsed_to.kind) {
            result true;
        } else switch (parsed_from.kind) {
            WindowsPath.Kind.NetworkShare => {
                result !networkShareServersEql(parsed_to.disk_designator, parsed_from.disk_designator);
            },
            WindowsPath.Kind.Drive => {
                result asciiUpper(parsed_from.disk_designator[0]) != asciiUpper(parsed_to.disk_designator[0]);
            },
            else => unreachable,
        }
    };

Or putting the result keywords where they are most pertinent, which is where you introduce a new block scope:

    const result_is_to = {
        if (parsed_from.kind != parsed_to.kind) result {
            result true;
        } else switch (parsed_from.kind) {
            WindowsPath.Kind.NetworkShare => result {
                result !networkShareServersEql(parsed_to.disk_designator, parsed_from.disk_designator);
            },
            WindowsPath.Kind.Drive => result {
                result asciiUpper(parsed_from.disk_designator[0]) != asciiUpper(parsed_to.disk_designator[0]);
            },
            else => unreachable,
        }
    };

These semantics seem pretty confusing to keep up with. I think I like my while (true) : (unreachable) solution better. Might just be my opinion though.

@raulgrell
Copy link
Contributor

My understanding is that the following examples would be equivalent, the first using only expressions, and the second wrapping every expression in a block:

const result_is_to = if (parsed_from.kind != parsed_to.kind)
    true
else switch (parsed_from.kind) {
    WindowsPath.Kind.NetworkShare => !networkShareServersEql(parsed_to.disk_designator, parsed_from.disk_designator),
    WindowsPath.Kind.Drive => asciiUpper(parsed_from.disk_designator[0]) != asciiUpper(parsed_to.disk_designator[0]),
    else => unreachable,
};

And with blocks around every expression

const result_is_to = { 
    result if (parsed_from.kind != parsed_to.kind) {
        result true;
    }
    else {
        result switch (parsed_from.kind) {
            WindowsPath.Kind.NetworkShare => {
                result !networkShareServersEql(parsed_to.disk_designator, parsed_from.disk_designator);
            }, 
            WindowsPath.Kind.Drive => {
                result asciiUpper(parsed_from.disk_designator[0]) != asciiUpper(parsed_to.disk_designator[0]);
            },
            else => unreachable,
        };  
    } 
};

I think in these examples the semantics are a bit clearer.

Your while (true) : (unreachable) is actually pretty good though, the semantics are at least more familiar to C (and etc) programmers. We would basically have two kinds of blocks: a "returning block" => block { } and a "scope block" => { }.

All blocks are equal, but some blocks are more equal than others =P

@tjpalmer
Copy link

I agree the current situation is awkward. I had to work that out once, and it wasn't super fun. Are any of these many new keyword proposals context sensitive? Or is there some escape for identifiers like in C# (or shell or whatnot)? Just that a lot of good identifier names are getting taken.

@andrewrk andrewrk modified the milestones: 0.3.0, 0.4.0 Feb 28, 2018
@jido
Copy link

jido commented Mar 8, 2018

You could use a closure and return instead of while (true) : (unreachable).

@andrewrk
Copy link
Member Author

I understand this is a relatively popular proposal, but I'm going to reject it. People don't like the overly verbose

const x = blk: {
    break :blk foo();
};

Especially when it's one line like that. If break was a new concept, I would make it always apply to the current block, rather than the innermost loop. But it's widely conventional in various C-like syntaxes for break to have status quo semantics.

Given that, and given that we do need the explicit syntax for breaking from an outer loop, a new keyword to break from the current block is redundant. Status quo, while verbose, allows people reading zig code to understand the control flow without being an expert in zig syntax, and, importantly, teaches zig coders how to use the explicit syntax for when they need it in those situations. This feature is strictly for convenience, and makes the language bigger. Although undeniably convenient, it's not worth the cost.

@andrewrk andrewrk modified the milestones: 0.5.0, 0.4.0 Apr 8, 2019
@shawnl
Copy link
Contributor

shawnl commented Jul 27, 2019

Why can't the name of the block be optional? Then we use the implicit scoping runes of while and for, extended to initialization blocks.

const x = {
    break foo();
};

@ghost
Copy link

ghost commented Apr 15, 2020

Another argument for this proposal would be that #4294 could be reconsidered because to my understanding it was closed for readability reasons in situations like this:

x = if (c) label: { break :label a; } else label: { break :label b; }

With this proposal and #4294, it could be written as:

x = if (c) { result a; } else { result b; }

which in my opinion is quite decent.

Edit: I still think this is a good approach, but this (reverting 629) is a better one imo.

@ghost
Copy link

ghost commented Apr 15, 2020

x = if (c) { result a; } else { result b; }

Wouldn't even need a new keyword if => was used:

x = if (c) { break => a; } else { break => b; }

I also wrote this in a another comment, but I hope it won't hurt to include it here as well.
Suggestion for new break syntax including => token to specify return (result) value:

break;                // normal
break => value;       // when returning a value from the block
break label;          // when breaking a specified label
break label => value; // when doing both of the above 
// break :label                  <-  the colon prefix would not be necessary anymore.

@ArborealAnole
Copy link

ArborealAnole commented Oct 16, 2021

In my opinion, this proposal should be reconsidered. There have been several proposals since (e.g. #2990, #5382, #5083, #9758) that aim to address the underlying problem but in my opinion none do so as well as this proposal.

The main problem stemming from #629 seems to be that the ergonomics of using blk labels to break the innermost block are not great. It is redundant to manually create the common pattern of returning from the innermost block, often with meaningless labels, and the convenience level is somewhat inconsistent given that we have break (value); for breaking out of the innermost loop.

For example: in the Zig codebase of 2021 September 10, uses of break :label value exceed all other uses of break by over 80%, and most of these return from the innermost block (per zzyxyzz).

This proposal would add value to simplify such cases with result, and would demonstrate their logical similarity. It would make it clearer both

  • where there are any innermost-loop breaks (which may be less common than innermost-block breaks)
  • where there are labeled-breaks that involve control flow that may be even less common than innermost loop breaks (those that labeled-break a non-innermost block).

As to instructive tradeoffs, the existence of the keyword result would make it clearer to Zig programmers that returning from blocks is both possible and not automatically performed with the last statement like in C/C++; and make the return-from-block pattern more usable in general, including where it is more elegant and clear than say, to assignment to a predeclared variable. This would reduce problems such as #9059.

Arguments against some alternative mitigations:

  • Restore block expressions #9758: Just returning the expression of the last statement of a block would not be fully general (it disallows returning in an earlier statement). Always returning the last statement would be too implicit and non-obvious, but only returning a last expression with no semicolon would be too unclear (per explicitly return from blocks instead of last statement being expression value #629). If requiring no semicolon to return value in last expression, semicolon omission or presence could cause problems despite type safety due to generics.

  • make break apply to all blocks, not just loops #2990: The natural approach (break x; === break :blk x; and break; === break :blk;) would make break inconsistent with C and continue. And it is a seriously breaking change. If you do break x; === break :blk x; but not break; === break :blk;, that is inconsistent itself.

  • Proposal: New labeled break syntax, removing colon prefix by reusing => token,  #5083: Using => in this manner is not very intuitive. The symbol seems to be taken from switch but => means "implies" and break is not a case or truth value. It also makes a non-obvious difference between the control-flow of break => value; and break;. And how to return nothing from the current block, break =>;? Personally I like the symmetry of label: and break :label.

@rohlem
Copy link
Contributor

rohlem commented Oct 21, 2021

Here's what I end up doing relatively consistently by now in my code.

adapted example from original post
const result_is_to = result_is_to: {
    break :result_is_to if (parsed_from.kind != parsed_to.kind) true
    else switch (parsed_from.kind) {
        WindowsPath.Kind.NetworkShare => {
            //say for some reason we actually need a block here
            break :result_is_to !networkShareServersEql(parsed_to.disk_designator, parsed_from.disk_designator);
        },
        WindowsPath.Kind.Drive => asciiUpper(parsed_from.disk_designator[0]) != asciiUpper(parsed_to.disk_designator[0]),
        else => unreachable,
    };
};

That is, in essence

  • surround the right-hand side (no matter how many if/else/switches control flow structures) with a block named after the variable that will be assigned to
  • if there is control flow branching at the end of that block (in this case immediately) prefix it with break :<block name> and suffix it with ;

While the = <variable name>:{ break :<variable name> at the beginning of deeper nesting is a bit of up-front Ctrl+C/V overhead, this pattern allows you to consistently:

  • use expression syntax (without explicit break) where convenient
  • end longer blocks with a named break that directly references the variable name.
    This makes it extra easy to track what value you are providing, even with arbitrarily deep/complex nesting (highly Ctrl+F -able).
(Also here is a idea I thought unworthy of its own proposal now that the issue template tells you "no proposals", but it applies directly to this use case, so I'll mention too:)

If this is considered "proper style", and everyone agrees on the benefits of mentioning the variable in the named break, the language could automatically provide the labeling. That is:

  • Assignment statements with a variable name on the left automatically introduce a labeled block around their right hand side.
  • To facilitate this, variables and labels would now be required to share a namespace (=> shadowing via user-provided explicit labels should no longer allowed).
    While this is a new restriction, at least in my code I've never intentionally shadowed them in any different way than described here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests

8 participants