Drop partially bound function parameters in the expected order by matthewjasper · Pull Request #56044 · rust-lang/rust

matthewjasper · 2018-11-18T15:17:00Z

Given the function

fnfoo((_x, _):(LogDrop,LogDrop),(_, _y):(LogDrop,LogDrop)){}

Prior to 1.12.0 we dropped both _x and _y before the rest of their
respective parameters, since then we dropped _x and _y after. The
original order appears to be the correct order, as the value created
later is dropped first, so we revert to that order and add a test for
it.

While this is technically a breaking change, I can't work out how
anyone could be relying on this without making their code very
brittle. If this is considered to be too likely to break real world code
then I can revert the change and change the test to check for the
current order.

rust-highfive · 2018-11-18T15:17:04Z

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2018-11-18T15:21:43Z

Given fn foo((_x, _): (LogDrop, LogDrop), (_, _y): (LogDrop, LogDrop)) {} I would expect the drop order to be arg1.1, arg2.0, _y, and _x because the _ arguments should be dropped immediately and then the other arguments are dropped in reverse order of declaration per http://rust-lang.github.io/rfcs/1857-stabilize-drop-order.html.

matthewjasper · 2018-11-18T15:32:17Z

Things bound to _ in arguments have never been dropped immediately (it's wrong for patterns using ref).

RalfJung · 2018-11-24T22:28:55Z

I would have expected fn foo(PAT: T) { ... } to be the same as fn foo(_tmp: T) { let PAT = _tmp; ... }. So I agree with @Mark-Simulacrum, dropping immediately would be most intuitive, and everything else seems like an odd exception.

cramertj · 2018-11-26T18:23:10Z

@bors r+

bors · 2018-11-26T18:23:11Z

📌 Commit 7dc0dd2 has been approved by cramertj

bors · 2018-11-27T06:34:21Z

⌛ Testing commit 7dc0dd2 with merge ee2f6f9...

…mertj Drop partially bound function parameters in the expected order Given the function ```rust fn foo((_x, _): (LogDrop, LogDrop), (_, _y): (LogDrop, LogDrop)) {} ``` Prior to 1.12.0 we dropped both `_x` and `_y` before the rest of their respective parameters, since then we dropped `_x` and `_y` after. The original order appears to be the correct order, as the value created later is dropped first, so we revert to that order and add a test for it. While this is technically a breaking change, I can't work out how anyone could be relying on this without making their code very brittle. If this is considered to be too likely to break real world code then I can revert the change and change the test to check for the current order.

bors · 2018-11-27T07:42:11Z

💔 Test failed - status-travis

rust-highfive · 2018-11-27T07:42:12Z

The job wasm32-unknown of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:05:17] [01:05:17] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:503:22
[01:05:17] error: test run failed!
[01:05:17] status: exit code: 101
[01:05:17] command: "/node-v9.2.0-linux-x64/bin/node" "/checkout/src/etc/wasm32-shim.js" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/binding/fn-arg-incomplete-pattern-drop-order/a.wasm"
[01:05:17] ------------------------------------------
[01:05:17] [01:05:17] ------------------------------------------
[01:05:17] stderr:
[01:05:17] stderr:
[01:05:17] ------------------------------------------
[01:05:17] RuntimeError: unreachable
[01:05:17] at __rust_start_panic (wasm-function[82]:1)
[01:05:17] at rust_panic (wasm-function[78]:39)
[01:05:17] at _ZN3std9panicking20rust_panic_with_hook17hefa20f9e2d7144fcE (wasm-function[73]:346)
[01:05:17] at _ZN3std9panicking11begin_panic17ha07741734e150442E (wasm-function[11]:49)
[01:05:17] at _ZN36fn_arg_incomplete_pattern_drop_order3foo17hb9cbcbcdc62d43caE (wasm-function[2]:607)
[01:05:17] at _ZN3std9panicking3try7do_call17hbbbd02620233e1d7E.llvm.12228513419448781796 (wasm-function[12]:348)
[01:05:17] at __rust_maybe_catch_panic (wasm-function[81]:5)
[01:05:17] at _ZN36fn_arg_incomplete_pattern_drop_order4main17h78f606c69bb2c1c8E (wasm-function[3]:295)
[01:05:17] at _ZN3std2rt10lang_start28_$u7b$$u7b$closure$u7d$$u7d$17h898ef31c452dd768E (wasm-function[6]:25)
[01:05:17] at _ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17hd27616b21d0538aaE (wasm-function[62]:8)
[01:05:17] at _ZN3std9panicking3try7do_call17hfb625b14a19c314fE (wasm-function[70]:20)
[01:05:17] at __rust_maybe_catch_panic (wasm-function[81]:5)
[01:05:17] at _ZN3std2rt19lang_start_internal17h89e1dbf50853ba79E (wasm-function[79]:270)
[01:05:17] at _ZN3std2rt10lang_start17hbfa996f5e8ab0c4dE (wasm-function[5]:42)
[01:05:17] at main (wasm-function[4]:11)
[01:05:17] at Object.<anonymous> (/checkout/src/etc/wasm32-shim.js:136:20)
[01:05:17] at Module._compile (module.js:641:30)
[01:05:17] at Object.Module._extensions..js (module.js:652:10)
[01:05:17] at Module.load (module.js:560:32)
[01:05:17] at tryModuleLoad (module.js:503:12)
[01:05:17] ------------------------------------------
[01:05:17] [01:05:17] thread '[run-pass] run-pass/binding/fn-arg-incomplete-pattern-drop-order.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3282:9
[01:05:17] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[01:05:17] test result: FAILED. 2636 passed; 1 failed; 251 ignored; 0 measured; 0 filtered out
[01:05:17] [01:05:17] [01:05:17] [01:05:17] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/wasm32-unknown-unknown/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-wasm32-unknown-unknown" "--mode" "run-pass" "--target" "wasm32-unknown-unknown" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--nodejs" "/node-v9.2.0-linux-x64/bin/node" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options -Lnative=/checkout/obj/build/wasm32-unknown-unknown/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--llvm-version" "8.0.0svn\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:05:17] [01:05:17] [01:05:17] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target wasm32-unknown-unknown src/test/run-make src/test/ui src/test/run-pass src/test/compile-fail src/test/mir-opt src/test/codegen-units src/libcore
[01:05:17] Build completed unsuccessfully in 1:02:53
---
travis_time:end:0b8a2e24:start=1543304505070614849,finish=1543304505077540370,duration=6925521
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:02cf9286
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:07de15a9
travis_time:start:07de15a9
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:258e1492
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

…rder, r=cramertj Drop partially bound function parameters in the expected order Given the function ```rust fn foo((_x, _): (LogDrop, LogDrop), (_, _y): (LogDrop, LogDrop)) {} ``` Prior to 1.12.0 we dropped both `_x` and `_y` before the rest of their respective parameters, since then we dropped `_x` and `_y` after. The original order appears to be the correct order, as the value created later is dropped first, so we revert to that order and add a test for it. While this is technically a breaking change, I can't work out how anyone could be relying on this without making their code very brittle. If this is considered to be too likely to break real world code then I can revert the change and change the test to check for the current order.

pnkfelix · 2018-11-29T13:54:42Z

I find the uses of the word "immediately" confusing in the comments above by @Mark-Simulacrum and @RalfJung

Can we clarify if that is supposed to mean "immediately" at the end of the execution of foo? or immediately at the start of foo? Better still, maybe just write a example where foo itself prints a message to make the order absolultely concrete?

RalfJung · 2018-11-29T14:40:06Z

@pnkfelix I stated my expected semantics as an equivalence with let bindings, as I care more about consistency of drop order than about when things are dropped. These two functions should do exactly the same (assuming ... does not refer to _tmp):

fnfoo(PAT:T){ ...}fnfoo(_tmp:T){letPAT = _tmp; ...}

I think this means they should be dropped at the beginning of foo? I must also be missing something else, or this is a native speaker thing, because I cannot imagine an interpretation of "immediate" that's not "dropped immediately", i.e., as soon as possible, i.e., when the function starts. What is "immediate" about dropping them at the end?

Mark-Simulacrum · 2018-11-29T18:42:04Z

I would expect this:

fnfoo((_x, _):(LogDrop,LogDrop),(_, _y):(LogDrop,LogDrop)){// for naming:// fn foo((a, b): (LogDrop, LogDrop), (c, d): (LogDrop, LogDrop)) {// variables dropped "immediately," before function runs;// technically I think c should drop first to preserve "reverse of declaration order"// c dropped// b dropped// function runs// variables dropped in reverse of declaration order:// d dropped// a dropped}

matthewjasper · 2018-11-29T19:43:57Z

fnfoo(PAT:T){ ...}fnfoo(_tmp:T){letPAT = _tmp; ...}

These currently have different drop order: playground. This PR makes them have the same drop order.

scottmcm · 2018-11-29T20:39:18Z

+ }
+}
+
+fn foo((_x, _): (LogDrop, LogDrop), (_, _y): (LogDrop, LogDrop)) {}


Could this test also have the let version, to explicitly demonstrate that they do the same thing?
Like
fnfoo2(_temp1:(LogDrop,LogDrop),_temp2:(LogDrop,LogDrop)){let(_x, _) = _temp1;let(_, _y) = _temp2;}

joshtriplett · 2018-11-29T20:41:24Z

Consensus in the lang team meeting was that patterns in the function arguments should work exactly like the equivalent let bindings at the top of the function, in terms of drop order. So, fn foo((_x, _): (LogDrop, LogDrop), (_, _y): (LogDrop, LogDrop)) {} should drop in the same order and at the same time as:

fnfoo(a:(LogDrop,LogDrop),b:(LogDrop,LogDrop)){let(_x, _) = a;let(_, _y) = b;// ...}

Given the function fn foo((_x, _): (LogDrop, LogDrop), (_, _y): (LogDrop, LogDrop)) {} Prior to 1.12 we dropped both `_x` and `_y` before the rest of their respective parameters, since then we dropped `_x` and `_y` after. The original order appears to be the correct order, as the value created later is dropped first, so we revert to that order and add a test for it.

Dylan-DPC-zz · 2019-01-14T10:51:19Z

ping from triage @cramertj waiting for your review on this

cramertj · 2019-01-14T18:05:23Z

@matthewjasper

fn foo(a: (LogDrop, LogDrop), b: (LogDrop, LogDrop)) {
let (_x, ) = a;
let (, _y) = b;
// ...
}
This is possible, but would be even more of a change.

Sorry for the delayed response-- can you explain how this is different from your current test?

fnbindings_with_let(a:(LogDrop,LogDrop),b:(LogDrop,LogDrop)){// Drop order in foo is the same as the following bindings.// _temp2 is declared after _x to avoid a difference between `_: T` and// `x: T` in function parameters.let _temp1 = a;let(_x, _) = _temp1;let _temp2 = b;let(_, _y) = _temp2;}

The difference I see is the introduction of _temp variables, but I don't understand what functional effect those have in this code.

matthewjasper · 2019-01-14T19:42:25Z

The difference is that _temp2 is dropped earlier in the version in this PR. The equivalent of this

fn foo(a:(LogDrop,LogDrop),b:(LogDrop,LogDrop)){let(_x,) = a;let(, _y) = b;

would be

let _temp1 = a;// _temp2 created before `_x`, so it's dropped afterlet _temp2 = b;let(_x,) = a;let(, _y) = b;

cramertj · 2019-01-14T20:01:01Z

@matthewjasper Thanks for clarifying! I think I was getting confused because in this particular example (where we're destructuring both and the tuple types don't implement Drop) the behavior is the same (right?).

The change is whether or not destructuring happens paired with temporary assignment for each argument one-at-a-time, or whether temporary assignment happens first for all arguments, followed by destructuring for all. I think it makes sense to do a full argument-at-a-time so long as this is documented clearly. Thanks for explaining!

@bors r+

bors · 2019-01-14T20:01:02Z

📌 Commit 914515f has been approved by cramertj

bors · 2019-01-15T03:34:00Z

⌛ Testing commit 914515f with merge 768ccc6972a8c875e519b95311f5e86c675c45c6...

bors · 2019-01-15T04:06:21Z

💔 Test failed - checks-travis

rust-highfive · 2019-01-15T04:06:22Z

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_fold:start:worker_info
Worker information
hostname: c17cd80a-0ae3-4622-a8bf-b4dbd914c122@1.production-2-worker-com-gce-ptcp
version: v6.2.0 https://github.com/travis-ci/worker/tree/5e5476e01646095f48eec13196fdb3faf8f5cbf7
instance: travis-job-72ee5c78-8973-4fa0-9ed6-3bb070f8e85d travis-ci-stevonnie-xenial-1547455581-2c98a19 (via amqp)
startup: 6.792550546s
travis_fold:end:worker_info
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

kennytm · 2019-01-15T09:32:31Z

@bors retry

…rder, r=cramertj Drop partially bound function parameters in the expected order Given the function ```rust fn foo((_x, _): (LogDrop, LogDrop), (_, _y): (LogDrop, LogDrop)) {} ``` Prior to 1.12.0 we dropped both `_x` and `_y` before the rest of their respective parameters, since then we dropped `_x` and `_y` after. The original order appears to be the correct order, as the value created later is dropped first, so we revert to that order and add a test for it. While this is technically a breaking change, I can't work out how anyone could be relying on this without making their code very brittle. If this is considered to be too likely to break real world code then I can revert the change and change the test to check for the current order.

@ghost

Rollup of 8 pull requests Successful merges: - #56044 (Drop partially bound function parameters in the expected order) - #57352 (forbid manually impl'ing one of an object type's marker traits) - #57456 (RawVec doesn't always abort on allocation errors) - #57467 (Implement `check_attribute` to forbid `#[allow_internal_unsafe]`) - #57579 (Add core::iter::once_with()) - #57587 (Add 'rustc-env:RUST_BACKTRACE=0' to const-pat-ice test) - #57608 (Simplify 'product' factorial example) - #57614 ([rustdoc] Fix crates filtering box not being filled) Failed merges: r? @ghost

rust-highfive assigned cramertj Nov 18, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2018

Centril added I-nominated T-lang Relevant to the language team labels Nov 19, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 27, 2018

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2018

emilyalbini mentioned this pull request Nov 28, 2018
Rollup of 29 pull requests #56325
Closed

Centril removed the I-nominated label Nov 29, 2018

scottmcm reviewed Nov 29, 2018
View reviewed changes

This comment has been minimized.
Sign in to view

matthewjasper force-pushed the function-param-drop-order branch from 5459338 to 914515fCompare November 30, 2018 19:44

XAMPPRocky added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 10, 2018

Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2019

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 14, 2019

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 15, 2019

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2019

Centril mentioned this pull request Jan 15, 2019
Rollup of 8 pull requests #57630
Merged

bors merged commit 914515f into rust-lang:masterJan 15, 2019

matthewjasper deleted the function-param-drop-order branch February 3, 2019 08:41

kennytm mentioned this pull request Feb 22, 2019
Spurious 30 minutes timeout on Travis CI #56112
Closed

arielb1 mentioned this pull request Feb 28, 2019
Parameter patterns are dropped *after* arguments #36088
Closed

Uh oh!

Uh oh!

Conversation

matthewjasper commented Nov 18, 2018

Uh oh!

rust-highfive commented Nov 18, 2018

Uh oh!

Mark-Simulacrum commented Nov 18, 2018

Uh oh!

matthewjasper commented Nov 18, 2018

Uh oh!

RalfJung commented Nov 24, 2018

Uh oh!

cramertj commented Nov 26, 2018

Uh oh!

bors commented Nov 26, 2018

Uh oh!

bors commented Nov 27, 2018

Uh oh!

bors commented Nov 27, 2018

Uh oh!

rust-highfive commented Nov 27, 2018

Uh oh!

pnkfelix commented Nov 29, 2018

Uh oh!

RalfJung commented Nov 29, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mark-Simulacrum commented Nov 29, 2018

Uh oh!

matthewjasper commented Nov 29, 2018

Uh oh!

scottmcmNov 29, 2018

Choose a reason for hiding this comment

Uh oh!

joshtriplett commented Nov 29, 2018

Uh oh!

This comment has been minimized.

Dylan-DPC-zz commented Jan 14, 2019

Uh oh!

cramertj commented Jan 14, 2019

Uh oh!

matthewjasper commented Jan 14, 2019

Uh oh!

cramertj commented Jan 14, 2019

Uh oh!

bors commented Jan 14, 2019

Uh oh!

bors commented Jan 15, 2019

Uh oh!

bors commented Jan 15, 2019

Uh oh!

rust-highfive commented Jan 15, 2019

Uh oh!

kennytm commented Jan 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

RalfJung commented Nov 29, 2018•
edited
Loading