-
Notifications
You must be signed in to change notification settings - Fork 505
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
Conversation
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 |
I thought about that after I went to bed as well. I could add an evaluation order section to |
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. |
I don't think I know enough to review this. Is this an actual commitment the language can make? Does 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. |
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. |
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 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. |
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. |
@ehuss Do you still feel unqualified to review this? |
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. |
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 It is the responsibility of optimizations to justify their correctness against the spec, and not vice versa. |
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. |
src/expressions.md
Outdated
@@ -85,6 +84,33 @@ in the order given by their associativity. | |||
| `=` `+=` `-=` `*=` `/=` `%=` <br> `&=` <code>|=</code> `^=` `<<=` `>>=` | right to left | | |||
| `return` `break` closures | | | |||
|
|||
## Evaluation order of operands | |||
|
|||
Unless otherwise stated on the expression's page, evaluation of the operands of |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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...
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. |
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.
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.
src/expressions.md
Outdated
|
||
> **Note**: Since this is applied recursively, expressions are also evaluated | ||
> from innermost to outermost, ignoring siblings until there are no inner | ||
> subexpressions. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 this is looking good, are you still expecting to make more additions here, or would you rather land for now? |
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. |
Given that there will probably be needed discussion on compound assignment operators, I wish to land that part separately from this. |
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>
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)
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)
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>
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.