Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

impl Hasher for {&mut Hasher, Box<Hasher>} #44015

Merged
merged 2 commits into from
Sep 13, 2017
Merged

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Aug 21, 2017

Rationale: The Hash trait has fn hash<H: Hasher>(&self, state: &mut H), which can only accept a Sized hasher, even if the Hasher trait is object-safe. We cannot retroactively add the ?Sized bound without breaking stability, thus implementing Hasher to a trait object reference is the next best solution.

Warning: These impl are insta-stable, and should need an FCP. I don't think a full RFC is necessary.

@bluss
Copy link
Member

bluss commented Aug 22, 2017

Just a head's up, probably not a big deal. New blanket impls for &T, &mut T, Box<T> are technically breaking for an existing trait. If both Foo and Box<Foo> implemented Hasher, that crate would now break. It's the effect of allowing the user's box implementation in the first place (the “fundamental rule”).

@kennytm
Copy link
Member Author

kennytm commented Aug 23, 2017

@bluss Oops, so crater is needed? The impl can be change to only applying on the trait objects &'a mut Hasher and Box<Hasher + 'a> if this turns out to be a problem.

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 23, 2017
@alexcrichton
Copy link
Member

cc @aidanhs

mind running cargobomb for this?

@kennytm
Copy link
Member Author

kennytm commented Aug 25, 2017

@bors try

@bors
Copy link
Contributor

bors commented Aug 25, 2017

⌛ Trying commit ccf6d90 with merge 9300482...

bors added a commit that referenced this pull request Aug 25, 2017
impl Hasher for {&mut Hasher, Box<Hasher>}

**Rationale:** The `Hash` trait has `fn hash<H: Hasher>(&self, state: &mut H)`, which can only accept a `Sized` hasher, even if the `Hasher` trait is object-safe. We cannot retroactively add the `?Sized` bound without breaking stability, thus implementing `Hasher` to a trait object reference is the next best solution.

**Warning:** These `impl` are insta-stable, and should need an FCP. I don't think a full RFC is necessary.
@bors
Copy link
Contributor

bors commented Aug 25, 2017

☀️ Test successful - status-travis
State: approved= try=True

@aidanhs
Copy link
Member

aidanhs commented Aug 25, 2017

Cargobomb run started

@aidanhs
Copy link
Member

aidanhs commented Aug 29, 2017

Cargobomb results: http://cargobomb-reports.s3.amazonaws.com/pr-44015/index.html

Blacklist (spurious failures etc) at https://github.com/rust-lang-nursery/cargobomb/blob/master/blacklist.md. If you see any spurious failures not on the list, please make a PR against that file.

@kennytm
Copy link
Member Author

kennytm commented Aug 29, 2017

14 regressions. All spurious.

Crate Status Cause
diesel_codegen_shared-0.8.0 Spurious diesel-rs/diesel#475
entity_rust-0.0.8 Spurious tinco/entity_rust#1
gcc-0.3.51 Spurious? Text file busy (os error 26)
gcc-0.3.53 No repro locally Failed to start ar and cc
lua53-ext-0.1.1 Spurious Jellonator/rust-lua53-ext#1
milagro-crypto-0.1.14 Spurious See (a)
mock_me-0.2.2 Spurious craftytrickster/mock_me#7
pokemon-go-protobuf-0.1.4 No repro locally rust-lang/crater#125
proxy_config-0.0.2 Spurious mattico/proxy-config#16
urdf-viz-0.1.2 Can't build locally
with any toolchain
C++ linker error??
vec-vp-tree-0.2.0-alpha.1 Spurious abonander/vec-vp-tree#1
vidar-0.1.0 Spurious aesir-vanir/vidar#1
kurnevsky.opai-rs.7c21e36 Spurious pointsgame/oppai-rs#5
vks.mpw-rs.d1ac69b No repro locally rust-lang/crater#125

Note (a): The test_random_mod_order test sometimes will segfault inside the BIG_mod C function, reason unknown. The segfault might be fixed by miracl/milagro-crypto-c@ebbe5acdce9d59086420d7da8b5df67dae014fe0 (the crate uses a very old version of the C library). The author of the crate does not have a repository to report the issue.

@arielb1 arielb1 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 Aug 29, 2017
@kennytm
Copy link
Member Author

kennytm commented Sep 2, 2017

@arielb1 Could you please change the tag back to S-waiting-for-review? The merge conflict has been fixed. Thanks.

@arielb1 arielb1 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 Sep 3, 2017
@alexcrichton
Copy link
Member

@bors: r+

Sorry for the delay but looks great!

@bors
Copy link
Contributor

bors commented Sep 7, 2017

📌 Commit 4f454db has been approved by alexcrichton

@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 7, 2017
@bors
Copy link
Contributor

bors commented Sep 8, 2017

⌛ Testing commit 4f454db9713c189405db8f3022ece88050e883ff with merge d14f2577ad399156609192de8d81c619fd48f54a...

@bors
Copy link
Contributor

bors commented Sep 8, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member Author

kennytm commented Sep 8, 2017

asmjs failed, legit, i128 is not supported on asm.js.

I'll check why this happens later.

[01:50:36] error: linking with `emcc` failed: exit code: 1
[01:50:36]   |
[01:50:36]   = note: "emcc" "-s" "DISABLE_EXCEPTION_CATCHING=0" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/asmjs-unknown-emscripten/release/deps/collectionstests-a051b7c76aca0939.0.o" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/asmjs-unknown-emscripten/release/deps/collectionstests-a051b7c76aca0939.js" "-s" "EXPORTED_FUNCTIONS=[\"_main\",\"___rdl_shrink_in_place\",\"___rdl_alloc_excess\",\"___rdl_usable_size\",\"___rdl_alloc\",\"___rdl_realloc_excess\",\"___rdl_realloc\",\"___rdl_oom\",\"___rdl_grow_in_place\",\"___rdl_alloc_zeroed\",\"___rdl_dealloc\",\"_rust_eh_personality\"]" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/asmjs-unknown-emscripten/release/deps/collectionstests-a051b7c76aca0939.crate.allocator.o" "-O2" "--memory-init-file" "0" "-g0" "-s" "DEFAULT_LIBRARY_FUNCS_TO_INCLUDE=[]" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/asmjs-unknown-emscripten/release/deps" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/release/deps" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/libtest-7174f69942758dfc.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/libterm-b41696b9a2882d36.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/libgetopts-b40550347d4814f5.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/libstd-5e8ebc384e5dfd82.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/libpanic_unwind-368647e9b9ca3b39.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/libunwind-eda6aedbad712bc0.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/liballoc_system-67992b04c9027e70.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/liblibc-0200a904d71d63a4.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/liballoc-c15d9e20191e711b.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/libstd_unicode-b3230a4723442795.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/librand-efc56d5e4c2b1ee8.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/libcore-3181dd9e46400ebd.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib/libcompiler_builtins-fa8533728d55e42b.rlib" "-l" "c" "-Wl,-rpath,$ORIGIN/../lib" "-s" "ERROR_ON_UNDEFINED_SYMBOLS=1"
[01:50:36]   = note: LLVM ERROR: Function _ZN4core4hash6Hasher10write_i12817h4a3f787e99e09ab3E has illegal integer argument
[01:50:36]           Traceback (most recent call last):
[01:50:36]             File "/emsdk-portable/emscripten/1.37.13//emcc", line 13, in <module>
[01:50:36]               emcc.run()
[01:50:36]             File "/emsdk-portable/emscripten/1.37.13/emcc.py", line 1526, in run
[01:50:36]               final = shared.Building.emscripten(final, append_ext=False, extra_args=extra_args)
[01:50:36]             File "/emsdk-portable/emscripten/1.37.13/tools/shared.py", line 1963, in emscripten
[01:50:36]               call_emscripten(cmdline)
[01:50:36]             File "/emsdk-portable/emscripten/1.37.13/emscripten.py", line 2190, in _main
[01:50:36]               temp_files.run_and_clean(lambda: main(
[01:50:36]             File "/emsdk-portable/emscripten/1.37.13/tools/tempfiles.py", line 78, in run_and_clean
[01:50:36]               return func()
[01:50:36]             File "/emsdk-portable/emscripten/1.37.13/emscripten.py", line 2195, in <lambda>
[01:50:36]               DEBUG=DEBUG,
[01:50:36]             File "/emsdk-portable/emscripten/1.37.13/emscripten.py", line 2095, in main
[01:50:36]               temp_files=temp_files, DEBUG=DEBUG)
[01:50:36]             File "/emsdk-portable/emscripten/1.37.13/emscripten.py", line 93, in emscript
[01:50:36]               backend_output = compile_js(infile, settings, temp_files, DEBUG)
[01:50:36]             File "/emsdk-portable/emscripten/1.37.13/emscripten.py", line 127, in compile_js
[01:50:36]               backend_output = open(temp_js).read()
[01:50:36]           IOError: [Errno 2] No such file or directory: '/tmp/tmppyyP2C.4.js'
[01:50:36]           
[01:50:36]
[01:50:36] error: aborting due to previous error
[01:50:36]
[01:50:36] error: Could not compile `alloc`.
[01:50:36] warning: build failed, waiting for other jobs to finish...
[01:51:11] error: build failed
[01:51:11]
[01:51:11]
[01:51:11] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "-j" "4" "--target" "asmjs-unknown-emscripten" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std:0.0.0" "-p" "unwind:0.0.0" "-p" "alloc:0.0.0" "-p" "alloc_system:0.0.0" "-p" "collections:0.0.0" "-p" "std_unicode:0.0.0" "-p" "panic_abort:0.0.0" "-p" "compiler_builtins:0.0.0" "-p" "rand:0.0.0" "-p" "core:0.0.0" "-p" "libc:0.0.0" "--"
[01:51:11] expected success, got: exit code: 101
[01:51:11]
[01:51:11]
[01:51:11] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target asmjs-unknown-emscripten
[01:51:11] Build completed unsuccessfully in 1:49:01

@kennytm kennytm changed the title impl Hasher for {&mut Hasher, Box<Hasher>} [WIP] impl Hasher for {&mut Hasher, Box<Hasher>} Sep 10, 2017
@kennytm kennytm force-pushed the hasher branch 2 times, most recently from e055eba to 4d9ec08 Compare September 10, 2017 18:50
@kennytm kennytm force-pushed the hasher branch 6 times, most recently from 8636ac2 to 70ecf4b Compare September 12, 2017 09:00
@kennytm kennytm changed the title [WIP] impl Hasher for {&mut Hasher, Box<Hasher>} impl Hasher for {&mut Hasher, Box<Hasher>} Sep 12, 2017
@kennytm
Copy link
Member Author

kennytm commented Sep 12, 2017

@alexcrichton Fixed by disabling the tests on Emscripten. Same issue as emscripten-core/emscripten-fastcomp#169. cc #35118.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 12, 2017

📌 Commit 143e2dc has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Sep 12, 2017

⌛ Testing commit 143e2dc with merge 011a41ff767b697e8b53ea5956e78108fd3a00de...

@bors
Copy link
Contributor

bors commented Sep 12, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member Author

kennytm commented Sep 12, 2017

@bors retry #44159

dist-x86_64-linux DEPLOY_ALT=1 failed to deploy

ERROR:  Could not find a valid gem 'aws-sdk-resources' (= 2.10.44) in any repository
ERROR:  Possible alternatives: aws-sdk-resources
/home/travis/.rvm/rubies/ruby-2.2.7/lib/ruby/site_ruby/2.2.0/rubygems/core_ext/kernel_require.rb:55:in `require': cannot load such file -- aws-sdk (LoadError)
	from /home/travis/.rvm/rubies/ruby-2.2.7/lib/ruby/site_ruby/2.2.0/rubygems/core_ext/kernel_require.rb:55:in `require'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider.rb:85:in `requires'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider/s3.rb:6:in `<class:S3>'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider/s3.rb:5:in `<class:Provider>'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider/s3.rb:4:in `<module:DPL>'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider/s3.rb:3:in `<top (required)>'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider.rb:59:in `const_get'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider.rb:59:in `block in new'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/cli.rb:41:in `fold'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/provider.rb:56:in `new'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/cli.rb:31:in `run'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/lib/dpl/cli.rb:7:in `run'
	from /home/travis/.rvm/gems/ruby-2.2.7/gems/dpl-1.8.43/bin/dpl:5:in `<top (required)>'
	from /home/travis/.rvm/gems/ruby-2.2.7/bin/dpl:23:in `load'
	from /home/travis/.rvm/gems/ruby-2.2.7/bin/dpl:23:in `<main>'

travis_fold:end:dpl.1
failed to deploy

@bors
Copy link
Contributor

bors commented Sep 12, 2017

⌛ Testing commit 143e2dc with merge 2fdccaf...

bors added a commit that referenced this pull request Sep 12, 2017
 impl Hasher for {&mut Hasher, Box<Hasher>}

**Rationale:** The `Hash` trait has `fn hash<H: Hasher>(&self, state: &mut H)`, which can only accept a `Sized` hasher, even if the `Hasher` trait is object-safe. We cannot retroactively add the `?Sized` bound without breaking stability, thus implementing `Hasher` to a trait object reference is the next best solution.

**Warning:** These `impl` are insta-stable, and should need an FCP. I don't think a full RFC is necessary.
@bors
Copy link
Contributor

bors commented Sep 13, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 2fdccaf to master...

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants