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

Make the expected/found notes a first class comment #38901

Closed
estebank opened this issue Jan 7, 2017 · 13 comments
Closed

Make the expected/found notes a first class comment #38901

estebank opened this issue Jan 7, 2017 · 13 comments

Comments

@estebank
Copy link
Contributor

estebank commented Jan 7, 2017

Instead of using notes for expected type/found type information when there are type mismatches, present something closer to:

error[E0308]: mismatched types
  --> ../../src/test/ui/mismatched_types/issue-38371.rs:26:9
   |
26 | fn agh(&&bar: &u32) {
   |         ^^^^ expected u32, found reference
   |
   = expected type `u32`
   =    found type `&_`
@nagisa
Copy link
Member

nagisa commented Jan 7, 2017

What’s the point of this. How is this would be an improvement enough to special case this error in particular, with us paying in inconsistency credits?

@sanmai-NL
Copy link

Could you describe the problem and solution a bit clearer? Just wondering.

@estebank
Copy link
Contributor Author

estebank commented Jan 7, 2017

@sanmai-NL, given a file

fn main() {
    let x: usize = "";
}

provide the following output

error[E0308]: mismatched types
 --> file.rs:2:20
  |
2 |     let x: usize = "";
  |                    ^^ expected usize, found reference
  |
  = expected type `usize`
  =    found type `&'static str`

instead of

error[E0308]: mismatched types
 --> file.rs:2:20
  |
2 |     let x: usize = "";
  |                    ^^ expected usize, found reference
  |
  = note: expected type `usize`
  = note:    found type `&'static str`

@nagisa, I feel that this is a common enough case that it should have its own presentation with (arguably) less clutter. Special casing this would also allow to express in the json for tooling to use as needed for specialization without having to inspect the notes to figure out which one contain the types they might want to use. Special casing these also opens the door to doing something like:

error[E0308]: mismatched types
 --> file.rs:2:20
  |
2 |     let x: usize = "";
  |                    ^^ expected usize, found reference
  |
  = expected type `usize`
  =    found type `&'static str`

With json output of

{"expected": "type `usize`", "found": "type `&'static str`"}

@Mark-Simulacrum
Copy link
Member

Both examples look identical to me? Unless I am missing something.

@nagisa
Copy link
Member

nagisa commented Jan 8, 2017

@Mark-Simulacrum the proposed output is missing note: before messages.


I still fail to see any benefit here. JSON output can easily avoid outputting note without inflicting inconsistent output to the TUI output as well; and that’s what happens already:

    "children": [
        {
            "message": "expected type `usize`",
            "code": null,
            "level": "note",
            "spans": [],
            "children": [],
            "rendered": null
        },
        {
            "message": "   found type `&'static str`",
            "code": null,
            "level": "note",
            "spans": [],
            "children": [],
            "rendered": null
        },
        {
            "message": "here are some functions which might fulfill your needs:\n - .len()",
            "code": null,
            "level": "help",
            "spans": [],
            "children": [],
            "rendered": null
        }
    ],

@estebank
Copy link
Contributor Author

estebank commented Jan 8, 2017

@nagisa: #38902 has a proposed implementation for this. The code with that PR produces the following colorized output:

and json format:

  "children":[
    {
      "message":"type `usize`",
      "code":null,
      "level":"expected",
      "spans":[],
      "children":[],
      "rendered":null
    },
    {
      "message":"type `&'static str`",
      "code":null,
      "level":"found",
      "spans":[],
      "children":[],
      "rendered":null
    },
    {
      "message":"here are some functions which might fulfill your needs:\n - .len()",
      "code":null,
      "level":"help",
      "spans":[],
      "children":[],
      "rendered":null
    }]

In contrast with the current colorized output:

@nagisa
Copy link
Member

nagisa commented Jan 8, 2017

I guess instead of asking why (as I’m not receiving the answer anyway), I should say what I find wrong with this proposal (and associated PR).

  • I see no reason to remove note:
    • It makes no sense to me that you’re removing note in order to add some highlight to a message;
    • Removing the note makes this note inconsistent compared to every other note out there, even though its still just a note;
  • It makes no sense to me that you’re special-casing this particular message instead of adding a general mechanism to add highlights to arbitrary locations within notes;
  • I do not want my terminal to become a rainbow – the messages are already plenty colourful;
    • I do not have any issues with finding the current messages in the terminal, thus I fail to see benefit of this change;
    • And I so far have seen exactly 0 motivation that justifies making my terminal a rainbow.
  • level: expected and level: found in JSON format seems wrong:
    • What should happen instead is proper machine readable errors (e.g. {error: "E0308", expected: "usize", expected_concrete: "usize", found: "&", found_concrete: "&'static str"}) or something similar;
    • Any json consumer ought not be parsing the messages as those can change at any time and are as free-form as output from the compiler gets;
    • And I fail to see how this change helps to generate proper machine readable errors in general case as well.

So all in all, I feel like the idea is good, but it is misdirected. Rather than tweaking a single error, we ought be thinking about how to achieve all the desirables (even-more-structured errors, ability to add highlights to arbitrary parts of the error message, etc) for all the stuff compiler may report.

cc @nrc @jonathandturner has there been any thinking done on machine parsable errors? Not the json that we have now (its still pretty much free-form), but something more structured.

@estebank
Copy link
Contributor Author

estebank commented Jan 8, 2017

@nagisa, sorry if it seemed like I was evading the question, I was with sporadic time to dedicate to this :-/

The original reason to create the issue was a comment in PR #38605 by @nikomatsakis that made me remember that I had been thinking about the possibility to remove the note: before the type errors. I created the PR first for myself to see how it'd actually look in real life, and second to receive precisely this kind of feedback :)

Reason to remove note for mismatched types:

My thinking is removing as much text from the output as possible is a worthy goal, as with less text to read from the faster it is to figure out what is going on.

It makes no sense to me that you’re removing note in order to add some highlight to a message;

It's true, highlighting of parts of a message are more generally useful than this case in particular. I had filed #37532 sometime ago precisely for this feature, I'd forgotten to get back to it.

Removing the note makes this note inconsistent compared to every other note out there, even though its still just a note

In my mind this shouldn't be (at least visually) a note, as the two lines are conceptually part of the same thing, so it'd be nice if it was seen that way in the output.

I do not want my terminal to become a rainbow – the messages are already plenty colourful;

In the PR I used Style::UnderlinePrimary to make it very evident, but personally I'd think just bolding the type is highlighted enough, without having a new color going around. I understand that more colors is not something that everyone wants, but I feel highlighting the most important part of the message is useful, specially for newcomers.

level: expected and level: found in JSON format seems wrong

I agree with this. Part of the intention was to be as undisruptive as possible while incorporating the new output.

@nrc
Copy link
Member

nrc commented Jan 9, 2017

Thoughts:

  • I prefer the message without the "note" in it - I agree that the expected/found is pretty much part of the message, not a separate note
  • I'm keen to reify this information explicitly in the JSON output, but that means having explicit expected/found support, rather than hacking implicit meaning on to messages by setting the level field (see @nagisa's comment for that).
  • I'm not sure about the colorisation/bolding. There is already too much of this in the error messages IMO, and it is making them harder to read, not easier, to add more. OTOH, I do find the example with highlighted types nicer to read, so I'm not really sure...

@nagisa
Copy link
Member

nagisa commented Jan 9, 2017

I prefer the message without the "note" in it - I agree that the expected/found is pretty much part of the message, not a separate note

I feel like that may apply to most of the messages… So why don’t we just remove note (and maybe even help) from all the messages?

@estebank
Copy link
Contributor Author

estebank commented Jan 9, 2017

I have a PR out to support multiline messages with a proper margin that might help find a compromise between the current proposal and the existing output. How would people feel with the following?

error[E0308]: mismatched types
 --> file.rs:2:20
  |
2 |     let x: usize = "";
  |                    ^^ expected usize, found reference
  |
  = note: expected type `usize`
             found type `&'static str`
  = help: here are some functions that might fulfill your needs:
          - len()

That output would happen if that PR lands and we change the two notes with only one multiline note, instead of two notes. This way we deal with a couple of the problems I saw:

  • only one note for what conceptually is one thing
  • tools can show these two messages now as a single unit, instead of two separate notes without any special casing (but now they have to be aware about multiline notes, then again they already needed to do so for the function suggestion help)
  • (marginally) less text in the output

While also maintaining consistency with the other notes. This doesn't look into the json output side of things.

@nikomatsakis
Copy link
Contributor

My two cents:

The multi-line version looks good to me (as does the one w/o the note:). I don't like the current display.

Another thing I have always wanted to do with these messages is to add richer type-diffs that server to draw the eye to the real distinction (often the types are long and the difference is just a small part). But I guess we can pursue that either way.

@estebank
Copy link
Contributor Author

estebank commented Jan 9, 2017

I have always wanted to do with these messages is to add richer type-diffs that server to draw the eye to the real distinction

Issue #21025 raises that point.

bors added a commit that referenced this issue Jan 20, 2017
Teach Diagnostics to highlight text

Support styled `Diagnostic` output:

<img width="469" alt="mismatched types error with colorized types in the note" src="https://cloud.githubusercontent.com/assets/1606434/21871227/93a84198-d815-11e6-88b1-0ede3c7e28ef.png">

Closes #37532 and #38901.

r? @nikomatsakis CC @jonathandturner @nagisa @nrc
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

No branches or pull requests

6 participants