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

Add metadata to diagnostic messages' json output #1855

Closed
wants to merge 1 commit into from

Conversation

estebank
Copy link
Contributor

Annotate the diagnostic messages with metadata for tools and terminal
colorized output.

Rendered.

Annotate the diagnostic messages with metadata for tools and terminal
colorized output.
@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. labels Jan 19, 2017
@nikomatsakis nikomatsakis self-assigned this Jan 19, 2017
@nikomatsakis
Copy link
Contributor

I am in favor of the ideas here, but I think we should go ahead and expand the scope of the RFC slightly to cover the JSON format in full. As far as I know we have no RFC on this topic, right? The closest thing I can find is this internals discussion, but I also found an old sketch of a JSON format proposal that @nrc and I mocked up, which I copied into a gist.

@nikomatsakis
Copy link
Contributor

Also, the need for having a "fully rendered" version in the JSON, in addition to the semantic markup, came up in a random thread recently.

@nagisa
Copy link
Member

nagisa commented Jan 19, 2017

I already expressed my optinion elsewhere; I’ll reproduce it here for posterity.

I think at this point everybody would be better served by actually structured json errors. Not the sort of messages that are still pretty much a text with attached span, and now, some other data, but fully structured stuff that could be inspected programatically. Something like this:

{ code: "0308", found_type: "...", expected_type: "...", found_concrete: "...", expected_concrete: "...", ... }

with accompanying stable library to figure out these structured jsons into textual representations if necessary.


IMO The problem here is more general. Tools (probably?) want to inspect semantic information in the error messages. They should be able to do it without parsing every single variant of the error message rustc could emit. Adding metadata helps with the issue only marginally (“hey, look these parts might be interesting, but its still up to you to figure out stuff”), and it seems to me like the RFC is proposing to work around the actual problem instead of really solving it.

@nikomatsakis
Copy link
Contributor

@nagisa Hmm, I think that could be a useful component to the message, but I think many tools will want to work generically across all messages without having to understand them semantically.

It seems to me that there is little harm in stuffing tons of data into these JSON messages. At least that has been our theory thus far. So I suspect we could start with a relatively generic format, close to what we have, but that doesn't preclude us from adding richer semantic content later, right?

@nagisa
Copy link
Member

nagisa commented Jan 19, 2017

but I think many tools will want to work generically across all messages without having to understand them semantically.

That’s where a shared library with something like

extern crate rust_json_diagnostics;
// Could even be a C API in this same rust_json_diagnostics library that’s distributed as a shared and static library.
extern fn make_it_string(json: Read) {
    let diags: Vec<Diagnostic> = rust_json_diagnostics::parse(json);
    let nicely_formatted = format!("{]", diags);
}

in it would come in.

@nikomatsakis
Copy link
Contributor

@nagisa but (a) not all consumers are in Rust and (b) even those that are will not necessarily want to be recompiled to work with new versions of rustc that have more errors than the old versions, right?

@nagisa
Copy link
Member

nagisa commented Jan 19, 2017

(a) not all consumers are in Rust

That’s why extern fn and not just any fn. While that still requires user to be able use C API in some way, it does not require the consumer be Rust code.

(b) even those that are will not necessarily want to be recompiled to work with new versions of rustc that have more errors than the old versions

I guess that’s true to some extent. The whole point of dylibs is to make them match the rest of the stuff on the system and I don’t see an usecase in having something like such library and rustc installation separate.

@nikomatsakis
Copy link
Contributor

OK, this RFC isn't getting much attention. Let me see if I can get it back off the ground. There have so far been two major comments here:

  1. I suggested that the scope of the RFC is too limited, and that it would be good to document the JSON format in its entirety. In theory we declared it "stable-ish" as part of the move towards the newer error formatting, but I don't know of any clear documentation. I know that @nrc, @jonathandturner, and I had some discussions about this at one point, and I reproduced two links:

I think the key ingredient from my POV was that we include both semantic and presentation information. I envisioned the presentation output being something like an "html-ified" version of rustc's output, except with some additional semantic content (e.g., for each line in a snippet, we should give the filename/line-number that it is quoting, and also indicate what is the error-code and so forth -- you could think of this as "HTML-like tags" w/ descriptive classes and attributes). In particular, we want to give consumers the ability to easily reproduce rustc's diagnostic output if they wanted, and to have that reproduction keep up as the compiler changes. I think there are a number of use cases for this:

  • The emacs mode would like to 'forward' rustc's output, but highlight errors so users can jump to them; we also need an easy way to know whether a message is an error, warning, etc. We could also allow for easy links for error codes and the likes.
  • Cargo would like to capture and filter output from dependencies and decide whether to show it at all based on its contents; on windows, because formatting is not done with control codes, this would cause formatting to get lost.
  1. @nagisa proposed that we only give structured information (we do however have to maintain some semblance of backwards compatibility, so we'd have to preserve what we have). We'd then offer a rust library with a C interface to help parse and produce the rendered form. I think this is an interesting idea, and it might make sense to refactor the compiler to extract such logic into a reusable library, but for most clients having to link to C code is a very high burden. e.g., I would not want to inject such a dependency into an emacs mode, and I imagine most IDEs would find it burdensome, compared to just parsing some JSON. C dependencies in general a drag for distribution purposes.

cc @rust-lang/tools -- thoughts?

@estebank, I'd be happy to work with you to expand the RFC slightly, or perhaps propose such edit, if you like.

@alexcrichton
Copy link
Member

Ideally I'd love to take both routes simultaneously. I think it makes sense to include a sort of visual markup in the error message so tools can implement the rendering however they like, and then it'd also be neat to include a library to parse these messages that does it all for you (either with a Rust or a C API).

Both, however, seem like very ambitious projects that seems unlikely to reach completion any time soon. I would be skeptical that anything short of full-blown HTML would be suitable for our purposes in terms of being maximally forwards-compatible while also representing errors faithfully. Similarly I can see how maintaining such a library would be burdensome on us as it's just yet more API stability we have to consider.

My personal preference would be to be as conservative as possible here. Let's only expose semantic information which is easily provided far into the future, and then we can see what builds up externally to work with these messages and display them.

@nikomatsakis
Copy link
Contributor

OK, so @alexcrichton I think you're saying you'd be happy to go with the RFC roughly as is? I can get behind that. I do agree it'd take a bit of iteration and experimentation to find a JSON format we are happy with.

@alexcrichton
Copy link
Member

I'm not intimately familiar with the error message/json message area this RFC is modifying, but yes to me it looks like a semantic extension that we should always be able to implement, so I'd be in favor of this.

@killercup killercup mentioned this pull request Mar 3, 2017
@aturon
Copy link
Member

aturon commented Apr 29, 2017

This RFC appears to've stalled. @alexcrichton, @nikomatsakis, from a quick read it seems like you are both roughly in favor of heading in the direction of this RFC? Either of you want to move toward FCP?

@alexcrichton
Copy link
Member

I fear that I've mostly just commented on this RFC in an "advisory capacity" in the sense that I don't personally have enough understanding of all the nuances here to propose an FCP. I would basically just being doing so because it "seems good" and it's conservative enough to not cause undue maintenance burden if we change course later.

I continue to also think that a "markup" version of compiler error messages, while mega useful, need not block this RFC as it's compatible.

@alexcrichton
Copy link
Member

Oh I also mean to cc @matklad with his IntelliJ eyes

@matklad
Copy link
Member

matklad commented Apr 29, 2017

I would love to see some concrete examples of tools and use cases in the RFC. My gut feeling is that this actually adds relatively little value. We don't use JSON errors in IntelliJ, so having more semantic information won't be useful for us.

If you want to go really fancy with errors, chances are you need something like compiler as an API / RLS to ask some custom questions, to get info beyond that presented in the error message directly, or to query about valid code.

@aturon
Copy link
Member

aturon commented May 1, 2017

cc @jonathandturner @nrc

@estebank
Copy link
Contributor Author

estebank commented May 2, 2017

@matklad what does IntelliJ use to display type and lifetime errors? Or, rather, what does IntelliJ use rustc for? Originally my intent was to allow vim/emacs plugins to have better visibility into the compiler output, but this might be moot given the RLS might be a better place to expose this.

@matklad
Copy link
Member

matklad commented May 2, 2017

@matklad what does IntelliJ use to display type and lifetime errors? Or, rather, what does IntelliJ use rustc for?

Currently, we just present the usual text output from the compiler/cargo and then hyperlink file locations in the text output, much like Emacs compilation mode.

We don't display errors inline with the code, so we don't need spans. We could have done inline errors, but I believe that it's fundamentally wrong to launch compilation of the whole project on every code modification (even with a proper incremental compilation, although I can be convinced otherwise on this particular point :) ), and there's no way to get errors for a subset of files on demand at the moment.

@nikomatsakis
Copy link
Contributor

The RLS as it is doesn't, I don't think, have an adequate way of exposing error messages -- mostly because the language server protocol is too simplistic, I think (i.e., it doesn't support the richly structured errors that Rust produces). @jonathandturner or @nrc could confirm though. But I imagine we'll have to grow such a thing, and then it might make sense to have emacs or vi communicate with that (I've been meaning to play around with the various RLS/emacs integrations that are out there and see how they work...)

@nrc
Copy link
Member

nrc commented May 3, 2017

The RLS as it is doesn't, I don't think, have an adequate way of exposing error messages

Confirmed, it is very restricted.

@nrc
Copy link
Member

nrc commented May 3, 2017

As @nagisa said, I'd rather have a properly structued error, than one with annotations. I would also like to have the fully rendered output - we have a field for it already, but don't use it much.

@nrc nrc removed the T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. label May 17, 2017
@aturon
Copy link
Member

aturon commented Sep 6, 2017

Ping @nrc and @nikomatsakis, what's up with this RFC?

@aturon
Copy link
Member

aturon commented Feb 7, 2018

I'm going to close this RFC as inactive.

Stakeholder, please feel free to reopen if you'd like to pick back up this topic!

@aturon aturon closed this Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants