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

Generate builtin impls for Clone #43690

Merged
merged 8 commits into from
Aug 22, 2017
Merged

Generate builtin impls for Clone #43690

merged 8 commits into from
Aug 22, 2017

Conversation

scalexm
Copy link
Member

@scalexm scalexm commented Aug 5, 2017

This fixes a long-standing ICE and limitation where some builtin types implement Copy but not Clone (whereas Clone is a super trait of Copy).

However, this PR has a few side-effects:

  • Clone is now marked as a lang item.
  • [T; N] is now Clone if T: Clone (currently, only if T: Copy and for N <= 32).
  • fn foo<'a>() where &'a mut (): Clone { } won't compile anymore because of how bounds for builtin traits are handled (e.g. same thing currently if you replace Clone by Copy in this example). Of course this function is unusable anyway, an error would pop as soon as it is called.

Hence, I'm wondering wether this PR would need an RFC...
Also, cc-ing @nikomatsakis, @arielb1.

Related issues: #28229, #24000.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@eddyb
Copy link
Member

eddyb commented Aug 5, 2017

IMO it does need an RFC and I am against this specific implementation atm, although I am pleasantly surprised it's possible at all. There might be some value to moving all builtin auto-derives to MIR.

cc @rust-lang/lang @rust-lang/compiler

@arielb1
Copy link
Contributor

arielb1 commented Aug 6, 2017

@eddyb

What's your problem with the implementation? I think it's pretty clean.

However, we are probably not going to merge this at least before @nikomatsakis returns from vacation next week.

@@ -143,6 +143,12 @@ fn resolve_associated_item<'a, 'tcx>(
substs: rcvr_substs
}
}
traits::VtableBuiltin(ref data) => {
Instance {
def: ty::InstanceDef::BuiltinShim(def_id, data.ty),
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to store ty anywhere - it is available as trait_ref.self_ty().

@eddyb
Copy link
Member

eddyb commented Aug 6, 2017

@arielb1 My problem is not how it's implemented, but using this method only for some Clone impls. I'd be far more comfortable if all built-in derive impls were moved to MIR shims.

@arielb1
Copy link
Contributor

arielb1 commented Aug 6, 2017

@eddyb

Because that would mean the code is "tested" more? I'm not so sure on that - it e.g. means we'll have to re-implement error reporting on #[repr(packed)] types, and who knows what else. Also, Clone is still an inductive trait (and must be, because it has a supertrait), so we'll have to do something about the bounds.

DropGlue(DefId, Option<Ty<'tcx>>),

/// Builtin method implementation, e.g. `Clone::clone`.
BuiltinShim(DefId, Ty<'tcx>),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this be CloneShim? In other places I have a separate shim class for each trait.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems fine, I'll change that.

/// Build a `Clone::clone` shim for `recvr_ty`. Here, `def_id` is `Clone::clone`.
fn build_clone_shim<'a, 'tcx>(tcx: ty::TyCtxt<'a, 'tcx, 'tcx>,
def_id: DefId,
rcvr_ty: ty::Ty<'tcx>)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't call this rcvr_ty - ty is fine. The receiver is the first argument of the method, which has type &Self (e.g. &[Vec<u32>; 2] if the impl is for <[Vec<u32>; 2] as Clone>. Also fix the comment.

/// Build a `Clone::clone` shim for `recvr_ty`. Here, `def_id` is `Clone::clone`.
fn build_clone_shim<'a, 'tcx>(tcx: ty::TyCtxt<'a, 'tcx, 'tcx>,
def_id: DefId,
rcvr_ty: ty::Ty<'tcx>)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't call this rcvr_ty - ty is fine. The receiver is the first argument of the method, which has type &Self (e.g. &[Vec<u32>; 2] if the impl is for <[Vec<u32>; 2] as Clone>. Also fix the comment above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually there are too many variables named ty appearing in the code, and I ended up confusing two of them (and having a wrong code generation), that's why I renamed it but I agree rcvr_ty is not ideal, I'll find something else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is self_ty fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup self_ty would be fine.

rcvr_ty: ty::Ty<'tcx>)
-> Mir<'tcx>
{
let sig = tcx.fn_sig(def_id);
Copy link
Contributor

@arielb1 arielb1 Aug 7, 2017

Choose a reason for hiding this comment

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

Small note: this is a "fully generic" shim, so it knows the concrete receiver type. You could check if the source type is Copy and emit an "optimized" impl that just copies the result. This will speed up cloning [u8; 1048576] on unoptimized builds and is generally a good idea.

Also, as @eddyb says, it might be a smart idea to plumb #[derive(Clone)] impls through this to avoid implementing the logic twice and to potentially speed up compilation - maybe have #[derive(Clone)] add a #[is_builtin_clone] attribute to the method? I'm not sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

@arielb1 how would we handle bounds checking? #[derive(Clone)] currently uses local variables of type AssertParamIsClone<T: Clone + ?Sized> for each field type T in order to deal with recursive types. This would not be applicable here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually since we are "fully generic" here as you said, we would could indeed check by hand that the bounds hold for each field type when building the shim.

Copy link
Contributor

Choose a reason for hiding this comment

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

About plumbing #[derive(Clone)] - we want to give an error on the generic types, before monomorphization, so we can't use this function. So we need a separate checking function - maybe that is not a good idea after all.


match rcvr_ty.sty {
ty::TyArray(ty, len) => {
let mut returns = Vec::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried this might create code explosion for large arrays (think of <[Box<u8>; 1048576]>::clone). Can you try to use a loop and let LLVM unroll it? You're late MIR, so I don't think you have to care about pre-initializing the array.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is also the possibility to only build a Clone shim for [T; N] where T: Copy, as it's the case already.

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 just write a loop.

func,
args: vec![Operand::Consume(ref_loc)],
destination: Some((loc.clone(), BasicBlock::new(i + 1))),
cleanup: None,
Copy link
Contributor

@arielb1 arielb1 Aug 7, 2017

Choose a reason for hiding this comment

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

This is wrong - if the drop of a field panics, you need to drop all the previous fields.

It sounds like we have way too many places that try to create their own drop ladders, and we need some sort of unified abstraction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I was going to ask you about this anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll need to write a drop ladder abstraction to help clean this up.

@arielb1 arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 8, 2017
@scalexm scalexm force-pushed the issue-28229 branch 2 times, most recently from 7370f8f to 779d2c5 Compare August 8, 2017 18:13
@aturon
Copy link
Member

aturon commented Aug 8, 2017

From a lang team perspective, I am on board with this change. I don't have an opinion on the compiler side of things.

@scalexm
Copy link
Member Author

scalexm commented Aug 9, 2017

@arielb1 should be good now

@bors
Copy link
Contributor

bors commented Aug 11, 2017

☔ The latest upstream changes (presumably #43748) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis nikomatsakis self-assigned this Aug 21, 2017
@nikomatsakis
Copy link
Contributor

I like the approach. =)

@bors r+

@bors
Copy link
Contributor

bors commented Aug 21, 2017

📌 Commit d58b40e has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 21, 2017

⌛ Testing commit d58b40e with merge 31c59189014531ca6f69c95d28add2bd4b2f3647...

@bors
Copy link
Contributor

bors commented Aug 21, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

@bors
Copy link
Contributor

bors commented Aug 22, 2017

⌛ Testing commit d58b40e with merge 942711e...

bors added a commit that referenced this pull request Aug 22, 2017
Generate builtin impls for `Clone`

This fixes a long-standing ICE and limitation where some builtin types implement `Copy` but not `Clone` (whereas `Clone` is a super trait of `Copy`).

However, this PR has a few side-effects:
* `Clone` is now marked as a lang item.
* `[T; N]` is now `Clone` if `T: Clone` (currently, only if `T: Copy` and for `N <= 32`).
* `fn foo<'a>() where &'a mut (): Clone { }` won't compile anymore because of how bounds for builtin traits are handled (e.g. same thing currently if you replace `Clone` by `Copy` in this example). Of course this function is unusable anyway, an error would pop as soon as it is called.

Hence, I'm wondering wether this PR would need an RFC...
Also, cc-ing @nikomatsakis, @arielb1.

Related issues: #28229, #24000.
@bors
Copy link
Contributor

bors commented Aug 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 942711e to master...

@bors bors merged commit d58b40e into rust-lang:master Aug 22, 2017
@bors bors mentioned this pull request Aug 22, 2017
7 tasks
@nikomatsakis
Copy link
Contributor

OK, so, I think I was a bit hasty in merging this PR. I thought that the only question at hand was whether it was ok for clone to be a lang-item and for us to generate impls of Clone for types where T: Copy. I have no objection to either of those things. In particular, I feel that the existence of said clone impls is already implied by the language, so we're not "growing" the language by having the compiler synthesizing them, just fixing a bug (in discussion on IRC, @eddyb took an opposite approach, claiming that the bug is that types with no clone impl are considered Copy. This is fair, but changing that would be backwards incompatible, since e.g. [u8; 256] would no longer be copy).

However, reading more carefully, I see that this PR generates clone impls for a few other cases beyond that minimal set. To be fair, these were called out in the PR description. I was just hasty.

  • For example, prior to the PR, [T; 3] is only Clone if T: Copy (i.e., we use a macro to generate impls, and we only generate them if T: Copy, rather than T: Clone). I don't understand that, but it appears to have been a deliberate decision. Perhaps someone on @rust-lang/libs would care to comment.
  • Also, I do see a distinction between generating a built-in impl for those cases that are Copy (and hence are required to be Clone for the lang to be consistent) and other types (i.e., impls where we must invoke clone() manually).
    • So, for example, making [String; 256] be Clone would fall into the latter category (even ignoring the fact that, today, even [String; 2] is not Clone, since String is not Copy).
    • I personally am in favor of such a change, but I do think it ought to have gone through a formal consensus process. I don't see the need necessarily for a full RFC, but rfcbot at least.

For those two reasons, I am thinking that perhaps we should back out this change -- or at least disable it temporarily until we reach a consensus. (And perhaps both libs and lang team ought to be involved, given that [String; 2] is not Clone today?)

@alexcrichton
Copy link
Member

I'm not sure it was necessarily intentional for [String; 2] to not be Clone AFAIK. The only downside I could think of is that it adds a "huge" amount of IR/metadata bloat for something that doesn't actually work most of the time and is used in vanishingly rare situations.

@nikomatsakis
Copy link
Contributor

@alexcrichton

I'm not sure it was necessarily intentional for [String; 2] to not be Clone AFAIK. The only downside I could think of is that it adds a "huge" amount of IR/metadata bloat for something that doesn't actually work most of the time and is used in vanishingly rare situations.

Fair. Of course, under the approach implemented in this PR, that is not an issue.

@dtolnay
Copy link
Member

dtolnay commented Aug 22, 2017

(String, String) already implemented Clone. I am on board with [String; 2] implementing Clone, and the other changes listed in the PR description.

@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 22, 2017
arielb1 added a commit to arielb1/rust that referenced this pull request Aug 22, 2017
…tsakis"

This reverts commit 942711e, reversing
changes made to 8df670b.
@killercup killercup mentioned this pull request Aug 23, 2017
10 tasks
bors added a commit that referenced this pull request Aug 30, 2017
Remove the trait selection impl in method::probe

This removes the hacky trait selection reimplementation in `method::probe`, which occasionally comes and causes problems.

There are 2 issues I've found with this approach:
1. The older implementation sometimes had a "guess" type from an impl, which allowed subtyping to work. This is why I needed to make a change in `libtest`: there's an `impl<A> Clone for fn(A)` and we're calling `<for<'a> fn(&'a T) as Clone>::clone`. The older implementation would do a subtyping between the impl type and the trait type, so it would do the check for `<fn(A) as Clone>::clone`, and confirmation would continue with the subtyping. The newer implementation directly passes `<for<'a> fn(&'a T) as Clone>::clone` to selection, which fails. I'm not sure how big of a problem that would be in reality, especially after #43690 would remove the `Clone` problem, but I still want a crater run to avoid breaking the world.
2. The older implementation "looked into" impls to display error messages. I'm not sure that's an advantage - it looked exactly 1 level deep.

r? @eddyb
@bluss
Copy link
Member

bluss commented Sep 29, 2017

Tracking issue #44496

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 3, 2017
Changelog:
Version 1.21.0 (2017-10-12)
==========================

Language
--------
- [You can now use static references for literals.][43838]
  Example:
  ```rust
  fn main() {
      let x: &'static u32 = &0;
  }
  ```
- [Relaxed path syntax. Optional `::` before `<` is now allowed in all contexts.][43540]
  Example:
  ```rust
  my_macro!(Vec<i32>::new); // Always worked
  my_macro!(Vec::<i32>::new); // Now works
  ```

Compiler
--------
- [Upgraded jemalloc to 4.5.0][43911]
- [Enabled unwinding panics on Redox][43917]
- [Now runs LLVM in parallel during translation phase.][43506]
  This should reduce peak memory usage.

Libraries
---------
- [Generate builtin impls for `Clone` for all arrays and tuples that
  are `T: Clone`][43690]
- [`Stdin`, `Stdout`, and `Stderr` now implement `AsRawFd`.][43459]
- [`Rc` and `Arc` now implement `From<&[T]> where T: Clone`, `From<str>`,
  `From<String>`, `From<Box<T>> where T: ?Sized`, and `From<Vec<T>>`.][42565]

Stabilized APIs
---------------

[`std::mem::discriminant`]

Cargo
-----
- [You can now call `cargo install` with multiple package names][cargo/4216]
- [Cargo commands inside a virtual workspace will now implicitly
  pass `--all`][cargo/4335]
- [Added a `[patch]` section to `Cargo.toml` to handle
  prepublication dependencies][cargo/4123] [RFC 1969]
- [`include` & `exclude` fields in `Cargo.toml` now accept gitignore
  like patterns][cargo/4270]
- [Added the `--all-targets` option][cargo/4400]
- [Using required dependencies as a feature is now deprecated and emits
  a warning][cargo/4364]


Misc
----
- [Cargo docs are moving][43916]
  to [doc.rust-lang.org/cargo](https://doc.rust-lang.org/cargo)
- [The rustdoc book is now available][43863]
  at [doc.rust-lang.org/rustdoc](https://doc.rust-lang.org/rustdoc)
- [Added a preview of RLS has been made available through rustup][44204]
  Install with `rustup component add rls-preview`
- [`std::os` documentation for Unix, Linux, and Windows now appears on doc.rust-lang.org][43348]
  Previously only showed `std::os::unix`.

Compatibility Notes
-------------------
- [Changes in method matching against higher-ranked types][43880] This may cause
  breakage in subtyping corner cases. [A more in-depth explanation is available.][info/43880]
- [rustc's JSON error output's byte position start at top of file.][42973]
  Was previously relative to the rustc's internal `CodeMap` struct which
  required the unstable library `libsyntax` to correctly use.
- [`unused_results` lint no longer ignores booleans][43728]

[42565]: rust-lang/rust#42565
[42973]: rust-lang/rust#42973
[43348]: rust-lang/rust#43348
[43459]: rust-lang/rust#43459
[43506]: rust-lang/rust#43506
[43540]: rust-lang/rust#43540
[43690]: rust-lang/rust#43690
[43728]: rust-lang/rust#43728
[43838]: rust-lang/rust#43838
[43863]: rust-lang/rust#43863
[43880]: rust-lang/rust#43880
[43911]: rust-lang/rust#43911
[43916]: rust-lang/rust#43916
[43917]: rust-lang/rust#43917
[44204]: rust-lang/rust#44204
[cargo/4123]: rust-lang/cargo#4123
[cargo/4216]: rust-lang/cargo#4216
[cargo/4270]: rust-lang/cargo#4270
[cargo/4335]: rust-lang/cargo#4335
[cargo/4364]: rust-lang/cargo#4364
[cargo/4400]: rust-lang/cargo#4400
[RFC 1969]: rust-lang/rfcs#1969
[info/43880]: rust-lang/rust#44224 (comment)
[`std::mem::discriminant`]: https://doc.rust-lang.org/std/mem/fn.discriminant.html
hcpl added a commit to hcpl/zalgo.rs that referenced this pull request Apr 9, 2018
@scalexm scalexm deleted the issue-28229 branch October 22, 2018 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.