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

Make Rc and Arc #[fundamental] #56955

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@cramertj
Copy link
Member

cramertj commented Dec 18, 2018

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 18, 2018

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.

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)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 18, 2018

As a baseline I'd want to be sure to run crater to see what happen, hopefully that breakage is just limited to specialization!

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 18, 2018

If I understand https://github.com/rust-lang/rfcs/blob/master/text/1023-rebalancing-coherence.md#errors-from-cargo-and-the-fundamental-attribute correctly, this means that we can never extend the set of traits implemented for Rc<T> or Arc<T> generically? (Unless that impl is added for a new trait in the same release cycle as the trait is stabilized?)

  • Hash
  • DispatchFromDyn<Rc<U>>
  • Eq
  • Ord
  • Borrow<T>
  • CoerceUnsized<Rc<U>>
  • Display
  • From<T>
  • From<Box<T>>
  • AsRef<T>
  • PartialOrd
  • Clone
  • Debug
  • PartialEq
  • Deref
  • Default
  • Drop
  • fmt::Pointer
  • Unpin
  • UnwindSafe

(The above is for Rc, I’ll assume Arc is similar enough for the purpose of this discussion.)

Does this also affect more specific impls, like for Rc<[T]> or Rc<str>?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 18, 2018

This means that we can never extend the set of traits implemented for Rc<T> or Arc<T> generically

I believe that is correct. Someone may have a impl OldTrait for Rc<MyType> already and you can't be sure (or they may have something where the negative reasoning is more subtle -- e.g., impl NewTrait for Rc<MyType> and impl<T: OldTrait> NewTrait for T.

I wonder: @cramertj, do you have specific examples of impls you wanted to write but couldn't? Maybe there is a more limited formulation of #[fundamental] we might consider? (Probably not, but it'd be interesting to see said examples regardless)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 18, 2018

I do tend to agree with @cramertj that it is hard to justify a discrepancy beween Box and Rc/Arc here, apart from "why commit to more than we have to".

@cramertj cramertj changed the title Make Rc and Arc #[fundamenatal] Make Rc and Arc #[fundamental] Dec 18, 2018

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 18, 2018

do you have specific examples of impls you wanted to write but couldn't?

They're basically all of the form impl othercrate::Trait for Rc<MyType> { ... }.

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 19, 2018

The job x86_64-gnu-llvm-6.0 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.
travis_time:end:1c643dbd:start=1545175885275744852,finish=1545175942557492207,duration=57281747355
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:46:55] .................................................................................................... 2200/5187
[00:46:58] .................................................................................................... 2300/5187
[00:47:02] .................................................................................................... 2400/5187
[00:47:06] .................................................................................................... 2500/5187
[00:47:09] ..................................................................F................................. 2600/5187
[00:47:17] .................................................................................................... 2800/5187
[00:47:20] .................................................................................................... 2900/5187
[00:47:24] .................................................................................................... 3000/5187
[00:47:27] ............................................................................................i....... 3100/5187
---
[00:48:39] 
[00:48:39] ---- [ui] ui/issues/issue-41974.rs stdout ----
[00:48:39] diff of stderr:
[00:48:39] 
[00:48:39] - error[E0119]: conflicting implementations of trait `std::ops::Drop` for type `std::boxed::Box<_>`:
[00:48:39] + error[E0119]: conflicting implementations of trait `std::ops::Drop` for type `std::sync::Arc<_>`:
[00:48:39] 3    |
[00:48:39] 3    |
[00:48:39] 4 LL | impl<T> Drop for T where T: A { //~ ERROR E0119
[00:48:39] 5    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:48:39] 6    |
[00:48:39] 7    = note: conflicting implementation in crate `alloc`:
[00:48:39] 7    = note: conflicting implementation in crate `alloc`:
[00:48:39] -            - impl<T> std::ops::Drop for std::boxed::Box<T>
[00:48:39] +            - impl<T> std::ops::Drop for std::sync::Arc<T>
[00:48:39] 9              where T: ?Sized;
[00:48:39] -    = note: downstream crates may implement trait `A` for type `std::boxed::Box<_>`
[00:48:39] +    = note: downstream crates may implement trait `A` for type `std::sync::Arc<_>`
[00:48:39] 11 
[00:48:39] 12 error[E0120]: the Drop trait may only be implemented on structures
[00:48:39] 
[00:48:39] 
[00:48:39] The actual stderr differed from the expected stderr.
[00:48:39] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-41974/issue-41974.stderr
[00:48:39] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-41974/issue-41974.stderr
[00:48:39] To update references, rerun the tests and pass the `--bless` flag
[00:48:39] To only update this specific test, also pass `--test-args issues/issue-41974.rs`
[00:48:39] error: 1 errors occurred comparing output.
[00:48:39] status: exit code: 1
[00:48:39] status: exit code: 1
[00:48:39] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/issues/issue-41974.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-41974/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-41974/auxiliary" "-A" "unused"
[00:48:39] ------------------------------------------
[00:48:39] 
[00:48:39] ------------------------------------------
[00:48:39] stderr:
[00:48:39] stderr:
[00:48:39] ------------------------------------------
[00:48:39] {"message":"conflicting implementations of trait `std::ops::Drop` for type `std::sync::Arc<_>`:","code":{"code":"E0119","explanation":"\nThere are conflicting trait implementations for the same type.\nExample of erroneous code:\n\n```compile_fail,E0119\ntrait MyTrait {\n    fn get(&self) -> usize;\n}\n\nimpl<T> MyTrait for T {\n    fn get(&self) -> usize { 0 }\n}\n\nstruct Foo {\n    value: usize\n}\n\nimpl MyTrait for Foo { // error: conflicting implementations of trait\n                       //        `MyTrait` for type `Foo`\n    fn get(&self) -> usize { self.value }\n}\n```\n\nWhen looking for the implementation for the trait, the compiler finds\nboth the `impl<T> MyTrait for T` where T is all types and the `impl\nMyTrait for Foo`. Since a trait cannot be implemented multiple times,\nthis is an error. So, when you write:\n\n```\ntrait MyTrait {\n    fn get(&self) -> usize;\n}\n\nimpl<T> MyTrait for T {\n    fn get(&self) -> usize { 0 }\n}\n```\n\nThis makes the trait implemented on all types in the scope. So if you\ntry to implement it on another one after that, the implementations will\nconflict. Example:\n\n```\ntrait MyTrait {\n    fn get(&self) -> usize;\n}\n\nimpl<T> MyTrait for T {\n    fn get(&self) -> usize { 0 }\n}\n\nstruct Foo;\n\nfn main() {\n    let f = Foo;\n\n    f.get(); // the trait is implemented so we can use it\n}\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/issues/issue-41974.rs","byte_start":518,"byte_end":547,"line_start":17,"line_end":17,"column_start":1,"column_end":30,"is_primary":true,"text":[{"text":"impl<T> Drop for T where T: A { //~ ERROR E0119","highlight_start":1,"highlight_end":30}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"conflicting implementation in crate `alloc`:\n- impl<T> std::ops::Drop for std::sync::Arc<T>\n  where T: ?Sized;","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"downstream crates may implement trait `A` for type `std::sync::Arc<_>`","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0119]: conflicting implementations of trait `std::ops::Drop` for type `std::sync::Arc<_>`:\n  --> /checkout/src/test/ui/issues/issue-41974.rs:17:1\n   |\nLL | impl<T> Drop for T where T: A { //~ ERROR E0119\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n   |\n   = note: conflicting implementation in crate `alloc`:\n           - impl<T> std::ops::Drop for std::sync::Arc<T>\n             where T: ?Sized;\n   = note: downstream crates may implement trait `A` for type `std::sync::Arc<_>`\n\n"}
[00:48:39] {"message":"the Drop trait may only be implemented on structures","code":{"code":"E0120","explanation":"\nAn attempt was made to implement Drop on a trait, which is not allowed: only\nstructs and enums can implement Drop. An example causing this error:\n\n```compile_fail,E0120\ntrait MyTrait {}\n\nimpl Drop for MyTrait {\n    fn drop(&mut self) {}\n}\n```\n\nA workaround for this problem is to wrap the trait up in a struct, and implement\nDrop on that. An example is shown below:\n\n```\ntrait MyTrait {}\nstruct MyWrapper<T: MyTrait> { foo: T }\n\nimpl <T: MyTrait> Drop for MyWrapper<T> {\n    fn drop(&mut self) {}\n}\n\n```\n\nAlternatively, wrapping trait objects requires something like the following:\n\n```\ntrait MyTrait {}\n\n//or Box<MyTrait>, if you wanted an owned trait object\nstruct MyWrapper<'a> { foo: &'a MyTrait }\n\nimpl <'a> Drop for MyWrapper<'a> {\n    fn drop(&mut self) {}\n}\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/issues/issue-41974.rs","byte_start":535,"byte_end":536,"line_start":17,"line_end":17,"column_start":18,"column_end":19,"is_primary":true,"text":[{"text":"impl<T> Drop for T where T: A { //~ ERROR E0119","highlight_start":18,"highlight_end":19}],"label":"implementing Drop requires a struct","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0120]: the Drop trait may only be implemented on structures\n  --> /checkout/src/test/ui/issues/issue-41974.rs:17:18\n   |\nLL | impl<T> Drop for T where T: A { //~ ERROR E0119\n   |                  ^ implementing Drop requires a struct\n\n"}
[00:48:39] {"message":"type parameter `T` must be used as the type parameter for some local type (e.g., `MyStruct<T>`)","code":{"code":"E0210","explanation":"\nThis error indicates a violation of one of Rust's orphan rules for trait\nimplementations. The rule concerns the use of type parameters in an\nimplementation of a foreign trait (a trait defined in another crate), and\nstates that type parameters must be \"covered\" by a local type. To understand\nwhat this means, it is perhaps easiest to consider a few examples.\n\nIf `ForeignTrait` is a trait defined in some external crate `foo`, then the\nfollowing trait `impl` is an error:\n\n```compile_fail,E0210\n# #[cfg(for_demonstration_only)]\nextern crate foo;\n# #[cfg(for_demonstration_only)]\nuse foo::ForeignTrait;\n# use std::panic::UnwindSafe as ForeignTrait;\n\nimpl<T> ForeignTrait for T { } // error\n# fn main() {}\n```\n\nTo work around this, it can be covered with a local type, `MyType`:\n\n```\n# use std::panic::UnwindSafe as ForeignTrait;\nstruct MyType<T>(T);\nimpl<T> ForeignTrait for MyType<T> { } // Ok\n```\n\nPlease note that a type alias is not sufficient.\n\nFor another example of an error, suppose there's another trait defined in `foo`\nnamed `ForeignTrait2` that takes two type parameters. Then this `impl` results\nin the same rule violation:\n\n```ignore (cannot-doctest-multicrate-project)\nstruct MyType2;\nimpl<T> ForeignTrait2<T, MyType<T>> for MyType2 { } // error\n```\n\nThe reason for this is that there are two appearances of type parameter `T` in\nthe `impl` header, both as parameters for `ForeignTrait2`. The first appearance\nis uncovered, and so runs afoul of the orphan rule.\n\nConsider one more example:\n\n```ignore (cannot-doctest-multicrate-project)\nimpl<T> ForeignTrait2<MyType<T>, T> for MyType2 { } // Ok\n```\n\nThis only differs from the previous `impl` in that the parameters `T` and\n`MyType<T>` for `ForeignTrait2` have been swapped. This example does *not*\nviolate the orphan rule; it is permitted.\n\nTo see why that last example was allowed, you need to understand the general\nrule. Unfortunately this rule is a bit tricky to state. Consider an `impl`:\n\n```ignore (only-for-syntax-highlight)\nimpl<P1, ..., Pm> ForeignTrait<T1, ..., Tn> for T0 { ... }\n```\n\nwhere `P1, ..., Pm` are the type parameters of the `impl` and `T0, ..., Tn`\nare types. One of the types `T0, ..., Tn` must be a local type (this is another\norphan rule, see the explanation for E0117). Let `i` be the smallest integer\nsuch that `Ti` is a local type. Then no type parameter can appear in any of the\n`Tj` for `j < i`.\n\nFor information on the design of the orphan rules, see [RFC 1023].\n\n[RFC 1023]: https://github.com/rust-lang/rfcs/blob/master/text/1023-rebalancing-coherence.md\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/issues/issue-41974.rs","byte_start":518,"byte_end":547,"line_start":17,"line_end":17,"column_start":1,"column_end":30,"is_primary":true,"text":[{"text":"impl<T> Drop for T where T: A { //~ ERROR E0119","highlight_start":1,"highlight_end":30}],"label":"type parameter `T` must be used as the type parameter for some local type","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"only traits defined in the current crate can be implemented for a type parameter","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0210]: type parameter `T` must be used as the type parameter for some local type (e.g., `MyStruct<T>`)\n  --> /checkout/src/test/ui/issues/issue-41974.rs:17:1\n   |\nLL | impl<T> Drop for T where T: A { //~ ERROR E0119\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type parameter `T` must be used as the type parameter for some local type\n   |\n   = note: only traits defined in the current crate can be implemented for a type parameter\n\n"}
[00:48:39] {"message":"aborting due to 3 previous errors","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to 3 previous errors\n\n"}
[00:48:39] {"message":"Some errors occurred: E0119, E0120, E0210.","code":null,"level":"","spans":[],"children":[],"rendered":"Some errors occurred: E0119, E0120, E0210.\n"}
[00:48:39] {"message":"For more information about an error, try `rustc --explain E0119`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about an error, try `rustc --explain E0119`.\n"}
[00:48:39] ------------------------------------------
[00:48:39] 
[00:48:39] thread '[ui] ui/issues/issue-41974.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3252:9
[00:48:39] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:48:39] 
[00:48:39] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:503:22
[00:48:39] 
[00:48:39] 
[00:48:39] 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/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:48:39] 
[00:48:39] 
[00:48:39] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:48:39] Build completed unsuccessfully in 0:03:55
[00:48:39] Build completed unsuccessfully in 0:03:55
[00:48:39] make: *** [check] Error 1
[00:48:39] Makefile:58: recipe for target 'check' failed
124324 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu
124320 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release
118652 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps
115344 ./src/llvm/test/CodeGen
---
travis_time:end:3284e09b:start=1545178872154075995,finish=1545178872160617996,duration=6542001
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:12aab3a4
$ 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:2735e02c
travis_time:start:2735e02c
$ 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:16a8e9e0
$ 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)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 19, 2018

@bors: try

bors added a commit that referenced this pull request Dec 19, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 19, 2018

⌛️ Trying commit 69e465a with merge e70472f...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 19, 2018

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

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 19, 2018

@craterbot run start=master#d99a320cba42f661aebfa1293b7b2ec3603dda75 end=try#e70472fc501b855c9080f3772432c10fe20b8866 mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Dec 19, 2018

👌 Experiment pr-56955 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Dec 19, 2018

🚧 Experiment pr-56955 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Dec 20, 2018

🎉 Experiment pr-56955 is completed!
📊 1650 regressed and 0 fixed (50551 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 20, 2018

Spot checking a few of those they look legitimate, unless that's a compiler but I don't think we can do this change :(

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 20, 2018

Ugh. Previously, we allowed negative reasoning that relied on #[fundamental] SomeUpstreamTrait not being implemented by Arc<T>. Making Arc<T> #[fundamental] makes it possible to implement a fundamental upstream trait for Arc<MyType>, making overlap now possible (interestingly, this also means that we can't ever implement Fn<..> for Arc<T> where T: Fn<...>, which seems extremely unfortunate).

example of error:

[INFO] [stderr] error[E0119]: conflicting implementations of trait `NewService` for type `std::sync::Arc<_>`:
[INFO] [stderr]    --> /opt/crater/cargo-home/registry/src/github.com-1ecc6299db9ec823/tower-service-0.1.0/src/lib.rs:279:1
[INFO] [stderr]     |
[INFO] [stderr] 262 | / impl<F, R, E, S> NewService for F
[INFO] [stderr] 263 | |     where F: Fn() -> R,
[INFO] [stderr] 264 | |           R: IntoFuture<Item = S, Error = E>,
[INFO] [stderr] 265 | |           S: Service,
[INFO] [stderr] ...   |
[INFO] [stderr] 276 | |     }
[INFO] [stderr] 277 | | }
[INFO] [stderr]     | |_- first implementation here
[INFO] [stderr] 278 | 
[INFO] [stderr] 279 |   impl<S: NewService + ?Sized> NewService for Arc<S> {
[INFO] [stderr]     |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `std::sync::Arc<_>`
[INFO] [stderr]     |
[INFO] [stderr]     = note: downstream crates may implement trait `std::ops::FnOnce<()>` for type `std::sync::Arc<_>`
[INFO] [stderr]     = note: downstream crates may implement trait `std::ops::Fn<()>` for type `std::sync::Arc<_>`
[INFO] [stderr] 
@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 20, 2018

I vaguely remember this situation in tokio-service, but I don't remember the exact details. Maybe there's an alternative fundamental-like rule that makes this work?

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 20, 2018

It's actually incoherent if a user can write impl FnOnce<...> for Arc<MyType>, esp. if MyType already implements FnOnce<...> with the appropriate requirements. In that case, without even referencing NewService trait it's possible to have conflicting impls of NewService.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 20, 2018

@cramertj correct, but what about a variant of fundamental that doesnt include impls of fundamental traits? so users just cant impl the fn traits for arcs and rcs, which is probably fine since you cant implement those traits on stable anyway and the established pattern is to write these blanket impls instead

since I don't think any fundamental traits are stable to implement its possible we could just change the meaning of the fundamental attribute in this way without breaking any stable code..

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 20, 2018

Yeah, we could introduce #[fundamental_but_not_for_fundamental] or something and add that to Rc and Arc, which would get you all the custom impls you want for #[fundamental] types that aren't for #[fundamental] traits.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 20, 2018

That would make it a breaking change to add the fundamental attribute to any existing trait, though!

Not sure what the next step in the process should be, since some change along these lines would be necessary before moving forward with this PR.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 20, 2018

Adding #[fundamental] to a trait being breaking doesn't seem unreasonable to me, but it is unfortunate.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 20, 2018

I feel like maybe the answer is that we should think about designing a "better solution" to the fundamental problem -- or perhaps it's best to broaden the scope a bit to coherence more generally. (I would also not be surprised to learn that adding fundamental to existing traits may be breaking, at least in the absence of new features.)

In general, I feel like there is a need to be able to "publicly affirm" that certain impls will never be added. For example, I'd like to be able to declare definitely that Box<T> will never be Copy. =) (Right now, external crates cannot rely on that.) In this case, one could imagine saying that (e.g.) Arc<T> will never be Fn for any type T.

(But of course this leaves open the question of how to say things like "Arc<T> will never implement any other existing trait" and so forth, which is basically what #[fundamental] is saying.)

I feel like as we move towards Chalk, we have the tools in place to talk carefully about these questions (e.g., we now have a model of coherence in Chalk and it was kind of illuminating). But it's not a small design challenge :( and maybe it is not sufficient priority to think about.

@Dylan-DPC

This comment has been minimized.

Copy link
Member

Dylan-DPC commented Jan 21, 2019

ping from triage @cramertj @alexcrichton @nikomatsakis any updates on this?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 21, 2019

This can't make progress unless we also change how fundamental types work in some way. With the current fundamental semantics, this PR is a breaking change.

@Dylan-DPC

This comment has been minimized.

Copy link
Member

Dylan-DPC commented Jan 21, 2019

@withoutboats thanks. Marking this as blocked.

@Dylan-DPC Dylan-DPC added the S-blocked label Jan 21, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 22, 2019

I'm going to just close this PR. Feel free to re-open @cramertj if you think there's value in having it open.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 22, 2019

Nah, closing is fine-- I'll just continue to sulk, but more quietly now, knowing that there's a reason we can't have nice things ;)

@cramertj cramertj deleted the cramertj:fundamental-arc branch Jan 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment