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

Conditional expressions #911

Merged
merged 22 commits into from
Jan 29, 2022
Merged

Conditional expressions #911

merged 22 commits into from
Jan 29, 2022

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Oct 22, 2021

This proposal introduces a conditional operator of the form:

if cond then value1 else value2

@zygoloid zygoloid added the proposal A proposal label Oct 22, 2021
@zygoloid zygoloid requested a review from a team October 22, 2021 00:41
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Oct 22, 2021
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
@zygoloid zygoloid marked this pull request as ready for review October 26, 2021 21:25
@zygoloid zygoloid requested a review from a team as a code owner October 26, 2021 21:25
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Oct 26, 2021
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

LGTM!

docs/design/expressions/if.md Show resolved Hide resolved
proposals/p0911.md Show resolved Hide resolved
proposals/p0911.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Show resolved Hide resolved
proposals/p0911.md Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
proposals/p0911.md Show resolved Hide resolved
proposals/p0911.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Reworked how we handle symmetrizing; there is now both a not-necessarily-symmetric interface and a guaranteed-symmetric constraint.

docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Show resolved Hide resolved
proposals/p0911.md Show resolved Hide resolved
proposals/p0911.md Show resolved Hide resolved
proposals/p0911.md Show resolved Hide resolved
proposals/p0911.md Outdated Show resolved Hide resolved
proposals/p0911.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
Copy link
Contributor

@josh11b josh11b left a comment

Choose a reason for hiding this comment

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

Here are a few preliminary comments. I'm going to look at this again in more detail since it looks a bit complicated at the moment, particularly the implicit conversion stuff.

docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
proposals/p0911.md Outdated Show resolved Hide resolved
zygoloid and others added 3 commits January 19, 2022 14:30
appear.

Don't allow `if` expressions as operands of any operator, as if they are
themselves a very low precedence operator.
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Couple of random drive-by comments and digging into one thread. Will try to do more thorough pass soon.

docs/design/expressions/if.md Outdated Show resolved Hide resolved
Comment on lines +57 to +60
The converted _condition_ is evaluated. If it evaluates to `true`, then the
converted _value1_ is evaluated and its value is the result of the expression.
Otherwise, the converted _value2_ is evaluated and its value is the result of
the expression.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the phrasing "[t]he converted expression is evaluated" a bit awkward or confusing... The conversion happens after the evaluation?

Maybe phrase this as _condition_ is evaluated and then implicitly converted to bool.... Dunno, its awkward. But I think it would be good to make it unambiguous that the conversion happens after evaluation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a distinction between operationally thinking of conversion as something you do to a value, and denotationally thinking of it as something you do to an expression. In the language of https://github.com/carbon-language/carbon-lang/tree/trunk/docs/design/expressions#conversions-and-casts, we take the latter view: implicit conversion is a process applied to an expression of one type to produce an expression of another type, as part of type-checking. As such, it happens before any evaluation is considered: first we transform cond into cond.(ImplicitAs(bool).Convert)() as part of type-checking, to form the converted condition expression, and then later whenever we evaluate the if expresssion, we evaluate that converted condition expression. (And similarly for the value1 and value2: type-checking converts those, and during evaluation we evaluate the converted version of whichever expression we select.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my challenge as a reader is that the phrase "a converted X is evaluated ..." is very ambiguous between "an X of the 'converted' kind is evaluated" and "after converting X, it is evaluated". They both seem quite plausible interpretations, and there are contexts in which either one would make more sense, and so I often end up confused which one is the one I should be using.

Maybe as a follow-up both here and in the "Conversions and Casts" section the language could switch to some construct that is less ambiguous when read if that makes sense (and such a formulation could be found)?

To be clear, this isn't blocking anything, just a lingering confusion for me as a reader.

docs/design/expressions/if.md Outdated Show resolved Hide resolved
Comment on lines 170 to 172
Another way of viewing this is that `if` is a very low-precedence form of
expression, and is lower precedence than is permitted in an expression-statement
-- that is, as the "operand" of a `;`.
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I find this more confusing than clarifying -- if has a higher precedence than parentheses and commas, so this perspective would imply that we don't allow tuple expressions or paren expressions to be expression-statements, which I don't think is (or should be) the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inside and outside of parentheses (and grammar constructs involving commas) will typically be different precedence levels, which I think is probably part of the confusion here. A parenthesized expression lives at a very high precedence level, so it can be used pretty much anywhere, but the context of the inside of it is a very low precedence context, so pretty much anything can appear within it. (I would predict that we won't have a C++-like comma operator, but I suppose only time will tell.)

But... given that this attempted clarification isn't helping, I'll just remove it.

- Don't specify how to convert the condition of an `if` expression; just
  defer to whatever `if` statements do.
- Remove attempted clarification of `if` expression positioning in terms
  of precedence, since it seems to be more confusing than helpful.
Given the current direction for #821, it doesn't seem to be necessary.
Copy link
Contributor

@josh11b josh11b left a comment

Choose a reason for hiding this comment

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

This looks good to me. I have a few small comments, but they are not blocking concerns, just small cleanups to the text.

docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Show resolved Hide resolved
proposals/p0911.md Show resolved Hide resolved
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

This is a very simple language feature that turns out (for good reasons) to be quite difficult and require an impressive amount of care and detail. Really amazing proposal walking through all of it and navigating the tradeoffs.

I've left a bunch if pending comments, but these is only one real change that I think is just clarifying something. The rest are totally optional readability tweaks. Happy for this to go in with these addressed or skipped, whatever makes sense. Thanks (and huge thanks to all the other reviews here to get this into such good shape).

Comment on lines +57 to +60
The converted _condition_ is evaluated. If it evaluates to `true`, then the
converted _value1_ is evaluated and its value is the result of the expression.
Otherwise, the converted _value2_ is evaluated and its value is the result of
the expression.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think my challenge as a reader is that the phrase "a converted X is evaluated ..." is very ambiguous between "an X of the 'converted' kind is evaluated" and "after converting X, it is evaluated". They both seem quite plausible interpretations, and there are contexts in which either one would make more sense, and so I often end up confused which one is the one I should be using.

Maybe as a follow-up both here and in the "Conversions and Casts" section the language could switch to some construct that is less ambiguous when read if that makes sense (and such a formulation could be found)?

To be clear, this isn't blocking anything, just a lingering confusion for me as a reader.

docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
docs/design/expressions/if.md Outdated Show resolved Hide resolved
@zygoloid zygoloid merged commit fcccb21 into carbon-language:trunk Jan 29, 2022
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Jan 29, 2022
zygoloid added a commit that referenced this pull request Mar 4, 2022
@zygoloid zygoloid deleted the proposal-conditional-expressi branch March 11, 2022 01:04
chandlerc added a commit that referenced this pull request Jun 28, 2022
This proposal introduces a conditional operator of the form:

```
if cond then value1 else value2
```

Co-authored-by: josh11b <[email protected]>
Co-authored-by: Chandler Carruth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants