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

Adt copy suggestions #94375

Merged
merged 4 commits into from
Mar 3, 2022
Merged

Adt copy suggestions #94375

merged 4 commits into from
Mar 3, 2022

Conversation

WaffleLapkin
Copy link
Member

Previously we've only suggested adding Copy bounds when the type being moved/copied is a type parameter (generic). With this PR we also suggest adding bounds when a type

  • Can be copy
  • All predicates that need to be satisfied for that are based on type params

i.e. we will suggest T: Copy for Option<T>, but won't suggest anything for Option<String>.

An example:

fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
    (t, t)
}

New error (current compiler doesn't provide help:):

error[E0382]: use of moved value: `t`
 --> t.rs:2:9
  |
1 | fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
  |                 - move occurs because `t` has type `Option<T>`, which does not implement the `Copy` trait
2 |     (t, t)
  |      -  ^ value used here after move
  |      |
  |      value moved here
  |
help: consider restricting type parameter `T`
  |
1 | fn duplicate<T: Copy>(t: Option<T>) -> (Option<T>, Option<T>) {
  |               ++++++

Fixes #93623
r? @estebank
@rustbot label +A-diagnostics +A-suggestion-diagnostics +C-enhancement


I'm not at all sure if this is the right implementation for this kind of suggestion, but it seems to work :')

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Feb 25, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2022
@WaffleLapkin WaffleLapkin marked this pull request as draft February 25, 2022 20:49
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

I need to do another pass, only skimmed, but it seems good!

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs Outdated Show resolved Hide resolved
Comment on lines 118 to 143
help: consider further restricting type parameter `T`
|
LL | T: B, T: Trait, T: Copy
| ~~~~~~~~~~~~~~~~~~~

error[E0382]: use of moved value: `t`
--> $DIR/use_of_moved_value_copy_suggestions.rs:57:9
|
LL | fn duplicate_custom_4<T: A>(t: S<T>) -> (S<T>, S<T>)
| - move occurs because `t` has type `S<T>`, which does not implement the `Copy` trait
...
LL | (t, t)
| - ^ value used here after move
| |
| value moved here
|
help: consider further restricting this bound
|
LL | T: B + Trait + Copy,
| ++++++++++++++
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that these two suggestions are different format but for the same thing.

Copy link
Member Author

@WaffleLapkin WaffleLapkin Feb 27, 2022

Choose a reason for hiding this comment

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

It tries to preserve layout of bounds. It actually currently works the same way in simpler cases (when type is type param), though I couldn't find any tests for this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3c458e11e88b2d22e32e6ebd0a3efae1

src/test/ui/moves/use_of_moved_value_copy_suggestions.rs Outdated Show resolved Hide resolved
Comment on lines +56 to +63
help: consider restricting type parameters
|
LL | fn duplicate_tup2<A: Copy, B: Copy>(t: (A, B)) -> ((A, B), (A, B)) {
| ++++++ ++++++
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, re: run-rustfix I think these won't be properly applied so they'd need to be split as well to their own test file (but try it anyways, they might have fixed rustfix since I last looked).

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like rustfix was fixed 👀

Add `rustc_middle::ty::suggest_constraining_type_params` that suggests
adding multiple constraints.

`suggest_constraining_type_param` now just forwards params to this new
function.
Previously we've only suggested adding `Copy` bounds when the type being
moved/copied is a type parameter (generic). With this commit we also
suggest adding bounds when a type
- Can be copy
- All predicates that need to be satisfied for that are based on type
  params

i.e. we will suggest `T: Copy` for `Option<T>`, but won't suggest
anything for `Option<String>`.

Future work: it would be nice to also suggest adding `.clone()` calls
@WaffleLapkin WaffleLapkin marked this pull request as ready for review March 1, 2022 14:48
@WaffleLapkin
Copy link
Member Author

Made the test // run-rustfix + rebased on master. I believe this PR is ready :)

@estebank
Copy link
Contributor

estebank commented Mar 1, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Mar 1, 2022

📌 Commit f0a16b8 has been approved by estebank

@bors 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 Mar 1, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 1, 2022
…ebank

Adt copy suggestions

Previously we've only suggested adding `Copy` bounds when the type being moved/copied is a type parameter (generic). With this PR we also suggest adding bounds when a type
- Can be copy
- All predicates that need to be satisfied for that are based on type params

i.e. we will suggest `T: Copy` for `Option<T>`, but won't suggest anything for `Option<String>`.

An example:
```rust
fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
    (t, t)
}
```
New error (current compiler doesn't provide `help`:):
```text
error[E0382]: use of moved value: `t`
 --> t.rs:2:9
  |
1 | fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
  |                 - move occurs because `t` has type `Option<T>`, which does not implement the `Copy` trait
2 |     (t, t)
  |      -  ^ value used here after move
  |      |
  |      value moved here
  |
help: consider restricting type parameter `T`
  |
1 | fn duplicate<T: Copy>(t: Option<T>) -> (Option<T>, Option<T>) {
  |               ++++++
```

Fixes rust-lang#93623
r? `@estebank`
`@rustbot` label +A-diagnostics +A-suggestion-diagnostics +C-enhancement

----

I'm not at all sure if this is the right implementation for this kind of suggestion, but it seems to work :')
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 2, 2022
…ebank

Adt copy suggestions

Previously we've only suggested adding `Copy` bounds when the type being moved/copied is a type parameter (generic). With this PR we also suggest adding bounds when a type
- Can be copy
- All predicates that need to be satisfied for that are based on type params

i.e. we will suggest `T: Copy` for `Option<T>`, but won't suggest anything for `Option<String>`.

An example:
```rust
fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
    (t, t)
}
```
New error (current compiler doesn't provide `help`:):
```text
error[E0382]: use of moved value: `t`
 --> t.rs:2:9
  |
1 | fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
  |                 - move occurs because `t` has type `Option<T>`, which does not implement the `Copy` trait
2 |     (t, t)
  |      -  ^ value used here after move
  |      |
  |      value moved here
  |
help: consider restricting type parameter `T`
  |
1 | fn duplicate<T: Copy>(t: Option<T>) -> (Option<T>, Option<T>) {
  |               ++++++
```

Fixes rust-lang#93623
r? ``@estebank``
``@rustbot`` label +A-diagnostics +A-suggestion-diagnostics +C-enhancement

----

I'm not at all sure if this is the right implementation for this kind of suggestion, but it seems to work :')
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 2, 2022
…ebank

Adt copy suggestions

Previously we've only suggested adding `Copy` bounds when the type being moved/copied is a type parameter (generic). With this PR we also suggest adding bounds when a type
- Can be copy
- All predicates that need to be satisfied for that are based on type params

i.e. we will suggest `T: Copy` for `Option<T>`, but won't suggest anything for `Option<String>`.

An example:
```rust
fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
    (t, t)
}
```
New error (current compiler doesn't provide `help`:):
```text
error[E0382]: use of moved value: `t`
 --> t.rs:2:9
  |
1 | fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
  |                 - move occurs because `t` has type `Option<T>`, which does not implement the `Copy` trait
2 |     (t, t)
  |      -  ^ value used here after move
  |      |
  |      value moved here
  |
help: consider restricting type parameter `T`
  |
1 | fn duplicate<T: Copy>(t: Option<T>) -> (Option<T>, Option<T>) {
  |               ++++++
```

Fixes rust-lang#93623
r? ```@estebank```
```@rustbot``` label +A-diagnostics +A-suggestion-diagnostics +C-enhancement

----

I'm not at all sure if this is the right implementation for this kind of suggestion, but it seems to work :')
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 2, 2022
…ebank

Adt copy suggestions

Previously we've only suggested adding `Copy` bounds when the type being moved/copied is a type parameter (generic). With this PR we also suggest adding bounds when a type
- Can be copy
- All predicates that need to be satisfied for that are based on type params

i.e. we will suggest `T: Copy` for `Option<T>`, but won't suggest anything for `Option<String>`.

An example:
```rust
fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
    (t, t)
}
```
New error (current compiler doesn't provide `help`:):
```text
error[E0382]: use of moved value: `t`
 --> t.rs:2:9
  |
1 | fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
  |                 - move occurs because `t` has type `Option<T>`, which does not implement the `Copy` trait
2 |     (t, t)
  |      -  ^ value used here after move
  |      |
  |      value moved here
  |
help: consider restricting type parameter `T`
  |
1 | fn duplicate<T: Copy>(t: Option<T>) -> (Option<T>, Option<T>) {
  |               ++++++
```

Fixes rust-lang#93623
r? ````@estebank````
````@rustbot```` label +A-diagnostics +A-suggestion-diagnostics +C-enhancement

----

I'm not at all sure if this is the right implementation for this kind of suggestion, but it seems to work :')
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 2, 2022
…ebank

Adt copy suggestions

Previously we've only suggested adding `Copy` bounds when the type being moved/copied is a type parameter (generic). With this PR we also suggest adding bounds when a type
- Can be copy
- All predicates that need to be satisfied for that are based on type params

i.e. we will suggest `T: Copy` for `Option<T>`, but won't suggest anything for `Option<String>`.

An example:
```rust
fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
    (t, t)
}
```
New error (current compiler doesn't provide `help`:):
```text
error[E0382]: use of moved value: `t`
 --> t.rs:2:9
  |
1 | fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
  |                 - move occurs because `t` has type `Option<T>`, which does not implement the `Copy` trait
2 |     (t, t)
  |      -  ^ value used here after move
  |      |
  |      value moved here
  |
help: consider restricting type parameter `T`
  |
1 | fn duplicate<T: Copy>(t: Option<T>) -> (Option<T>, Option<T>) {
  |               ++++++
```

Fixes rust-lang#93623
r? `````@estebank`````
`````@rustbot````` label +A-diagnostics +A-suggestion-diagnostics +C-enhancement

----

I'm not at all sure if this is the right implementation for this kind of suggestion, but it seems to work :')
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 2, 2022
…ebank

Adt copy suggestions

Previously we've only suggested adding `Copy` bounds when the type being moved/copied is a type parameter (generic). With this PR we also suggest adding bounds when a type
- Can be copy
- All predicates that need to be satisfied for that are based on type params

i.e. we will suggest `T: Copy` for `Option<T>`, but won't suggest anything for `Option<String>`.

An example:
```rust
fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
    (t, t)
}
```
New error (current compiler doesn't provide `help`:):
```text
error[E0382]: use of moved value: `t`
 --> t.rs:2:9
  |
1 | fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
  |                 - move occurs because `t` has type `Option<T>`, which does not implement the `Copy` trait
2 |     (t, t)
  |      -  ^ value used here after move
  |      |
  |      value moved here
  |
help: consider restricting type parameter `T`
  |
1 | fn duplicate<T: Copy>(t: Option<T>) -> (Option<T>, Option<T>) {
  |               ++++++
```

Fixes rust-lang#93623
r? ``````@estebank``````
``````@rustbot`````` label +A-diagnostics +A-suggestion-diagnostics +C-enhancement

----

I'm not at all sure if this is the right implementation for this kind of suggestion, but it seems to work :')
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 2, 2022
…ebank

Adt copy suggestions

Previously we've only suggested adding `Copy` bounds when the type being moved/copied is a type parameter (generic). With this PR we also suggest adding bounds when a type
- Can be copy
- All predicates that need to be satisfied for that are based on type params

i.e. we will suggest `T: Copy` for `Option<T>`, but won't suggest anything for `Option<String>`.

An example:
```rust
fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
    (t, t)
}
```
New error (current compiler doesn't provide `help`:):
```text
error[E0382]: use of moved value: `t`
 --> t.rs:2:9
  |
1 | fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
  |                 - move occurs because `t` has type `Option<T>`, which does not implement the `Copy` trait
2 |     (t, t)
  |      -  ^ value used here after move
  |      |
  |      value moved here
  |
help: consider restricting type parameter `T`
  |
1 | fn duplicate<T: Copy>(t: Option<T>) -> (Option<T>, Option<T>) {
  |               ++++++
```

Fixes rust-lang#93623
r? ```````@estebank```````
```````@rustbot``````` label +A-diagnostics +A-suggestion-diagnostics +C-enhancement

----

I'm not at all sure if this is the right implementation for this kind of suggestion, but it seems to work :')
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 2, 2022
…ebank

Adt copy suggestions

Previously we've only suggested adding `Copy` bounds when the type being moved/copied is a type parameter (generic). With this PR we also suggest adding bounds when a type
- Can be copy
- All predicates that need to be satisfied for that are based on type params

i.e. we will suggest `T: Copy` for `Option<T>`, but won't suggest anything for `Option<String>`.

An example:
```rust
fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
    (t, t)
}
```
New error (current compiler doesn't provide `help`:):
```text
error[E0382]: use of moved value: `t`
 --> t.rs:2:9
  |
1 | fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
  |                 - move occurs because `t` has type `Option<T>`, which does not implement the `Copy` trait
2 |     (t, t)
  |      -  ^ value used here after move
  |      |
  |      value moved here
  |
help: consider restricting type parameter `T`
  |
1 | fn duplicate<T: Copy>(t: Option<T>) -> (Option<T>, Option<T>) {
  |               ++++++
```

Fixes rust-lang#93623
r? ````````@estebank````````
````````@rustbot```````` label +A-diagnostics +A-suggestion-diagnostics +C-enhancement

----

I'm not at all sure if this is the right implementation for this kind of suggestion, but it seems to work :')
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 2, 2022
…ebank

Adt copy suggestions

Previously we've only suggested adding `Copy` bounds when the type being moved/copied is a type parameter (generic). With this PR we also suggest adding bounds when a type
- Can be copy
- All predicates that need to be satisfied for that are based on type params

i.e. we will suggest `T: Copy` for `Option<T>`, but won't suggest anything for `Option<String>`.

An example:
```rust
fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
    (t, t)
}
```
New error (current compiler doesn't provide `help`:):
```text
error[E0382]: use of moved value: `t`
 --> t.rs:2:9
  |
1 | fn duplicate<T>(t: Option<T>) -> (Option<T>, Option<T>) {
  |                 - move occurs because `t` has type `Option<T>`, which does not implement the `Copy` trait
2 |     (t, t)
  |      -  ^ value used here after move
  |      |
  |      value moved here
  |
help: consider restricting type parameter `T`
  |
1 | fn duplicate<T: Copy>(t: Option<T>) -> (Option<T>, Option<T>) {
  |               ++++++
```

Fixes rust-lang#93623
r? `````````@estebank`````````
`````````@rustbot````````` label +A-diagnostics +A-suggestion-diagnostics +C-enhancement

----

I'm not at all sure if this is the right implementation for this kind of suggestion, but it seems to work :')
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2022
Rollup of 9 pull requests

Successful merges:

 - rust-lang#92061 (update char signess for openbsd)
 - rust-lang#93072 (Compatible variants suggestion with desugaring)
 - rust-lang#93354 (Add documentation about `BorrowedFd::to_owned`.)
 - rust-lang#93663 (Rename `BorrowedFd::borrow_raw_fd` to `BorrowedFd::borrow_raw`.)
 - rust-lang#94375 (Adt copy suggestions)
 - rust-lang#94433 (Improve allowness of the unexpected_cfgs lint)
 - rust-lang#94499 (Documentation was missed when demoting Windows XP to no_std only)
 - rust-lang#94505 (Restore the local filter on mono item sorting)
 - rust-lang#94529 (Unused doc comments blocks)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7537b20 into rust-lang:master Mar 3, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 3, 2022
@WaffleLapkin WaffleLapkin deleted the copy-suggestion branch March 3, 2022 09:11
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 17, 2022
…ons, r=estebank

Remove redundant code from copy-suggestions

Follow up to rust-lang#94375, just remove some code that is not necessary anymore. This may make the perf of such suggestions a little bit worse, but I don't think this is significant.

r? `@estebank`
@WaffleLapkin WaffleLapkin mentioned this pull request Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-enhancement Category: An issue proposing an enhancement or a PR with one. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance compiler error for constraining builtin traits
5 participants