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

hir_typeck: be more conservative in making "note caller chooses ty param" note #126558

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Jun 16, 2024

In #122195 I added a "caller chooses ty for type param" note for when the return expression type a.k.a. found type does not match the expected return type.

#126547 found that this note was confusing when the found return type contains the expected type, e.g.

fn f<T>(t: &T) -> T {
    t
}

because the found return type &T will always be different from the expected return type T, so the note was needlessly redundant and confusing.

This PR addresses that by not making the note if the found return type contains the expected return type.

r? @fmease (since you reviewed the original PR)

Fixes #126547

@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 Jun 16, 2024
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines 877 to 880
debug!("return type (hir::Ty) {:?}", hir_ty);
let ty = self.lowerer().lower_ty(hir_ty);
debug!("return type {:?}", ty);
debug!("return type (ty) {:?}", ty);
debug!("expected type {:?}", expected);
Copy link
Member

@fmease fmease Jun 18, 2024

Choose a reason for hiding this comment

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

Btw, tracing has a nice shorthand for this but it doesn't matter. For future reference:

debug!(?hir_ty, "return type");
let ty = self.lowerer().lower_ty(hir_ty);
debug!(?ty, "return type");
debug!(ty = ?expected, "expected type");

which will print:

return type hir_ty=...
return type ty=...
expected type ty=...

Copy link
Member Author

Choose a reason for hiding this comment

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

(I actually did know that shorthand, I just copied over what existed lol)

Copy link
Member Author

Choose a reason for hiding this comment

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

... but now that you mentioned it, it's indeed nicer so I changed it ^^

@fmease
Copy link
Member

fmease commented Jun 18, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 18, 2024

📌 Commit a5eff62 has been approved by fmease

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 18, 2024
hir_typeck: be more conservative in making "note caller chooses ty param" note

In rust-lang#122195 I added a "caller chooses ty for type param" note for when the return expression type a.k.a. found type does not match the expected return type.

rust-lang#126547 found that this note was confusing when the found return type *contains* the expected type, e.g.

```rs
fn f<T>(t: &T) -> T {
    t
}
```

because the found return type `&T` will *always* be different from the expected return type `T`, so the note was needlessly redundant and confusing.

This PR addresses that by not making the note if the found return type contains the expected return type.

r? `@fmease` (since you reviewed the original PR)

Fixes rust-lang#126547
@bors
Copy link
Contributor

bors commented Jun 18, 2024

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

@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 Jun 18, 2024
jieyouxu added 2 commits June 18, 2024 21:05
…ram" note

- Avoid "caller chooses ty for type param" note if the found type a.k.a.
  the return expression type *contains* the type parameter, because e.g.
  `&T` will always be different from `T` (i.e. "well duh").
- Rename `note_caller_chooses_ty_for_ty_param` to
  `try_note_caller_chooses_ty_for_ty_param` because the note is not
  always made.

Issue: rust-lang#126547
@rust-cloud-vms rust-cloud-vms bot force-pushed the caller-chooses-ty branch from a5eff62 to 939026c Compare June 18, 2024 21:07
@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 18, 2024

Rebased for diag.subdiagnostic() improvements that removed need for passing dcx() (hecc yeah) and cleaned up the tracing debug usages. EDIT: ui tests pass locally.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 18, 2024
…llaumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#123782 (Test that opaque types can't have themselves as a hidden type with incompatible lifetimes)
 - rust-lang#124580 (Suggest removing unused tuple fields if they are the last fields)
 - rust-lang#125852 (improve tip for inaccessible traits)
 - rust-lang#126422 (Suggest using a standalone doctest for non-local impl defs)
 - rust-lang#126427 (Rewrite `intrinsic-unreachable`, `sepcomp-cci-copies`, `sepcomp-inlining` and `sepcomp-separate` `run-make` tests to rmake.rs)
 - rust-lang#126493 (safe transmute: support non-ZST, variantful, uninhabited enums)
 - rust-lang#126572 (override user defined channel when using precompiled rustc)
 - rust-lang#126615 (Add `rustc-ice*` to `.gitignore`)
 - rust-lang#126632 (Replace `move||` with `move ||`)

Failed merges:

 - rust-lang#126558 (hir_typeck: be more conservative in making "note caller chooses ty param" note)

r? `@ghost`
`@rustbot` modify labels: rollup
@jieyouxu
Copy link
Member Author

@rustbot review

@rustbot rustbot 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 Jun 18, 2024
@fmease
Copy link
Member

fmease commented Jun 18, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jun 18, 2024

📌 Commit 939026c has been approved by fmease

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jun 19, 2024
hir_typeck: be more conservative in making "note caller chooses ty param" note

In rust-lang#122195 I added a "caller chooses ty for type param" note for when the return expression type a.k.a. found type does not match the expected return type.

rust-lang#126547 found that this note was confusing when the found return type *contains* the expected type, e.g.

```rs
fn f<T>(t: &T) -> T {
    t
}
```

because the found return type `&T` will *always* be different from the expected return type `T`, so the note was needlessly redundant and confusing.

This PR addresses that by not making the note if the found return type contains the expected return type.

r? `@fmease` (since you reviewed the original PR)

Fixes rust-lang#126547
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Rollup of 10 pull requests

Successful merges:

 - rust-lang#124135 (delegation: Implement glob delegation)
 - rust-lang#125078 (fix: break inside async closure has incorrect span for enclosing closure)
 - rust-lang#125293 (Place tail expression behind terminating scope)
 - rust-lang#126422 (Suggest using a standalone doctest for non-local impl defs)
 - rust-lang#126493 (safe transmute: support non-ZST, variantful, uninhabited enums)
 - rust-lang#126504 (Sync fuchsia test runner with clang test runner)
 - rust-lang#126558 (hir_typeck: be more conservative in making "note caller chooses ty param" note)
 - rust-lang#126586 (Add `@badboy` and `@BlackHoleFox` as Mac Catalyst maintainers)
 - rust-lang#126615 (Add `rustc-ice*` to `.gitignore`)
 - rust-lang#126632 (Replace `move||` with `move ||`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bf841c4 into rust-lang:master Jun 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Rollup merge of rust-lang#126558 - jieyouxu:caller-chooses-ty, r=fmease

hir_typeck: be more conservative in making "note caller chooses ty param" note

In rust-lang#122195 I added a "caller chooses ty for type param" note for when the return expression type a.k.a. found type does not match the expected return type.

rust-lang#126547 found that this note was confusing when the found return type *contains* the expected type, e.g.

```rs
fn f<T>(t: &T) -> T {
    t
}
```

because the found return type `&T` will *always* be different from the expected return type `T`, so the note was needlessly redundant and confusing.

This PR addresses that by not making the note if the found return type contains the expected return type.

r? ``@fmease`` (since you reviewed the original PR)

Fixes rust-lang#126547
@jieyouxu jieyouxu deleted the caller-chooses-ty branch June 29, 2024 08:18
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.

Confusing "the caller chooses a type which can be different" [E308]
4 participants