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

Highlight and simplify mismatched types #41205

Merged
merged 1 commit into from
Apr 12, 2017

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Apr 10, 2017

Shorten mismatched types errors by replacing subtypes that are not
different with _, and highlighting only the subtypes that are
different.

Given a file

struct X<T1, T2> {
    x: T1,
    y: T2,
}

fn foo() -> X<X<String, String>, String> {
    X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
}

fn bar() -> Option<String> {
    "".to_string()
}

provide the following output

error[E0308]: mismatched types
  --> file.rs:6:5
   |
 6 |     X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `std::string::String`, found {integer}
   |
   = note: expected type `X<X<_, std::string::String>, _>`
                                 ^^^^^^^^^^^^^^^^^^^   // < highlighted
              found type `X<X<_, {integer}>, _>`
                                 ^^^^^^^^^             // < highlighted

error[E0308]: mismatched types
  --> file.rs:6:5
   |
10 |     "".to_string()
   |     ^^^^^^^^^^^^^^ expected struct `std::option::Option`, found `std::string::String`
   |
   = note: expected type `Option<std::string::String>`
                          ^^^^^^^                   ^  // < highlighted
              found type `std::string::String`

Fix #21025. Re: #40186. Follow up to #39906.

I'm looking to change how this output is accomplished so that it doesn't create list of strings to pass around, but rather add an elided Ty placeholder, and use the same string formatting for normal types. I'll be doing that soonish.

r? @nikomatsakis

@estebank
Copy link
Contributor Author

Visual output:

@arielb1
Copy link
Contributor

arielb1 commented Apr 11, 2017

@estebank
Shouldn't the "found type std::string::String" be highlighted as well?

@estebank
Copy link
Contributor Author

It only highlights differing parts, while dimming the parts that are the same. In that case, only the std::option::Option<...> part is relevant to the error. May be we can incorporate the elision logic without any highlighting change for the "composition" case?

@arielb1
Copy link
Contributor

arielb1 commented Apr 11, 2017

@estebank

I mean, I expected:


note: expected type `std::option::Option<std::string::String>`
                     ^^^^^^^^^^^^^^^^^^^^                   ^
      found type    `std::string::String`
                     ^^^^^^^^^^^^^^^^^^^

/// The type error output will behave in the following way:
///
/// ```text
/// Foo<Bar<Qux>
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax error: mismatched parentheses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

/// populate `other_value` with `other_ty`.
///
/// ```text
/// Foo<Bar<Qux>
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax error: mismatched parentheses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -35,6 +35,46 @@ pub struct SubDiagnostic {
pub render_span: Option<RenderSpan>,
}

#[derive(PartialEq, Eq)]
Copy link
Contributor

@arielb1 arielb1 Apr 11, 2017

Choose a reason for hiding this comment

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

Call this StyledString or something? There's already snippet::StyledString, so maybe have a Vec of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@nikomatsakis nikomatsakis 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 modulo test failures; @arielb1's thoughts on underlining seem potentially fine, but I could go either way (and even if we did want to do it, I'd be fine w/ leaving it for a later PR).

let l2 = lifetime_display(lifetimes.1);
if l1 == l2 {
values.0.push_normal("'_");
values.1.push_normal("'_");
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlike _, this is not legal syntax for a user to type, but I guess that's ok. Not sure what else to use, in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with your reasoning. I followed this path given that the compiler already has diagnostics that show '_ in some cases.

30 | Foo { bar: 1 }
| ^^^^^^^^^^^^^^ expected enum `std::option::Option`, found struct `Foo`
|
= note: expected type `std::option::Option<Foo>`
Copy link
Contributor

Choose a reason for hiding this comment

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

too bad we lose the highlighting here, but I don't really know how to fix that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... I've been thinking that we could expand ui errors to have a mode where ANSI escape codes are kept (.stderr_full? A comment flag in the .rs test file?).

@nikomatsakis
Copy link
Contributor

I see @arielb1 also posted some other suggestions that seem potentially good; e.g. using StyledString or what have you. I don't have a strong opinion. This didn't seem like a lot of code and it read fairly easily (thanks @estebank for the comments, the code seemed pretty easy to understand this time) but DRYer is always better.

@estebank estebank force-pushed the shorter-mismatched-types-2 branch 3 times, most recently from a1a2ff5 to f875e08 Compare April 11, 2017 22:44
Shorten mismatched types errors by replacing subtypes that are not
different with `_`, and highlighting only the subtypes that are
different.

Given a file

```rust
struct X<T1, T2> {
    x: T1,
    y: T2,
}

fn foo() -> X<X<String, String>, String> {
    X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
}

fn bar() -> Option<String> {
    "".to_string()
}
```

provide the following output

```rust
error[E0308]: mismatched types
  --> file.rs:6:5
   |
 6 |     X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `std::string::String`, found {integer}
   |
   = note: expected type `X<X<_, std::string::String>, _>`
                                 ^^^^^^^^^^^^^^^^^^^   // < highlighted
              found type `X<X<_, {integer}>, _>`
                                 ^^^^^^^^^             // < highlighted

error[E0308]: mismatched types
  --> file.rs:6:5
   |
10 |     "".to_string()
   |     ^^^^^^^^^^^^^^ expected struct `std::option::Option`, found `std::string::String`
   |
   = note: expected type `Option<std::string::String>`
                          ^^^^^^^                   ^  // < highlighted
              found type `std::string::String`
```
@estebank
Copy link
Contributor Author

@nikomatsakis I believe I addressed all the points (except the highlighting case brought up by @arielb1), as well as fixed a small presentational bug that was left from when I was first preparing this code.

@estebank
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Apr 12, 2017

📌 Commit 2389830 has been approved by nikomatsakis

TimNN added a commit to TimNN/rust that referenced this pull request Apr 12, 2017
…, r=nikomatsakis

Highlight and simplify mismatched types

Shorten mismatched types errors by replacing subtypes that are not
different with `_`, and highlighting only the subtypes that are
different.

Given a file

```rust
struct X<T1, T2> {
    x: T1,
    y: T2,
}

fn foo() -> X<X<String, String>, String> {
    X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
}

fn bar() -> Option<String> {
    "".to_string()
}
```

provide the following output

```rust
error[E0308]: mismatched types
  --> file.rs:6:5
   |
 6 |     X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `std::string::String`, found {integer}
   |
   = note: expected type `X<X<_, std::string::String>, _>`
                                 ^^^^^^^^^^^^^^^^^^^   // < highlighted
              found type `X<X<_, {integer}>, _>`
                                 ^^^^^^^^^             // < highlighted

error[E0308]: mismatched types
  --> file.rs:6:5
   |
10 |     "".to_string()
   |     ^^^^^^^^^^^^^^ expected struct `std::option::Option`, found `std::string::String`
   |
   = note: expected type `Option<std::string::String>`
                          ^^^^^^^                   ^  // < highlighted
              found type `std::string::String`
```

Fix rust-lang#21025. Re: rust-lang#40186. Follow up to rust-lang#39906.

I'm looking to change how this output is accomplished so that it doesn't create list of strings to pass around, but rather add an elided `Ty` placeholder, and use the same string formatting for normal types. I'll be doing that soonish.

r? @nikomatsakis
bors added a commit that referenced this pull request Apr 12, 2017
Rollup of 9 pull requests

- Successful merges: #41063, #41087, #41141, #41166, #41183, #41205, #41206, #41232, #41243
- Failed merges:
@bors bors merged commit 2389830 into rust-lang:master Apr 12, 2017
@estebank estebank deleted the shorter-mismatched-types-2 branch November 9, 2023 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants