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

Introduce var/let-else-statement #1871

Closed

Conversation

rscircus
Copy link
Contributor

@rscircus rscircus commented Aug 1, 2022

As implied in PR #1388 and requested in #1758 this proposal introduces the rationale behind the simplification of a common error-handling pattern, informally known as let-else statement (or var-else statement). It can be interpreted as syntactic sugar for a match-statement where the non-matched case diverges. Hence, the usage as error-handling pattern.

@rscircus
Copy link
Contributor Author

rscircus commented Aug 1, 2022

@josh11b - this needs the labels proposal and WIP. Can't add them unfortunately.

@github-actions github-actions bot added proposal A proposal proposal rfc Proposal with request-for-comment sent out labels Aug 1, 2022
@josh11b josh11b marked this pull request as draft August 1, 2022 21:26
@josh11b
Copy link
Contributor

josh11b commented Aug 1, 2022

Looking at the files changed, looks like this is still a work in progress, so I've made it a draft proposal.

@rscircus
Copy link
Contributor Author

rscircus commented Aug 1, 2022

Looking at the files changed, looks like this is still a work in progress, so I've made it a draft proposal.

Yep. Still putting some thought into this right now. Especially, into @geoffromer's comment "whether we should have this feature at all. There may be other combinations of pattern matching/error handling features which would make this one superfluous".

@jonmeow jonmeow removed the proposal rfc Proposal with request-for-comment sent out label Aug 1, 2022
@rscircus
Copy link
Contributor Author

rscircus commented Aug 1, 2022

OK, so far so good. I added another suggestion for this pattern:

let (x: i32, true) = F(1) else {
  // Can't use `x` here.
  return false;
}

=>

let (x: i32, true) = F(1) ?: {  return false; }

A "compressed" ternary operator omitting the happy path. It'll be very familiar to all curly-brace language programmers, I think.

@rscircus
Copy link
Contributor Author

rscircus commented Aug 1, 2022

Considering @geoffromer comment, I think, it can be, that depending upon how or is implemented, this might work out of the box:

var/let (x: i32, true) = F(1) or { return; };

@rscircus
Copy link
Contributor Author

rscircus commented Aug 1, 2022

Looking at the files changed, looks like this is still a work in progress, so I've made it a draft proposal.

If the label recommendation of WIP changed, it might make sense to update the equivalent line in the template. I'll throw in a PR in a sec.

Edit: Here it is: #1874

@rscircus rscircus marked this pull request as ready for review August 1, 2022 22:55
@rscircus
Copy link
Contributor Author

rscircus commented Aug 2, 2022

@geoffromer
Copy link
Contributor

This PR is marked "ready for review", but it's still in the "Draft" column of the "Proposals" project (see "Projects" on the right side of the GitHub window). If you intend this to be ready for review, I think it needs to be moved to the "RFC" column. Otherwise, the PR should be marked as a draft (use the "convert to draft" link in the "Reviewers" section).

It's unfortunate that we track this information in two places, but I'm not sure if there's a better option :-(

@jonmeow
Copy link
Contributor

jonmeow commented Aug 12, 2022

This PR is marked "ready for review", but it's still in the "Draft" column of the "Proposals" project (see "Projects" on the right side of the GitHub window). If you intend this to be ready for review, I think it needs to be moved to the "RFC" column. Otherwise, the PR should be marked as a draft (use the "convert to draft" link in the "Reviewers" section).

It's unfortunate that we track this information in two places, but I'm not sure if there's a better option :-(

I'm assuming that this is intended to be in rfc given the "ready for review", and marking it such. See #1898 for some context.

@jonmeow jonmeow added the proposal rfc Proposal with request-for-comment sent out label Aug 12, 2022
Comment on lines +27 to +29
This statement is syntactic sugar for a set of other patterns, specifically it
guards a code block if a requirement of this code block is violated. That is,
there is actually no problem.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a problem, otherwise we should not pursue this proposal. I believe var/let-else solves a real problem, and it is important that this proposal articulate what that problem is. The problem has to do with making common programming tasks both readable and writable. The specific benefit of var/let-else is when the task has two cases:

  • One case which can be identified using a pattern, and the main body of the function following will be handling.
  • All other cases can be handled uniformly and briefly.

If code follows that pattern, then this construct has two advantages: it is concise and not going to introduce as much indenting as other constructs. Both of these help readability and writability. Consider:

match (my_container.LookUp(...)) {
  case .None => { return NotFound; }
  case .Some(result: String) => {
    very;
    long;
    block;
    of;
    code;
    return Found(processed_result);
  }
}

compared to:

let .Some(result: String) = my_container.LookUp(...)
    else { return NotFound; }
very;
long;
block;
of;
code;
return Found(processed_result);

The big question is whether this use case is important / common. And I think that comes down to looking at examples when you would use this construct. I think the "function returns an optional" situation is one common example where this comes up. Another example is when calling a function that returns an error that this function wants to transform into a different error. Other examples would be valuable to identify.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to search around for the "return early pattern" to find examples where this would be useful.

Comment on lines +90 to +92
This pattern is said to improve the readability of code, compared to an
equivalent `match` or `if` statement. It allows to keep code which handles
violated requirements next to the requirements.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationales in Carbon are to be written in terms of Carbon's stated goals and principles. The proposal template has some links, the one that I think is relevant to this proposal is: [Code that is easy to read, understand, and write](/docs/project/goals.md#code-that-is-easy-to-read-understand-and-write)

Comment on lines +96 to +99
- `var`/`let` ... `or {...}`
- `var`/`let` ... `?: {...}` a short-circuited ternary operator
- `var`/`let` ... `otherwise {...}`
- introducer syntax: `guard var`/`let` ... `else {...}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These alternatives should include the advantages and disadvantages of these options and a rationale for why we chose the proposal over the alternative. If you don't know, that is a good time to reach out to the community.

Comment on lines +52 to +53
This proposal adds this pattern as it seems to be rather popular and simplifies
common error-handling patterns.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to go deeper into this here.

@rscircus rscircus marked this pull request as draft September 10, 2022 06:28
@rscircus
Copy link
Contributor Author

Marking this as draft again. Thanks @josh11b for the thorough review.

@rscircus
Copy link
Contributor Author

So, maybe you can send me a screenshot of the discussion @josh11b mentioned in #1388 (comment) ? I'd really like to get #1871 out of its draft state and also close #1388 ...

From #2165

@rscircus
Copy link
Contributor Author

rscircus commented Sep 20, 2022

@PramodhTVK - feel free to continue on this one. I am redirecting my energy to other FLOSS work.

Edit: Waiting 1mo until deleting https://github.com/rscircus/carbon-lang containing the branch from this PR.

@enghitalo
Copy link

@PramodhTVK - feel free to continue on this one. I am redirecting my energy to other FLOSS work.

Edit: Waiting 1mo until deleting https://github.com/rscircus/carbon-lang containing the branch from this PR.

What is missing??? This is the best request for carbon error handling that I see. Very similar to v lang.

@zygoloid
Copy link
Contributor

zygoloid commented Sep 21, 2022

So, maybe you can send me a screenshot of the discussion @josh11b mentioned in #1388 (comment) ? I'd really like to get #1871 out of its draft state and also close #1388 ...

let-else-1
let-else-2

transfer statement as `break`, `continue`, `return`, or a function that does not
return.

[Rust](https://github.com/rust-lang/rfcs/blob/master/text/3137-let-else.md)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you instead add the actual pull so that our reviewers can feel the impression at that time?


## Details

TBD after enough C in this RFC.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC - Request For Comment

Hence C is Comment(s). Like yours. ;)

@josh11b
Copy link
Contributor

josh11b commented Nov 1, 2022

Should this still be a draft?

@josh11b
Copy link
Contributor

josh11b commented Nov 3, 2022

FYI, Rust has recently stabilized let...else, see Announcing Rust 1.65.0. On Hacker News, there are some comments saying why they are excited by this features 1, 2,

@rscircus
Copy link
Contributor Author

rscircus commented Nov 8, 2022

Hey @josh11b - thanks for having a look. I pulled out of working here and encouraged @PramodhTVK to continue after that time-consuming Discord mess and refocused, as it took too much time and I didn't receive any help from whatever side I tried. However, @zygoloid helped meanwhile, as far as I see. Thank you.

Then, you can find the summary of my last thoughts on this here. I'll be having a look at Rust. Thanks again.

@rscircus
Copy link
Contributor Author

rscircus commented Nov 8, 2022 via email

@github-actions
Copy link

github-actions bot commented Feb 7, 2023

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please comment or remove the inactive label.
This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

@github-actions github-actions bot added the inactive Issues and PRs which have been inactive for at least 90 days. label Feb 7, 2023
@github-actions
Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it. \n\n\n This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

@github-actions github-actions bot closed this Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Issues and PRs which have been inactive for at least 90 days. proposal rfc Proposal with request-for-comment sent out proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants