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

New "full compiler diagnostic" view could use some color #13648

Closed
jplatte opened this issue Nov 19, 2022 · 14 comments · Fixed by #13848
Closed

New "full compiler diagnostic" view could use some color #13648

jplatte opened this issue Nov 19, 2022 · 14 comments · Fixed by #13848
Assignees
Labels
C-feature Category: feature request

Comments

@jplatte
Copy link
Contributor

jplatte commented Nov 19, 2022

Thanks @Veykril for adding this nice feature in #13633! How hard would it be to use the same colors in the compiler diagnostic view that you also get when seeing the same error in the terminal?

@Veykril
Copy link
Member

Veykril commented Nov 19, 2022

Pretty hard given the rendered output in the json output doesn't contain the color, and reconstructing that ourselves isn't really an option.
cc @estebank

@HKalbasi
Copy link
Member

Reconstructing the error isn't that hard, using libraries like ariadne (I did it one time for evcxr, jupyter kernel for rust) but given that r-a doesn't need to change line numbers and such, it is way more reasonable to ask rustc for colored output inside json.

@bjorn3
Copy link
Member

bjorn3 commented Nov 19, 2022

Rustc supports outputting ansi color codes in the json messages and in fact that is what cargo itself uses. Not sure if cargo supports passing this through, but if it does you could parse the ansi escape codes to get the colors.

@Veykril
Copy link
Member

Veykril commented Nov 20, 2022

Ye i think cargo might be eating the colors here then from what I've seen.

@estebank
Copy link
Contributor

Adding an HTML formatted output option to enum HumanReadableErrorType1 and using that instead in json_rendered2 (if a flag is set) then you wouldn't have to do any of these hacks (other than maybe supplying CSS).

@ian-h-chamberlain
Copy link
Contributor

ian-h-chamberlain commented Nov 29, 2022

As a not-optimal workaround, I found a way to get back the colors that Cargo displays by default (in VSCode at least):

  "rust-analyzer.checkOnSave.overrideCommand": [
    "cargo",
    "check",
    "--message-format=json-diagnostic-rendered-ansi"
  ],

Then use an extension like https://marketplace.visualstudio.com/items?itemName=iliazeus.vscode-ansi to show the rendered colors. Unfortunately it seems like there is no native support in the VSCode buffer view (microsoft/vscode#38834) so it would probably require effectively re-implementing what they've done in that extension to render the ANSI codes.

Not sure how other editors might handle this kind of scenario, but for ones that do support rendering ANSI codes natively, a simple flag to switch between message-formats might be nice to have.

@Veykril
Copy link
Member

Veykril commented Nov 29, 2022

Other editors don't have this feature, and yes, I would expect us to implement ANSI code parsing and effectively transforming things into html or similar that we can emit in a colored fashion.

@estebank
Copy link
Contributor

We should bite the bullet and provide a --message-format=json-diagnostic-rendered-html on the rustc side. If we gave you annotated spans that would be enough for you to provide your own CSS, right?

@Veykril
Copy link
Member

Veykril commented Nov 29, 2022

Ye we should be able to provide the css there

@ian-h-chamberlain
Copy link
Contributor

In the example of the above extension, they actually use a text decoration provider rather than any CSS/HTML output. I suppose if the intention is to eventually switch to a webview or something like that, the HTML/CSS would be fine, but for the existing editor implementation it would have to then translate from HTML to the VSCode text decoration API, right? Is there a significant different parsing ANSI codes (already available with cargo) vs HTML (not yet implemented)?

@Veykril
Copy link
Member

Veykril commented Nov 30, 2022

Well there are two ways to solve this. Either via a normal text editor colored with decorators/custom syntax highlighting or via a webview that renders the html.

@jonas-schievink jonas-schievink added the C-feature Category: feature request label Nov 30, 2022
@ian-h-chamberlain
Copy link
Contributor

I am interested in working on this and actually made some decent progress on a proof of concept, but I had some questions around licensing and whether this is actually the desired approach... I posted over on Zulip but haven't heard there so thought I'd repost on this issue to try and get some input here:

  1. Is it worth pursuing the ANSI escape code / text decoration approach, or should I hold off in favor of a future webview / HTML approach?
  2. For proof-of-concept, I've copied a bunch of code from https://github.com/iliazeus/vscode-ansi (MIT licensed) — is it viable to include this in the final product with a copy of their license, or would I need to basically reimplement it from scratch (or find an alternative npm dependency that has similar functionality)? I couldn't find any other "vendored" code in the repo (and rather few npm deps) so wondering what the expectation /policy on that might be

@Veykril
Copy link
Member

Veykril commented Dec 19, 2022

Sorry for not receiving any replies regarding this for so long.

The ANSI approach seems fine to me, both approaches should be feature equivalent for our purposes. If there is no simple npm package that offers the relevant things we'd want here then copying from that extension seems fine imo, ideally keeping the copied parts in a separate ts file that we can put the license together (so a subdir in src) to make the licensing stuff clear there. That would also simplify removing that again should we move to the html output at some point.

@ian-h-chamberlain
Copy link
Contributor

ian-h-chamberlain commented Dec 25, 2022

Cool! I ended up finding a small package that works well enough and basically rewrote everything I had been using from vscode-ansi so I don't believe there should be any issues there. Just opened a PR with the initial implementation I came up with.

Edit: Oops I guess I should have done this sooner:

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: feature request
Projects
None yet
7 participants