Repository navigation
Provide more context when an expected type was a type parameter - #160180
Conversation
This comment has been minimized.
This comment has been minimized.
|
r? @tiif rustbot has assigned @tiif. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| --> $DIR/defaults-specialization.rs:8:18 | ||
| | | ||
| LL | trait Tr { | ||
| ... |
There was a problem hiding this comment.
non-blocking style nit: will it be possible to have...?
LL | trait Tr {
| ...
There was a problem hiding this comment.
That change is something to bring up to rust-lang/annotate-snippets-rs. I don't think that rendering is necessarily ideal, because it would be hard to differentiate between the actual code being
trait Tr {
...and
trait Tr {
// this like will be skipped by the diagnostic renderer and replaced with ` ...`
By having it on the other side of the gutter, it makes that a bit clearer.
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
| && let param = generics.type_param(*param, tcx) | ||
| && let GenericParamDefKind::Type { has_default: true, .. } = param.kind | ||
| { | ||
| context.push(tcx.def_span(trait_m.container_id(tcx)).shrink_to_lo()); |
There was a problem hiding this comment.
What does this context do? Removing this line doesn't seem to change any test.
There was a problem hiding this comment.
It will ensure that line that starts the containing trait will be displayed. It would only happen with weirdly formatted code or really long signatures.
| && p.name == kw::SelfUpper | ||
| && infcx.can_eq(param_env, *ty, impl_trait_ref.self_ty()) | ||
| { | ||
| format!("expected to match the `Self` default of type parameter `{param_name}`") |
There was a problem hiding this comment.
We seem to not have test for this, could you help to add one?
| trait_err_span.map(|sp| { | ||
| let mut span: MultiSpan = sp.into(); | ||
| span.push_span_context(tcx.def_span(trait_m.container_id(tcx)).shrink_to_lo()); | ||
| span.push_span_context(tcx.def_span(trait_m.def_id).shrink_to_lo()); |
There was a problem hiding this comment.
This also doesn't seem to change anything when removed. What is the intention of adding this line?
| --> $DIR/defaults-specialization.rs:8:18 | ||
| | | ||
| LL | trait Tr { | ||
| ... |
This becomes particularly confusing when a type parameter has a default.
```
--> $DIR/default-type-parameter.rs:8:17
|
LL | fn foo() -> u32 { 0 }
| ^^^ expected `()`, found `u32`
|
note: type in trait
--> $DIR/default-type-parameter.rs:2:17
|
LL | fn foo() -> T;
| ^
= note: expected signature `fn() -> ()`
found signature `fn() -> u32`
note: expected to match this type parameter
--> $DIR/default-type-parameter.rs:1:13
|
LL | trait Trait<T = ()> {
| ^^^^^^
help: change the output type to match the trait
|
LL - fn foo() -> u32 { 0 }
LL + fn foo() -> () { 0 }
```
…the trait
```
error[E0053]: method `foo` has an incompatible type for trait
--> $DIR/async-example-desugared-boxed-in-trait.rs:11:28
|
LL | impl MyTrait for i32 {
LL | async fn foo(&self) -> i32 {
| ^^^ expected `Pin<Box<dyn Future<Output = i32>>>`, found future
|
note: type in trait
--> $DIR/async-example-desugared-boxed-in-trait.rs:7:22
|
LL | trait MyTrait {
LL | fn foo(&self) -> Pin<Box<dyn Future<Output = i32> + '_>>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: expected signature `fn(&_) -> Pin<Box<dyn Future<Output = i32>>>`
found signature `fn(&_) -> impl Future<Output = i32>`
```
…type in trait" note
```
error[E0053]: method `foo` has an incompatible type for trait
--> $DIR/default-type-parameter.rs:8:17
|
LL | impl Trait for S {
LL | fn foo() -> u32 { 0 }
| ^^^ expected `()`, found `u32`
|
note: type in trait
--> $DIR/default-type-parameter.rs:2:17
|
LL | trait Trait<T = ()> {
| ------ expected to match the default of type parameter `T`
LL | fn foo() -> T;
| ^
= note: expected signature `fn() -> ()`
found signature `fn() -> u32`
help: change the output type to match the trait
|
LL - fn foo() -> u32 { 0 }
LL + fn foo() -> () { 0 }
|
```
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@tiff modified a few tests to exercise the new logic. The context spans are not always needed under normally formatted code, but for more convoluted/long code the thing being pointed at is not on the same line as the item itself, which makes it harder to understand what part of the code the compiler is talking about at a glance. Ensuring the item is in view, helps to disambiguate, specially when there are many types or traits that have associated items with the same name. |
| LL | trait Trait< | ||
| LL | T = (), | ||
| | ------ expected to match the default of type parameter `T` | ||
| ... | ||
| LL | fn foo() | ||
| LL | -> T; | ||
| | ^ |
There was a problem hiding this comment.
This is an example of the context spans actually doing something.
| LL | K = Self, | ||
| | -------- expected to match the `Self` default of type parameter `K` |
There was a problem hiding this comment.
I'm surprised I even wrote logic for this case without a test. Good catch. IIRC, it is a common case for the math traits like Add<Rhs = Self>.
There was a problem hiding this comment.
The context span stuff is interesting to know, thank you! I pushed a commit to fix a typo to avoid another round of review cycle. It's time to land this.
@bors r+ rollup
…uwer Rollup of 6 pull requests Successful merges: - #138143 (rustdoc: add unstable option `--crate-list-heading` to customize the sidebar crate list.) - #165251 (Restore aarch64-apple-1 jobs with free disk space) - #160180 (Provide more context when an expected type was a type parameter) - #165262 (Replace `AbiAlign` with `Align` in function returns) - #165265 (Bless mir-opt tests again, again) - #165266 (Revert recent changes to async_drop)
Provide more context when an expected type was a type parameter
This becomes particularly confusing when a type parameter has a default.
```
error[E0053]: method `foo` has an incompatible type for trait
--> $DIR/default-type-parameter.rs:8:17
|
LL | impl Trait for S {
LL | fn foo() -> u32 { 0 }
| ^^^ expected `()`, found `u32`
|
note: type in trait
--> $DIR/default-type-parameter.rs:2:17
|
LL | trait Trait<T = ()> {
| ------ expected to match the default of type parameter `T`
LL | fn foo() -> T;
| ^
= note: expected signature `fn() -> ()`
found signature `fn() -> u32`
help: change the output type to match the trait
|
LL - fn foo() -> u32 { 0 }
LL + fn foo() -> () { 0 }
|
```
Fix rust-lang#67991.
Rollup merge of #160180 - estebank:issue-67991, r=tiif Provide more context when an expected type was a type parameter This becomes particularly confusing when a type parameter has a default. ``` error[E0053]: method `foo` has an incompatible type for trait --> $DIR/default-type-parameter.rs:8:17 | LL | impl Trait for S { LL | fn foo() -> u32 { 0 } | ^^^ expected `()`, found `u32` | note: type in trait --> $DIR/default-type-parameter.rs:2:17 | LL | trait Trait<T = ()> { | ------ expected to match the default of type parameter `T` LL | fn foo() -> T; | ^ = note: expected signature `fn() -> ()` found signature `fn() -> u32` help: change the output type to match the trait | LL - fn foo() -> u32 { 0 } LL + fn foo() -> () { 0 } | ``` Fix #67991.
…uwer Rollup of 6 pull requests Successful merges: - rust-lang/rust#138143 (rustdoc: add unstable option `--crate-list-heading` to customize the sidebar crate list.) - rust-lang/rust#165251 (Restore aarch64-apple-1 jobs with free disk space) - rust-lang/rust#160180 (Provide more context when an expected type was a type parameter) - rust-lang/rust#165262 (Replace `AbiAlign` with `Align` in function returns) - rust-lang/rust#165265 (Bless mir-opt tests again, again) - rust-lang/rust#165266 (Revert recent changes to async_drop)
View all comments
This becomes particularly confusing when a type parameter has a default.
Fix #67991.