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

Document execution order #888

Merged
merged 7 commits into from
Jan 14, 2021
Merged

Document execution order #888

merged 7 commits into from
Jan 14, 2021

Conversation

Havvy
Copy link
Contributor

@Havvy Havvy commented Oct 4, 2020

Thank eddyb and Steve Klabnik for complaining on Twitter for this PR.

Closes #248

The order is now documented. I considered using an example over expression evaluation using println!, but the twitter thread is explicitly using iterators and this is probably the most common reason to care about evaluation order in tuple expressions anyways.

I also cleaned up the page to bring it into style compliance. I did both at the same time, so there's only one noisy commit. Sorry to the reviewer.

@Havvy Havvy added New Content Missing features or aspects of language not currently documented. A-expressions Area: Expressions labels Oct 4, 2020
@Havvy Havvy requested a review from ehuss October 4, 2020 06:13
@eddyb
Copy link
Member

eddyb commented Oct 4, 2020

Why document any one thing in particular? The order is universally "postorder" (on the AST), i.e. "more nested" expressions first (effectively mandated by "expressions depend on the result of their subexpressions"), and between siblings it's specifically left-to-right (outside of perhaps assignments' LHS? I forget how we resolved that one).

Can't we specify it in one place? Doing it on a case-by-case basis seems inefficient and would suggest the order isn't uniformly defined, whereas it is (otherwise e.g. the borrow-checker couldn't possibly work).

cc @rust-lang/lang

@Havvy
Copy link
Contributor Author

Havvy commented Oct 4, 2020

I thought about that after I went to bed as well. I could add an evaluation order section to expressions.md instead.

@Havvy
Copy link
Contributor Author

Havvy commented Oct 6, 2020

I've completely rewritten this PR to now document the default at the top level. I'll be doing the tuple changes as their own PR.

@ehuss
Copy link
Contributor

ehuss commented Oct 18, 2020

I don't think I know enough to review this. Is this an actual commitment the language can make? Does rustc actually enforce this? Doesn't LLVM do some amount of reordering? Can an implementation reorder expressions if it can prove it results in the same value and observable side-effects?

I would expect this to have some more precise language. Perhaps make it clear that this left-to-right ordering is referring to the operands of an expression, and this is not related operator precedence and associativity? (Or, at least, spell out the relationship with operator precedence and associativity?) I think "operands" is a little more specific than "subexpressions" or "inner expressions".

I'm also a little unclear on how this relates to execution order, which is sometimes defined relative to "sequence" or "critical execution" points.

@Havvy Havvy changed the title Document expression order for tuple expressions. Document execution order Oct 19, 2020
@Havvy
Copy link
Contributor Author

Havvy commented Oct 19, 2020

For reordering expressions: If there are no observable side effects done by doing so, then expressions can be reordered in their evaluation order. Heck, they can even be evaluated at compile time under certain conditions. But this is also true about the ordering of statements in a block expression. And yet, those aren't semantics of the language. They're optimizations in the implementation. Anywhere there are actual side effects, this becomes true.

With respect to "operands", I think "subexpressions" makes a more clear term. Most expressions contain inner expressions that are subsumed by that expression. Operand would have to be defined and outside of arithemetic operator expressions, you don't see the term used.

With respect to "operator precedence", I should add a note about that.

Evaluation order and execution order are basically synonymous to me. I don't think I've seen a distinction between the two terms in any literature I've read.

@ehuss
Copy link
Contributor

ehuss commented Oct 19, 2020

The reason I suggest "operand" is because for me, "sub-expression" can be ambiguous if it is recursive. Like, the "borrow expression" clearly (to me) has one operand, but has a limitless number of sub-expressions if you consider it recursively. Also, "operand" is already defined and used throughout expressions.md and operator-expr.md, and it seems a little more direct to me. Just my opinion, though, I don't think it matters too much.

I guess I just feel a little uneasy to have an hard rule of "left to right", when that's not universally true if optimizers are allowed to reorder based on some unwritten rules. Like C++17 very clearly defines ordering with respect to value computation and side effects. And Java very clearly expresses the order and caveats on when it can be rewritten. Many other specs just vaguely mention "left-to-right" (like Go and Javascript and D and C#), so it is not unusual, but it seems strange to me to be so imprecise in a spec.

I think it is interesting that C# used to have an ordering based on well-defined side effects, but that seems to have been removed and it only mentions left-to-right ordering now.

@Havvy
Copy link
Contributor Author

Havvy commented Oct 26, 2020

See the commit message.

Since operand is used already, I'll continue to use it. Didn't actually see it before.

What optimizer do is something that we don't concern ourselves with in the reference. It can do much worse things to the semantics of the language already by eliding copies and movies and function calls or replacing function calls with a constant instead. Yes, it can reorder if it doesn't change the ultimate effect. Nothing in the reference is universal in the face of an optimizer.

@Havvy
Copy link
Contributor Author

Havvy commented Nov 5, 2020

@ehuss Do you still feel unqualified to review this?

@ehuss
Copy link
Contributor

ehuss commented Nov 9, 2020

I would be more comfortable if someone from the language team reviewed this. It looks good to me, and it does follow the convention used in several other languages as noted above. I'm just uncertain if that's the most appropriate way to define the order. Otherwise I'm fine with it.

@Havvy Havvy added the S-needs-reviewer The pull request poses questions that need to be answered by non-maintainers of the reference label Nov 9, 2020
@RalfJung
Copy link
Member

RalfJung commented Nov 9, 2020

I guess I just feel a little uneasy to have an hard rule of "left to right", when that's not universally true if optimizers are allowed to reorder based on some unwritten rules.

It is universally true though. Optimizations are only allowed to reorder things in a way that causes no change of observable behavior (the "as-if" rule). The rules for what optimizations are allowed to do are not unwritten, they are "you must only do things where the resulting program only behaves in ways that the original program could already behave as" -- this is the universal correctness condition for all compilers of all programming languages.

That's just like how the spec says that x*2 results in the value of x being multiplied by 2, but optimizations can still change this to something else, like x << 1, or remove the computation entirely if the result is unused (and no overflow occurred). And yet the spec will just talk about multiplication and not about how the compiler could choose to implement that multiplication.

It is the responsibility of optimizations to justify their correctness against the spec, and not vice versa.

@RalfJung
Copy link
Member

RalfJung commented Nov 9, 2020

Nothing in the reference is universal in the face of an optimizer.

That would be horrible! Everything in the reference is universal for all correct optimizers, when considering the observable effects of running a Rust program. Don't think in terms of assembly, think of the resulting program as a black box that you can only probe by running it, feeding it input and observing its output. Everything that happens that way must conform to what the reference says.

The reference says nothing about the inside of that black box, but that does not at all mean that the reference is any less universal. The inside of that black box is simply out of scope, just like, e.g., the exact formatting of error messages.

@@ -85,6 +84,33 @@ in the order given by their associativity.
| `=` `+=` `-=` `*=` `/=` `%=` <br> `&=` <code>&#124;=</code> `^=` `<<=` `>>=` | right to left |
| `return` `break` closures | |

## Evaluation order of operands

Unless otherwise stated on the expression's page, evaluation of the operands of
Copy link
Member

Choose a reason for hiding this comment

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

Can we cross-reference any cases where this isn't true from within this section, to not leave people wondering "what expressions don't evaluate their sub-expressions left-to-right"?

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 listed every case where it is true. I was being lazy with that sentence prior. I'm still being lazy by not actually linking to those expression pages, but that can always be done later and they're in the sidebar.

Copy link
Contributor

Choose a reason for hiding this comment

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

One prominent case that is not mentioned here is a = b. Our execution odrer there, I believe, is to evaluate b before a, though I want to double check now =)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I seem to recall @eddyb saying that it actually is even more complicated than that, but I forgot the details...

@joshtriplett
Copy link
Member

A couple of minor nits, but otherwise, this looks great. With my lang team hat on (though not speaking for the rest of the lang team), 👍 to documenting "left to right" as the semantics.

@nikomatsakis nikomatsakis self-assigned this Nov 11, 2020
Havvy added a commit to Havvy/reference that referenced this pull request Nov 12, 2020
Borrowing language from Ralf Jung from a comment on PR 888
(rust-lang#888 (comment))
describe the relationship of optimizers and what the reference states.
Havvy added a commit to Havvy/reference that referenced this pull request Nov 12, 2020
Borrowing language from Ralf Jung from a comment on PR 888
(rust-lang#888 (comment))
describe the relationship of optimizers and what the reference states.
@Havvy Havvy added S-waiting-on-review Status: The marked PR is awaiting review from the PR author. and removed S-needs-reviewer The pull request poses questions that need to be answered by non-maintainers of the reference labels Nov 17, 2020

> **Note**: Since this is applied recursively, expressions are also evaluated
> from innermost to outermost, ignoring siblings until there are no inner
> subexpressions.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to extend this PR with coverage of

  • assignment expressions
  • the order of "augmented assignment" like +=, particularly in the face of operator overloading -- this gets a bit subtle, see e.g. Settle execution order uncertainty for += rust#28160 -- I think at this point it's clear though that we just have to document the current behavior and live with it

How much did you test to verify the execution order for various corner cases? (e.g., overloaded index?) I think that what you wrote above is correct, but I'd love to see a comprehensive set of test cases added to the repository, e.g., in a place like src/test/ui/evaluation_order.rs =) this doesn't all have to fall on you, of course, we could just write to write up a list of those tests and see if we can advertise folks to write them up

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 haven't done any testing of the corner cases. Although I did just make one for Index. But given that for all of these operations, the actual method call happens after the operands are evaluated, I can't see execution order changing by using language known impls or trait impls.

I also am not sure a single evaluation_order.rs test file is the best place. I would think you'd want src/test/ui/exprs/%expr_kind%/evaluation_order.rs so that it's grouped with other tests of the expression kind. But of course, that'd require quite a lot of reorganization of the tests and ultimately all of this falls outside of the domain of the reference docs.

I also just did a quick test for assignment operators, and they apparently evaluate the right expression before the left expression. That I did not know, so that does require updating this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Havvy yeah I totally agree that a "directory" is the right place to sort those kinds of tests.

@Havvy
Copy link
Contributor Author

Havvy commented Nov 20, 2020

I need to still do compound assignment expressions, but I'm getting tired. Since I was modifying assignment expressions, I figured I'd give it the same treatment I gave tuples (and would like to give to all expressions).

Image of rendered change

image

@Havvy Havvy added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: The marked PR is awaiting review from the PR author. labels Nov 21, 2020
@nikomatsakis
Copy link
Contributor

@Havvy this is looking good, are you still expecting to make more additions here, or would you rather land for now?

@Havvy
Copy link
Contributor Author

Havvy commented Dec 14, 2020

I keep putting off finishing up the section on compound assignment; lack of energy after work or last week a patch dropped in an MMO. If you want to merge as is, I can send that section as its own PR.

@Havvy
Copy link
Contributor Author

Havvy commented Dec 27, 2020

Given that there will probably be needed discussion on compound assignment operators, I wish to land that part separately from this.

@Havvy Havvy added S-waiting-on-review Status: The marked PR is awaiting review from the PR author. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jan 1, 2021
@joshtriplett
Copy link
Member

One minor clarification, and otherwise, r=me.

I've italicized "operand" showing that it is a definition. I didn't
actually remember that being added to the reference, so I basically
tried to redefine it in the previous commit. I don't like the term, but
since it's already there, I'll just use it.

I also put in a note saying that operator precedence determines the
operands of an expression. That section could probably be written in a
style that better expresses that perspective, but I'm trying to keep
this change minimal.

I also stated that the evaluation of operands is done prior to applying
the effect. This goes in line with the beginning of the chapter with
what the meaning of an expression.

Note also that this only describes the default. Expressions that deviate
from the default already should describe their evaluation order.
This commit does not try to define their evaluation order, they just
removed them from the list of shared LTR evaluated expressions.
I'm being wish-washy with expression/operand in the syntax section. I'm
not quite sure if we should call them `place operands` everywhere.
It could have been read that it was any field, not just uninitialized fields.

Co-authored-by: Josh Triplett <josh@joshtriplett.org>
@Havvy Havvy merged commit 20d9fe6 into rust-lang:master Jan 14, 2021
@Havvy Havvy deleted the exe-order branch January 14, 2021 06:27
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 26, 2021
Update books

## nomicon

7 commits in a8584998eacdea7106a1dfafcbf6c1c06fcdf925..bbf06ad39d1f45654047e9596b750cc6e6d1b693
2021-01-06 12:49:49 -0500 to 2021-01-22 07:07:31 -0800
- Fix alloc link in exotic-sizes for local docs (rust-lang/nomicon#255)
- Remove TODO
- Fix small punctuation error
- Arc revisions (Clone atomic explanation) (pt2/3(+?))
- Fix Arc Clone
- Arc revisions (pt1/2(+?))
- Simple Arc implementation (without Weak refs)

## reference

5 commits in 50af691f838937c300b47812d0507c6d88c14f97..f02b09eb6e8af340ad1256a54adb7aae2ff3163e
2021-01-12 21:19:20 -0800 to 2021-01-22 01:53:02 -0800
- Fix missing space (rust-lang/reference#941)
- Start documenting name resolution. (rust-lang/reference#937)
- Fix plural and delete spurious words in comparison ops (rust-lang/reference#932)
- Document execution order (rust-lang/reference#888)
- Compound operator expressions (rust-lang/reference#915)

## book

3 commits in ac57a0ddd23d173b26731ccf939f3ba729753275..e724bd826580ff95df48a8533af7dec1080693d4
2021-01-09 14:18:45 -0500 to 2021-01-20 08:19:49 -0600
- Fixes rust-lang/book#2417. Get the index from user input instead of a const. (rust-lang/book#2566)
- Turn off the playground in a bunch more lib.rs inclusions (rust-lang/book#2569)
- Merge pull request rust-lang/book#2567 from rust-lang/rust-1.49

## rust-by-example

1 commits in 03e23af01f0b4f83a3a513da280e1ca92587f2ec..f633769acef68574427a6fae6c06f13bc2199573
2021-01-09 10:20:28 -0300 to 2021-01-13 20:58:25 -0300
- Fixed styling on closure example (rust-lang/rust-by-example#1405)
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 26, 2021
Update books

## nomicon

7 commits in a8584998eacdea7106a1dfafcbf6c1c06fcdf925..bbf06ad39d1f45654047e9596b750cc6e6d1b693
2021-01-06 12:49:49 -0500 to 2021-01-22 07:07:31 -0800
- Fix alloc link in exotic-sizes for local docs (rust-lang/nomicon#255)
- Remove TODO
- Fix small punctuation error
- Arc revisions (Clone atomic explanation) (pt2/3(+?))
- Fix Arc Clone
- Arc revisions (pt1/2(+?))
- Simple Arc implementation (without Weak refs)

## reference

5 commits in 50af691f838937c300b47812d0507c6d88c14f97..f02b09eb6e8af340ad1256a54adb7aae2ff3163e
2021-01-12 21:19:20 -0800 to 2021-01-22 01:53:02 -0800
- Fix missing space (rust-lang/reference#941)
- Start documenting name resolution. (rust-lang/reference#937)
- Fix plural and delete spurious words in comparison ops (rust-lang/reference#932)
- Document execution order (rust-lang/reference#888)
- Compound operator expressions (rust-lang/reference#915)

## book

3 commits in ac57a0ddd23d173b26731ccf939f3ba729753275..e724bd826580ff95df48a8533af7dec1080693d4
2021-01-09 14:18:45 -0500 to 2021-01-20 08:19:49 -0600
- Fixes rust-lang/book#2417. Get the index from user input instead of a const. (rust-lang/book#2566)
- Turn off the playground in a bunch more lib.rs inclusions (rust-lang/book#2569)
- Merge pull request rust-lang/book#2567 from rust-lang/rust-1.49

## rust-by-example

1 commits in 03e23af01f0b4f83a3a513da280e1ca92587f2ec..f633769acef68574427a6fae6c06f13bc2199573
2021-01-09 10:20:28 -0300 to 2021-01-13 20:58:25 -0300
- Fixed styling on closure example (rust-lang/rust-by-example#1405)
psychon added a commit to psychon/x11rb that referenced this pull request Feb 3, 2024
winit uses atom_manager! with 59 atom names. Due to some sub-optimal
code generation, this causes the generated new() function to end up with
a size of 111.6 KiB, or almost 5% of the .text size of a winit example.

The issue is that each "intern_atom()?" can cause an early exit and
needs to call drop() of the already generated Cookies. This leads to
some quadratic growth.

To work around this, in this commit I change the implementation of
new(). It now iterates over the atom names, using .map() to send the
InternAtom requests. Finally, collect() is then used to change this
Iterator<Item=Result<Cookie, Error>> into Result<Vec<Cookie>, Error>.
This even preserves the early-exit behaviour in case of errors.

The reply() function is then changed to turn the Vec into an iterator,
using next() to pull out the items. This relies on the evaluation of
this code to be well-defined (which e.g. wouldn't be guaranteed in C).
Luckily, Rust provides the needed guarantee:
rust-lang/reference#888

Fixes: #912
Signed-off-by: Uli Schlachter <psychon@znc.in>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-expressions Area: Expressions New Content Missing features or aspects of language not currently documented. S-waiting-on-review Status: The marked PR is awaiting review from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document expression evaluation order
6 participants