Assignment statements #2511

zygoloid · 2023-01-04T22:43:19Z

Assignment is permitted only as a complete statement, not as a subexpression.
Assignment and compound assignment syntax follow C++ in all other respects.
Pre-increment is provided. Post-increment is not. Uses of all of these operators
are translated into calls to interface members.

docs/design/assignment.md

chandlerc

Some quick initial thoughts...

chandlerc · 2023-01-06T04:07:20Z

docs/design/README.md

@@ -1223,25 +1222,6 @@ fn Foo() {
 > -   Proposal
 >     [#162: Basic Syntax](https://github.com/carbon-language/carbon-lang/pull/162)

-### Assignment statements


Any reason to not keep at least a brief overview here?

We have a very brief overview on line 253-260. I didn't feel like this section pulled its weight compared to that very brief overview and the detailed description in the separate page.

FWIW, I still would find it helpful, although its not a big deal. Maybe @josh11b has a thought one way or the other?

Assuming we don't keep it, I think then the section above or up one level should be adjusted to at least have a list of the different kinds of statements and links to their dedicated page. Otherwise, folks dropping in this section from a link to that section can't really navigate to all the relevant parts of the design.

@josh11b to maybe provide guidance on the right amount of content and structure here.

FWIW, the only thing I'm really looking for here is some reasonably discoverable link to the dedicated document. Totally happy with however much or little detail accompanies it here and whatever structure works best to incorporate it. =]

I do think there should be something inside the previous section about assignment statements if this section is going to be removed, like around the sentence starting with "Expressions."

Thanks, that makes sense to me. Done.

docs/design/assignment.md

proposals/p2511.md

jonmeow · 2023-01-06T16:25:16Z

proposals/p2511.md

+
+## Alternatives considered
+
+### Allow assignment as a subexpression


I will note particular concerns with this for increment and decrement. e.g., absl's flat_hash_map erase relies on postincrement as a subexpression. This could make particular challenges for migrating code, as either adding additional locals (since map.erase(it); ++it; is potentially different) or creating new APIs becomes a prerequisite for migration, likely reducing automated migration.

But it's not clear to me that you're looking for this kind of concern at this time, so I'll mostly suggest that if you're still considering this, "alternatives considered" indicates that it has been ruled out; "open questions" might be more appropriate? I think the difference here is whether the plan is "look more" versus "wait and see if there are too many complaints"; if it's the latter, I would agree with "alternatives considered", but would also want more consideration for how we migrate existing C++ code on the assumption that this isn't reconsidered.

Thought: for migration, maybe we just transform C++ it++ into something like [&]() -> auto { var x: auto = it; ++it; return x; }() (something like that can become a general solution for subexpressions, although maybe not ideal for readers)

Thanks, I've added some brief discussion here giving the rationale for rejecting this alternative, which was previously missing, and also describing possibilities for getting the same effect (eg, for migration) and some potential limitations when doing so.

What do you think about having a distinct alternative for increment and decrement? It can I think be much more brief, but I feel like there are some unique aspects to those, both in terms of the impact and potentially more limited pitfalls. And so it might be clearer to treat those separately.

Minor point either way though -- there is a fundamental tradeoff here in that this will create some unknown amount of migration friction, and it'll be experience with tooling and actual migrations that teach us whether we end up want to revisit parts of this or not.

Just to be explicit, I'm fine with the resolution here, although apprehensive about migration costs.

docs/design/assignment.md

jonmeow · 2023-01-06T16:43:28Z

proposals/p2511.md

+In leads issue [#451](https://github.com/carbon-language/carbon-lang/issues/451)
+it was decided that we would initially not support chained assignment.


Should the rationale be provided here so that leads issues aren't a canonical reference for decisions?

Perhaps more importantly, what was the rationale? What trade-offs did the leads consider? On a skim, I see a decision, but without further context.

Added some words, but honestly I'm not sure we had much rationale here other than that we didn't see sufficient motivation to add this feature compared to the cost.

A lack of justification is still important to document. :)

proposals/p2511.md

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

chandlerc

Just noting a few things here orthogonal to the naming discussion...

chandlerc · 2023-01-14T04:04:04Z

docs/design/README.md

@@ -1223,25 +1222,6 @@ fn Foo() {
 > -   Proposal
 >     [#162: Basic Syntax](https://github.com/carbon-language/carbon-lang/pull/162)

-### Assignment statements


FWIW, I still would find it helpful, although its not a big deal. Maybe @josh11b has a thought one way or the other?

Assuming we don't keep it, I think then the section above or up one level should be adjusted to at least have a list of the different kinds of statements and links to their dedicated page. Otherwise, folks dropping in this section from a link to that section can't really navigate to all the relevant parts of the design.

proposals/p2511.md

chandlerc · 2023-01-14T10:08:42Z

proposals/p2511.md

+
+## Alternatives considered
+
+### Allow assignment as a subexpression


What do you think about having a distinct alternative for increment and decrement? It can I think be much more brief, but I feel like there are some unique aspects to those, both in terms of the impact and potentially more limited pitfalls. And so it might be clearer to treat those separately.

Minor point either way though -- there is a fundamental tradeoff here in that this will create some unknown amount of migration friction, and it'll be experience with tooling and actual migrations that teach us whether we end up want to revisit parts of this or not.

jonmeow

LGTM overall.

proposals/p2511.md

jonmeow · 2023-01-17T18:40:57Z

proposals/p2511.md

+
+## Alternatives considered
+
+### Allow assignment as a subexpression


Just to be explicit, I'm fine with the resolution here, although apprehensive about migration costs.

jonmeow · 2023-01-17T18:46:36Z

proposals/p2511.md

+In leads issue [#451](https://github.com/carbon-language/carbon-lang/issues/451)
+it was decided that we would initially not support chained assignment.


A lack of justification is still important to document. :)

Co-authored-by: Jon Ross-Perkins <jperkins@google.com> Co-authored-by: Chandler Carruth <chandlerc@gmail.com>

chandlerc

Definitely converging. Mostly suggesting some added information to cover a bit of discussion that we had around extensibility of increment and decrement to try and more fully document the principles that underpin the design and the rationale for those.

I also think we need at least some fixes to the top level overview.

chandlerc · 2023-01-19T01:46:58Z

docs/design/README.md

@@ -1223,25 +1222,6 @@ fn Foo() {
 > -   Proposal
 >     [#162: Basic Syntax](https://github.com/carbon-language/carbon-lang/pull/162)

-### Assignment statements


@josh11b to maybe provide guidance on the right amount of content and structure here.

FWIW, the only thing I'm really looking for here is some reasonably discoverable link to the dedicated document. Totally happy with however much or little detail accompanies it here and whatever structure works best to incorporate it. =]

docs/design/assignment.md

proposals/p2511.md

statements to the list of things that can appear in a block.

Add alternative considered for providing increment automatically whenever we can add 1.

Add support for user-defined assignment, as well as compound assignment and increment, following the design direction in pending proposal #2511. Some of this isn't fully testable yet: because explorer doesn't properly support `impl` specialization, the blanket `impl`s in the prelude prevent types from customizing assignment.

chandlerc

LGTM! I spotted a tiny wording nit just as I was skimming it, but seems trivial to fix forward if the suggestion isn't quite right. =D

chandlerc

(sorry for not including this tweak...)

docs/design/assignment.md

…rences

With this change `docs/design/lexical_conventions/symbolic_tokens.md` should now include all operators and symbols from accepted proposals.

zygoloid added proposal A proposal proposal draft Proposal in draft, not ready for review labels Jan 4, 2023

Filling out template with PR 2511

c1be5e4

zygoloid force-pushed the proposal-assignment-statement branch from 497c90c to c1be5e4 Compare January 4, 2023 22:43

zygoloid added 7 commits January 5, 2023 16:04

Proposal.

ed1f566

Add alternatives considered to design.

7f09fe4

Remove period from heading

1c446b3

Fix code formatting

861d574

Fix typo in rewrite rule.

6d6f55f

Make indentation consistent.

ec5ccda

Remove period for consistency

51181ee

zygoloid marked this pull request as ready for review January 6, 2023 00:15

github-actions bot added proposal rfc Proposal with request-for-comment sent out and removed proposal draft Proposal in draft, not ready for review labels Jan 6, 2023

github-actions bot requested a review from chandlerc January 6, 2023 00:15

carsonRadtke reviewed Jan 6, 2023

View reviewed changes

docs/design/assignment.md Show resolved Hide resolved

zygoloid commented Jan 6, 2023

View reviewed changes

docs/design/assignment.md Outdated Show resolved Hide resolved

Add use of $= to match introduction of $.

7fc7d20

chandlerc reviewed Jan 6, 2023

View reviewed changes

jonmeow reviewed Jan 6, 2023

View reviewed changes

zygoloid and others added 3 commits January 6, 2023 14:20

Fix typo.

dbd9b37

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

Address review comments. Rename interfaces.

90fdd30

Fix constraint to put OpWith in scope before we use it.

55d877d

zygoloid mentioned this pull request Jan 14, 2023

Add support for compound assignment and increment #2526

Merged

chandlerc reviewed Jan 14, 2023

View reviewed changes

jonmeow reviewed Jan 17, 2023

View reviewed changes

zygoloid and others added 4 commits January 17, 2023 14:13

Apply suggestions from code review

082c87b

Co-authored-by: Jon Ross-Perkins <jperkins@google.com> Co-authored-by: Chandler Carruth <chandlerc@gmail.com>

AssignOp -> OpAssign

1abe930

Fix typos.

ea1d2f1

Trying bold instead of italics.

b4a1e32

zygoloid added 3 commits January 17, 2023 18:08

Bold+italic.

3166879

Fix

563dd0b

We have no lvalue conditionals; remove suggestion that we do.

227a62f

chandlerc reviewed Jan 19, 2023

View reviewed changes

zygoloid added 2 commits January 19, 2023 12:34

Add example of increment in the tour of the language. Add assignment

b79a867

statements to the list of things that can appear in a block.

Add writeup of intended compound assignment and increment semantics.

424ea1b

Add alternative considered for providing increment automatically whenever we can add 1.

chandlerc self-requested a review January 27, 2023 01:12

zygoloid requested a review from jonmeow January 27, 2023 01:19

chandlerc approved these changes Jan 27, 2023

View reviewed changes

chandlerc approved these changes Jan 28, 2023

View reviewed changes

docs/design/assignment.md Outdated Show resolved Hide resolved

Minor wording tweak.

70f0c8a

zygoloid commented Jan 28, 2023

View reviewed changes

docs/design/assignment.md Outdated Show resolved Hide resolved

Appease prettier.

77db955

zygoloid merged commit a89aba2 into carbon-language:trunk Jan 28, 2023

josh11b mentioned this pull request May 24, 2023

Added info on symbolic tokens in design docs #2657

Merged

josh11b mentioned this pull request Jun 2, 2023

Operation order in struct/class assignment/initialization #686

Closed

josh11b added a commit to josh11b/carbon-lang that referenced this pull request Jun 2, 2023

Follow-up to carbon-language#2657: include carbon-language#2511, refe…

0d86000

…rences

zygoloid pushed a commit that referenced this pull request Jun 2, 2023

Follow-up to #2657: include #2511, references (#2870)

684168f

With this change `docs/design/lexical_conventions/symbolic_tokens.md` should now include all operators and symbols from accepted proposals.

jonmeow mentioned this pull request Aug 24, 2023

Take value representation into account when lowering functions and calls. #3149

Merged

zygoloid deleted the proposal-assignment-statement branch October 4, 2023 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assignment statements #2511

Assignment statements #2511

zygoloid commented Jan 4, 2023 •

edited

chandlerc left a comment

chandlerc Jan 6, 2023

zygoloid Jan 6, 2023

chandlerc Jan 14, 2023

chandlerc Jan 19, 2023

josh11b Jan 19, 2023

zygoloid Jan 19, 2023 •

edited

jonmeow Jan 6, 2023

jonmeow Jan 6, 2023 •

edited

zygoloid Jan 10, 2023

chandlerc Jan 14, 2023

jonmeow Jan 17, 2023

jonmeow Jan 6, 2023

zygoloid Jan 10, 2023

jonmeow Jan 17, 2023

chandlerc left a comment

chandlerc Jan 14, 2023

chandlerc Jan 14, 2023

jonmeow left a comment

jonmeow Jan 17, 2023

jonmeow Jan 17, 2023

chandlerc left a comment

chandlerc Jan 19, 2023

chandlerc left a comment

chandlerc left a comment


		## Alternatives considered

		### Allow assignment as a subexpression

		In leads issue [#451](https://github.com/carbon-language/carbon-lang/issues/451)
		it was decided that we would initially not support chained assignment.

Assignment statements #2511

Assignment statements #2511

Conversation

zygoloid commented Jan 4, 2023 • edited

chandlerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygoloid Jan 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow Jan 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

zygoloid commented Jan 4, 2023 •

edited

zygoloid Jan 19, 2023 •

edited

jonmeow Jan 6, 2023 •

edited