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

Detect when method call on argument could be removed to fulfill failed trait bound #121100

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Feb 14, 2024

When encountering

struct Foo;
struct Bar;
impl From<Bar> for Foo {
    fn from(_: Bar) -> Self { Foo }
}
fn qux(_: impl From<Bar>) {}
fn main() {
    qux(Bar.into());
}

Suggest removing .into():

error[E0283]: type annotations needed
 --> f100.rs:8:13
  |
8 |     qux(Bar.into());
  |     ---     ^^^^
  |     |
  |     required by a bound introduced by this call
  |
  = note: cannot satisfy `_: From<Bar>`
note: required by a bound in `qux`
 --> f100.rs:6:16
  |
6 | fn qux(_: impl From<Bar>) {}
  |                ^^^^^^^^^ required by this bound in `qux`
help: try using a fully qualified path to specify the expected types
  |
8 |     qux(<Bar as Into<T>>::into(Bar));
  |         +++++++++++++++++++++++   ~
help: consider removing this method call, as the receiver has type `Bar` and `Bar: From<Bar>` trivially holds
  |
8 -     qux(Bar.into());
8 +     qux(Bar);
  |

Fix #71252

@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 14, 2024
expr.span.with_lo(rcvr.span.hi()),
format!(
"consider removing this method call, as the receiver has type `{ty}` and \
`{pred}` can be fulfilled",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`{pred}` can be fulfilled",
`{pred}` is trivial",

or "trivially holds"

Copy link
Contributor Author

@estebank estebank Feb 14, 2024

Choose a reason for hiding this comment

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

@compiler-errors do we ever use that wording in user-facing messages? AFAICT we only use that wording for other compiler devs. (We can argue that "can be fulfilled" has the same problem.) Maybe "{pred} is met"?

Copy link
Member

@compiler-errors compiler-errors Feb 14, 2024

Choose a reason for hiding this comment

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

I could say the same about "fulfilled". 😆 don't believe the original wording gets across the fact that the method call is redundant, which is what I think this should be pointing out.

Copy link
Member

Choose a reason for hiding this comment

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

The point that I'm trying to make is that the wording should explain to the user why removing the method call is beneficial, which I don't believe it is doing right now; rn it's just stating facts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@compiler-errors yeah, that makes sense. Initially I was thinking "well, there could be cases where the predicate is actually more complex and the Self is something like an assoc type or impl Trait", but thinking about it for just a minute was enough to realize that you're right and "yes, every case where this would work would be a trivial obligation".

Copy link
Member

Choose a reason for hiding this comment

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

I like "is trivial" better than "can be fulfilled" 👍

I wonder if something even simpler would work ("is valid" or "is true" perhaps)?

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me after nits

if let hir::Expr { kind: hir::ExprKind::MethodCall(_, rcvr, _, _), .. } = expr
&& let Some(ty) = typeck_results.node_type_opt(rcvr.hir_id)
&& let Some(failed_pred) = failed_pred.to_opt_poly_trait_pred()
&& let pred = failed_pred.skip_binder().with_self_ty(tcx, ty)
Copy link
Member

Choose a reason for hiding this comment

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

use map_bound here. otherwise, it's gonna ICE because you're skipping binders.

&& let Some(failed_pred) = failed_pred.to_opt_poly_trait_pred()
&& let pred = failed_pred.skip_binder().with_self_ty(tcx, ty)
&& self
.evaluate_obligation_no_overflow(&Obligation::misc(
Copy link
Member

Choose a reason for hiding this comment

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

why did you use evaluate_obligation_no_overflow here? please use predicate_may_hold_modulo_regions.

@compiler-errors compiler-errors 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 Feb 15, 2024
@estebank
Copy link
Contributor Author

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Feb 16, 2024

📌 Commit 5a1c454 has been approved by compiler-errors

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 16, 2024
@compiler-errors
Copy link
Member

Why did you use predicate_may_hold? That can hold even if there is inference that still needs to be made. Could you use predicate_may_hold_modulo_regions? False positives here feel like they would be really confusing.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 16, 2024
@compiler-errors
Copy link
Member

Also, is there a reason why you left out the test from the PR description? None of the UI tests that were changed are trait ambiguity errors, just regular trait failures. Please add that test, too.

@rust-log-analyzer

This comment has been minimized.

…d trait bound

When encountering

```rust
struct Foo;
struct Bar;
impl From<Bar> for Foo {
    fn from(_: Bar) -> Self { Foo }
}
fn qux(_: impl From<Bar>) {}
fn main() {
    qux(Bar.into());
}
```

Suggest removing `.into()`:

```
error[E0283]: type annotations needed
 --> f100.rs:8:13
  |
8 |     qux(Bar.into());
  |     ---     ^^^^
  |     |
  |     required by a bound introduced by this call
  |
  = note: cannot satisfy `_: From<Bar>`
note: required by a bound in `qux`
 --> f100.rs:6:16
  |
6 | fn qux(_: impl From<Bar>) {}
  |                ^^^^^^^^^ required by this bound in `qux`
help: try using a fully qualified path to specify the expected types
  |
8 |     qux(<Bar as Into<T>>::into(Bar));
  |         +++++++++++++++++++++++   ~
help: consider removing this method call, as the receiver has type `Bar` and `Bar: From<Bar>` can be fulfilled
  |
8 -     qux(Bar.into());
8 +     qux(Bar);
  |
```

Fix rust-lang#71252
@estebank
Copy link
Contributor Author

Updated

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Feb 16, 2024

📌 Commit 9b3fcf9 has been approved by compiler-errors

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 16, 2024
@estebank
Copy link
Contributor Author

Why did you use predicate_may_hold? That can hold even if there is inference that still needs to be made. Could you use predicate_may_hold_modulo_regions?

I got confused between predicate_may_hold and predicate_must_hold_modulo_regions. That's what I get for multitasking paying half attention.

Nadrieril added a commit to Nadrieril/rust that referenced this pull request Feb 17, 2024
…rrors

Detect when method call on argument could be removed to fulfill failed trait bound

When encountering

```rust
struct Foo;
struct Bar;
impl From<Bar> for Foo {
    fn from(_: Bar) -> Self { Foo }
}
fn qux(_: impl From<Bar>) {}
fn main() {
    qux(Bar.into());
}
```

Suggest removing `.into()`:

```
error[E0283]: type annotations needed
 --> f100.rs:8:13
  |
8 |     qux(Bar.into());
  |     ---     ^^^^
  |     |
  |     required by a bound introduced by this call
  |
  = note: cannot satisfy `_: From<Bar>`
note: required by a bound in `qux`
 --> f100.rs:6:16
  |
6 | fn qux(_: impl From<Bar>) {}
  |                ^^^^^^^^^ required by this bound in `qux`
help: try using a fully qualified path to specify the expected types
  |
8 |     qux(<Bar as Into<T>>::into(Bar));
  |         +++++++++++++++++++++++   ~
help: consider removing this method call, as the receiver has type `Bar` and `Bar: From<Bar>` trivially holds
  |
8 -     qux(Bar.into());
8 +     qux(Bar);
  |
```

Fix rust-lang#71252
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 17, 2024
…rrors

Detect when method call on argument could be removed to fulfill failed trait bound

When encountering

```rust
struct Foo;
struct Bar;
impl From<Bar> for Foo {
    fn from(_: Bar) -> Self { Foo }
}
fn qux(_: impl From<Bar>) {}
fn main() {
    qux(Bar.into());
}
```

Suggest removing `.into()`:

```
error[E0283]: type annotations needed
 --> f100.rs:8:13
  |
8 |     qux(Bar.into());
  |     ---     ^^^^
  |     |
  |     required by a bound introduced by this call
  |
  = note: cannot satisfy `_: From<Bar>`
note: required by a bound in `qux`
 --> f100.rs:6:16
  |
6 | fn qux(_: impl From<Bar>) {}
  |                ^^^^^^^^^ required by this bound in `qux`
help: try using a fully qualified path to specify the expected types
  |
8 |     qux(<Bar as Into<T>>::into(Bar));
  |         +++++++++++++++++++++++   ~
help: consider removing this method call, as the receiver has type `Bar` and `Bar: From<Bar>` trivially holds
  |
8 -     qux(Bar.into());
8 +     qux(Bar);
  |
```

Fix rust-lang#71252
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2024
…llaumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#118264 (Optimize `VecDeque::drain` for (half-)open ranges)
 - rust-lang#121079 (distribute tool documentations and avoid file conflicts on `x install`)
 - rust-lang#121100 (Detect when method call on argument could be removed to fulfill failed trait bound)
 - rust-lang#121160 (rustdoc: fix and refactor HTML rendering a bit)
 - rust-lang#121198 (Add more checks for `unnamed_fields` during HIR analysis)
 - rust-lang#121221 (AstConv: Refactor lowering of associated item bindings a bit)
 - rust-lang#121237 (Use better heuristic for printing Cargo specific diagnostics)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#120526 (rustdoc: Correctly handle long crate names on mobile)
 - rust-lang#121100 (Detect when method call on argument could be removed to fulfill failed trait bound)
 - rust-lang#121160 (rustdoc: fix and refactor HTML rendering a bit)
 - rust-lang#121198 (Add more checks for `unnamed_fields` during HIR analysis)
 - rust-lang#121218 (Fix missing trait impls for type in rustc docs)
 - rust-lang#121221 (AstConv: Refactor lowering of associated item bindings a bit)
 - rust-lang#121237 (Use better heuristic for printing Cargo specific diagnostics)

r? `@ghost`
`@rustbot` modify labels: rollup
CKingX added a commit to CKingX/rust that referenced this pull request Feb 18, 2024
…rrors

Detect when method call on argument could be removed to fulfill failed trait bound

When encountering

```rust
struct Foo;
struct Bar;
impl From<Bar> for Foo {
    fn from(_: Bar) -> Self { Foo }
}
fn qux(_: impl From<Bar>) {}
fn main() {
    qux(Bar.into());
}
```

Suggest removing `.into()`:

```
error[E0283]: type annotations needed
 --> f100.rs:8:13
  |
8 |     qux(Bar.into());
  |     ---     ^^^^
  |     |
  |     required by a bound introduced by this call
  |
  = note: cannot satisfy `_: From<Bar>`
note: required by a bound in `qux`
 --> f100.rs:6:16
  |
6 | fn qux(_: impl From<Bar>) {}
  |                ^^^^^^^^^ required by this bound in `qux`
help: try using a fully qualified path to specify the expected types
  |
8 |     qux(<Bar as Into<T>>::into(Bar));
  |         +++++++++++++++++++++++   ~
help: consider removing this method call, as the receiver has type `Bar` and `Bar: From<Bar>` trivially holds
  |
8 -     qux(Bar.into());
8 +     qux(Bar);
  |
```

Fix rust-lang#71252
@bors bors merged commit 6499eb5 into rust-lang:master Feb 18, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 18, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2024
Rollup merge of rust-lang#121100 - estebank:issue-71252, r=compiler-errors

Detect when method call on argument could be removed to fulfill failed trait bound

When encountering

```rust
struct Foo;
struct Bar;
impl From<Bar> for Foo {
    fn from(_: Bar) -> Self { Foo }
}
fn qux(_: impl From<Bar>) {}
fn main() {
    qux(Bar.into());
}
```

Suggest removing `.into()`:

```
error[E0283]: type annotations needed
 --> f100.rs:8:13
  |
8 |     qux(Bar.into());
  |     ---     ^^^^
  |     |
  |     required by a bound introduced by this call
  |
  = note: cannot satisfy `_: From<Bar>`
note: required by a bound in `qux`
 --> f100.rs:6:16
  |
6 | fn qux(_: impl From<Bar>) {}
  |                ^^^^^^^^^ required by this bound in `qux`
help: try using a fully qualified path to specify the expected types
  |
8 |     qux(<Bar as Into<T>>::into(Bar));
  |         +++++++++++++++++++++++   ~
help: consider removing this method call, as the receiver has type `Bar` and `Bar: From<Bar>` trivially holds
  |
8 -     qux(Bar.into());
8 +     qux(Bar);
  |
```

Fix rust-lang#71252
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Calling .into() on argument that expects Into<Type> gives confusing error
6 participants