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

Proposal: obligatory captures #8400

Closed
yohannd1 opened this issue Apr 1, 2021 · 3 comments
Closed

Proposal: obligatory captures #8400

yohannd1 opened this issue Apr 1, 2021 · 3 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@yohannd1
Copy link

yohannd1 commented Apr 1, 2021

I have noticed that, in some contexts, captures can be omitted. Check out this example:

const v: error{A}!i32 = error.A;

if (v) |_| {} else |_| {} // explicitly ignoring the values captured
if (v) {} else {} // the captures are completely omitted, yet this still works

Not only this makes it so there are two different ways to ignore a capture, but sometimes this can also contribute for the programmer to completely forget the variable. If #335 gets implemented, simply not capturing the variable (which could happen due to forgetting to type the capture) would probably not result in unused identifier errors/warnings.

Here is a list of places where I tried to analyze these captures:

test "capturing and ignoring on an optional value" {
    // This one seems to work pretty much as this proposal suggests.

    const val: ?i32 = null;

    // :: Explicitly ignoring the capture
    // Compiles successfully (as wanted)
    if (val) |_| {} else {}

    // :: Ommiting the capture
    // Does not compile, although the error is somewhat cryptic. I might be interpreting it wrong...
    if (val) {} else {}
}

test "capturing and ignoring on an error union" {
    const val: error{A}!i32 = error.A;

    // :: Explicitly ignoring both captures
    if (val) |_| {} else |_| {}

    // :: Explicitly ignoring only the error capture
    // This works fine - with this proposal, it shouldn't.
    if (val) {} else |_| {}

    // :: Explicitly ignoring only the success capture
    // Hmm... this one errors out, but because the compiler thought we were trying to capture an optional value.
    if (val) |_| {} else {}
}

test "forgetting to capture on a for loop" {
    const arr: [5]u8 = [_]u8{ 1, 2, 3, 4, 5 };

    // This one hopefully does not compile already, although it complains about `{` being an invalid token. Maybe a
    // clearer error (a note maybe?) could be shown here ("missing capture").
    for (arr) {}
}

test "capturing and ignoring on a while loop" {
    // Just a sample struct to iterate from 0 to 4.
    const To4Iterator = struct {
        i: u8 = 0,

        fn next(self: *@This()) ?u8 {
            if (self.i < 5) {
                self.i += 1;
                return self.i - 1;
            } else {
                return null;
            }
        }
    };

    var it = To4Iterator{};

    // Works as expected
    while (it.next()) |_| {}

    // Hopefully this doesn't work, but, as the other errors, the one from here is kind of unclear and might not be for this reason.
    while (it.next()) {}
}
@Vexu
Copy link
Member

Vexu commented Apr 1, 2021

Currently the behavior on if and while is that no captures makes the conditional boolean, capture after the condition makes it an optional and a capture after else makes it an error union. for works differently always requiring a capture after the condition and never allowing one after else.

// This works fine - with this proposal, it shouldn't.

Here's Andrew's current proposal to how this should work #6060 (comment).

The cryptic errors should improve once stage2 is ready.

@yohannd1
Copy link
Author

yohannd1 commented Apr 1, 2021

That seems to solve these problems, thanks!

I just noticed though that I forgot to mention catch - are there any plans to make the capture obligatory there?
At least on my version (0.8.0-dev.1482+6787f163e) the behavior is kind of strange:

fn doSomething() error{A}!void {}

test "catch tests" {
    // This seems to be the current way to ignore an error.
    doSomething() catch unreachable;

    // This causes a compile error, and a rather strange one:
    //
    // "error: unused variable: '_'"
    //
    // Is it using `_` as a normal identifier..?
    doSomething() catch |_| unreachable;
}

Also, I do think that catch without a capture could be used for error types with only one variant, but I'm not sure if that would be the best way to express that. It would be nice though, because you then wouldn't need to type an entire catch |err| switch (err) { error.A => { doThing(); } } and at the same you wouldn't risk ignoring other possible errors added on the future.
Should I make this another proposal, though? I think I might be going off a tangent here.

@alexnask alexnask added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Apr 1, 2021
@alexnask alexnask added this to the 0.8.0 milestone Apr 1, 2021
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 May 19, 2021
@yohannd1
Copy link
Author

@andrewrk Does the closure mean it's been rejected or implemented? Since some stuff was already in the works (#6060, as previously mentioned) I'm not sure about what has been decided 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

4 participants