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

Teach Diagnostics to highlight text #38955

Merged
merged 1 commit into from Jan 20, 2017

Conversation

Projects
None yet
4 participants
@estebank
Copy link
Contributor

estebank commented Jan 10, 2017

Support styled Diagnostic output:

mismatched types error with colorized types in the note

Closes #37532 and #38901.

r? @nikomatsakis CC @jonathandturner @nagisa @nrc

@estebank estebank force-pushed the estebank:highlighted-diags branch from 40ba66c to b9a04cc Jan 10, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 10, 2017

Cool!

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 10, 2017

The travis failures look legit:

error[E0308]: mismatched types
  --> /checkout/src/librustc_driver/test.rs:82:30
   |
82 |         remove_message(self, &db.message, db.level);
   |                              ^^^^^^^^^^^ expected str, found struct `std::vec::Vec`
   |
   = note: expected type `&str`
   = note:    found type `&std::vec::Vec<(std::string::String, errors::snippet::Style)>`
   = help: here are some functions which might fulfill your needs:
 - .remove(...)
 - .swap_remove(...)

error[E0308]: mismatched types
  --> /checkout/src/librustc_driver/test.rs:84:34
   |
84 |             remove_message(self, &child.message, child.level);
   |                                  ^^^^^^^^^^^^^^ expected str, found struct `std::vec::Vec`
   |
   = note: expected type `&str`
   = note:    found type `&std::vec::Vec<(std::string::String, errors::snippet::Style)>`
   = help: here are some functions which might fulfill your needs:
 - .remove(...)
 - .swap_remove(...)
@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Jan 10, 2017

@nikomatsakis they're. I'll fix them but will have to wait for #38916 to be merged before approving this as I think there'll be merge conflicts between that PR and this one. How does the approach to this change look overall?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 10, 2017

@estebank seems pretty good to me.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 11, 2017

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 11, 2017

#38916 is now merged; r=me after rebase.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jan 11, 2017

I would not approach the problem in this way, I think that code which creates error messages should supply semantic information (e.g., 'this is a type') and the error handling code should decide how to format it (e.g., all types in notes will be highlighted). That allows easier customisation of error display in the compiler and allows more information to be reflected in JSON errors for IDEs and other tools which might want to do more sophisticated things than just highlight.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Jan 11, 2017

@nrc in principle I would agree with you, but I'm foreseeing cases where we want to highlight things that do not match one to one to semantic information, like the ones outlined in #21025 where'd it be nice to have something like the following output:

 expected type `core::result::Result<lossfunction::scenario::PredictionRecord, Box<misc::OMErrorT + 'static>>`
    found type `core::result::Result<&str, Box<misc::OMErrorT + 'static>>`

I can imagine this also being used for emphasis in places where asterisks might be used now. And of course, I feel that even though the feature is needed it should be used extremely sparingly.

As for IDEs, each segment of the path of every part of the expected and found types should potentially have links to their definition. I feel all that semantic information could be presented out of line (with span like information) in the JSON output for IDEs to be easily capable of using that type info, but at the same time have simple text for clients that do not wish to use it. Leaving this PR as merely a display embellishment, and I can create a new PR that allows to associate semantic type information to parts of the diagnostics' text.

@estebank estebank force-pushed the estebank:highlighted-diags branch 5 times, most recently from c99fe7f to 5b0efef Jan 11, 2017

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Jan 11, 2017

Rebased after #38916 and turned the expected/found into a single multiline note:

@estebank estebank force-pushed the estebank:highlighted-diags branch from 5b0efef to c56ec9a Jan 12, 2017

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jan 12, 2017

I'm foreseeing cases where we want to highlight things that do not match one to one to semantic information

This seems exactly like something that should have a semantic identification - something that indicates that it is a type and the element of a type which is of interest to the user.

As for IDEs, each segment of the path of every part of the expected and found types should potentially have links to their definition.

This is somewhat harder than it sounds. However, we are already outputting this kind of information for IDEs separately from the error messages, so there is no need to duplicate that info here (it might make sense to link in to that information by id or something, but that is probably future work).

We already sort of do what you describe next - we have a 'rendered' field in the JSON error which has the message rendered as the compiler would do. The rest of the JSON message is a structural representation of the error. I think it is best to avoid embellishing the structural elements with presentation information; this ought to be kept as plain data. The highlighting might have a place in the rendered version.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 12, 2017

@nrc

I would not approach the problem in this way, I think that code which creates error messages should supply semantic information (e.g., 'this is a type') and the error handling code should decide how to format it (e.g., all types in notes will be highlighted)

I considered this, but I thought this would be a reasonable step in that direction. That said, it wouldn't be so hard to make this semantic now, I suppose, just basically needs a new enum, right?


@estebank

I'm foreseeing cases where we want to highlight things that do not match one to one to semantic information, like the ones outlined in #21025 where'd it be nice to have something like the following output:

 expected type `core::result::Result<lossfunction::scenario::PredictionRecord, Box<misc::OMErrorT + 'static>>`
    found type `core::result::Result<&str, Box<misc::OMErrorT + 'static>>`

This isn't necessarily in conflict, is it? That is, you might imagine a semantic tag for "type" (that encloses the entire type in both instances) and a semantic tag for "diff" (that encloses the diffed parts, which you showed in bold).

@estebank estebank force-pushed the estebank:highlighted-diags branch from c56ec9a to 5cda1c4 Jan 12, 2017

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Jan 12, 2017

This isn't necessarily in conflict, is it?

True, @nikomatsakis.

Given that the current json output in this PR for the epected note is:

{
  "message": "expected type `usize`\n   found type `&'static str`",
  "code":null,
  "level":"note",
  "spans":[],
  "children":[],
  "rendered":null
}

Would it make sense to turn it into

{
  "message": "expected type `usize`\n   found type `&'static str`",
  "message_highlights": [
    {"start": 15, "end": 20},
    {"start": 37, "end": 49},
  ],
  "code":null,
  "level":"note",
  "spans":[],
  "children":[],
  "rendered":null
}

or, more closely to what @nrc advocates:

{
  "message": "expected type `usize`\n   found type `&'static str`",
  "message_annotations": [
    {"start": 15, "end": 20, "type": "type", "content": "usize"},
    {"start": 37, "end": 49, "type": "type", "content": "&'static str"},
  ],
  "code":null,
  "level":"note",
  "spans":[],
  "children":[],
  "rendered":null
}

@estebank estebank force-pushed the estebank:highlighted-diags branch from 5cda1c4 to 7578549 Jan 12, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 13, 2017

@estebank I'd be fine with either of those. It seems good to at least include the what kind of highlight it is. But overall I think it'd be nice to write up an RFC documenting what our JSON output ought to look like, and then implement it bit by bit. I'm wary because changes to the JSON are kind of "insta-stable" -- people can rely on them! So I might be inclined to leave the JSON alone until we at least come up with an end-to-end plan that is written down.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Jan 13, 2017

@nikomatsakis does it make sense then to merge this PR as is? The places that would need to change to allow for semantic meaning instead of style are self contained and should be easy to implement once a consensus on the expected output is reached in the RFC.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 13, 2017

@estebank to be clear, the highlights are not currently visible in the JSON form, right?

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Jan 13, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 17, 2017

@bors r+

OK, I say we land it for now, but we should start thinking about a more semantic markup (as @nrc suggested) and also about a richer JSON format.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 17, 2017

📌 Commit 7578549 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 17, 2017

⌛️ Testing commit 7578549 with merge 848b0a3...

bors added a commit that referenced this pull request Jan 17, 2017

Auto merge of #38955 - estebank:highlighted-diags, r=nikomatsakis
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
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 17, 2017

💔 Test failed - status-appveyor

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 17, 2017

@estebank estebank force-pushed the estebank:highlighted-diags branch from 7578549 to fc774e6 Jan 17, 2017

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Jan 18, 2017

@nikomatsakis fixed.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Jan 19, 2017

@nikomatsakis, @nrc, created rust-lang/rfcs#1855 for the metadata annotations information.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 19, 2017

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 19, 2017

📌 Commit fc774e6 has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 19, 2017

@estebank nice, thanks.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 19, 2017

Rollup merge of rust-lang#38955 - estebank:highlighted-diags, r=nikom…
…atsakis

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 rust-lang#37532 and rust-lang#38901.

r? @nikomatsakis CC @jonathandturner @nagisa @nrc

bors added a commit that referenced this pull request Jan 20, 2017

bors added a commit that referenced this pull request Jan 20, 2017

bors added a commit that referenced this pull request Jan 20, 2017

bors added a commit that referenced this pull request Jan 20, 2017

bors added a commit that referenced this pull request Jan 20, 2017

bors added a commit that referenced this pull request Jan 20, 2017

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 20, 2017

⌛️ Testing commit fc774e6 with merge b7ca2b9...

bors added a commit that referenced this pull request Jan 20, 2017

Auto merge of #38955 - estebank:highlighted-diags, r=nikomatsakis
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
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing b7ca2b9 to master...

@bors bors merged commit fc774e6 into rust-lang:master Jan 20, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

bors added a commit that referenced this pull request Jan 20, 2017

bors added a commit that referenced this pull request Jan 20, 2017

bors added a commit that referenced this pull request Jan 20, 2017

bors added a commit that referenced this pull request Jan 20, 2017

bors added a commit that referenced this pull request Jan 21, 2017

killercup added a commit to killercup/rust-clippy that referenced this pull request Jan 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment