Experiment: add optional error codes to diagnostics

[davidmalcolm]

rustc has error codes to identify specific errors, with URLs documenting each error.

In GCC 13 I've extended GCC's diagnostic subsystem so that a diagnostic can be associated with zero or more diagnostic_metadata::rule instances, which have a textual description and a URL. I meant this for rules in coding standards and specifications, but it struck me that potentially gccrs could reuse the same error codes as rustc.

The following pull request implements an experimental form of this; I picked a single error at random: E0054.

With this patch, gccrs emits e.g.:

bad_as_bool_char.rs:8:19: error: invalid cast [u8] to [bool] [E0054]
    8 |   let nb = 0u8 as bool;
      |            ~      ^

where the trailing [E0054] is colorized, and, in a suitably capable terminal is a clickable URL to https://doc.rust-lang.org/error-index.html#E0054.

The error code is after the diagnostic message, whereas rustc puts it after the word "error". I could change that in gcc's diagnostic.cc, perhaps.

I'm not sure if this is a good idea (e.g. is it OK and maintainable to share error codes with rustc?), but thought it was worth sharing.

[CohenArthur]

Hey David, thank you for working on this. I think this is an extremly important step that we needed to get to at some point. I'm happy to see it being integrated directly into gcc and used for other frontends as well! One issue we're currently facing at https://github.com/Rust-GCC/testing is that we cannot go through rustc's testsuite's invalid testcases, precisely because we do not have the same exact error messages. Our options would be to have some sort of "lookup table" to match error messages from rustc with ours, or to reuse the same exact error codes. On top of that, I believe (and have read) multiple times that they are stable and won't change, so this is the most reliable method imo. I can't remember a source for this and I'm hoping @bjorn3 will come in with one :DD

I've tried the code and am a bit sad to see my terminal does not allow me to click the links, despite usually being pretty good at handling URLs. I guess this could be mitigated by adding the plain URL after the error message and the location? These are simply formatting concerns however, and me being nitpicky. Thank you for this work and sharing it with us! I really appreciate it!

[philberty]

We definitely want to start using the rust error codes. Early on in this project, I stayed away from them, at the time, the error code numbers were generated as part of an enum in rustic iirc. But now so much documentation and url's rely on the number convention that I think it seems safe to start using them now.

The changes you made to GCC are they upstream? We need to do a merge from upstream soon so maybe we should do that before merging? What do you think? @tschwinge

Also just want to say this is really cool thanks so much for this!

[tschwinge]

Ha, @davidmalcolm -- now you beat me to that one, great! I had seen your GCC master branch commit 0b14f59 "diagnostics: add ability to associate diagnostics with rules from coding standards", and immediatelly thought "let's use/extend that for linking to the rustc error codes". :-)

---

The changes you made to GCC are they upstream? We need to do a merge from upstream soon so maybe we should do that before merging? What do you think? @tschwinge

As we all know, my "soon" may easily expand into multiple weeks... ;'-\

So, I'm fine either way: either we wait with this -- or better: merge this as-is, including the "Partial commit of: [...]" commit as included here, and I'll later sort out any merge conflicts (which I assume will be trivial, given that we don't have any other changes in the diagnostics implementation, as far as I remember).

[CohenArthur]

As we all know, my "soon" may easily expand into multiple weeks... ;'-\

Let's not put extra pressure on ourselves :DD We all appreciate the time and effort you put into these merges very much, and I'd hate to see you feel bad about a couple of weeks :) You're allowed rest, especially since you have a real full time job to attend to instead of silly open source contributions :P No worries and thanks a lot Thomas

[davidmalcolm]

Ha, @davidmalcolm -- now you beat me to that one, great! I had seen your GCC master branch commit 0b14f59 "diagnostics: add ability to associate diagnostics with rules from coding standards", and immediatelly thought "let's use/extend that for linking to the rustc error codes". :-)

:-)

The changes you made to GCC are they upstream? We need to do a merge from upstream soon so maybe we should do that before merging? What do you think? @tschwinge

As we all know, my "soon" may easily expand into multiple weeks... ;'-\

So, I'm fine either way: either we wait with this -- or better: merge this as-is, including the "Partial commit of: [...]" commit as included here, and I'll later sort out any merge conflicts (which I assume will be trivial, given that we don't have any other changes in the diagnostics implementation, as far as I remember).

IIRC the only way in which 0b14f59 from gcc trunk didn't apply cleanly to the gccrs branch were minor changes to gcc/diagnostic-format-json.cc and to the new file gcc/diagnostic-format-sarif.cc. But it's probably best to wait until your next natural merging point.

The second patch, to add error_meta isn't in gcc upstream yet, but I can self-approve it (though maybe it should simply be another overload of error_at)

I introduced an ErrorCode class to wrap a string, but if there's a enum, probably best to just use that.

My other concerns were:

• this implies lots of tedious work going through all the errors, adding the error codes. I was interested in the hack of being able to emit error codes, but have no great desire to follow through and actually annotate them all (and I should be upfront about that). This seems like it adds a lot of work for only moderate benefit. From a technical point-of-view it's very easy work (say, for a newcomer???) - just very tedious.
• how would the rustc developers feel about gccrs piggybacking off of their codes?

[davidmalcolm]

I've tried the code and am a bit sad to see my terminal does not allow me to click the links, despite usually being pretty good at handling URLs.

It's using the standard proposed in: https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda using the mechanism I added in GCC 10 (e.g. for links to documentation about GCC warnings). Does the [-Wfoo] suffix for a regular GCC warning get URLified in the terminal for you?

Note that the testsuite adds -fdiagnostics-plain-output which (amongst other things) suppresses the URL control codes.

[CohenArthur]

this implies lots of tedious work going through all the errors, adding the error codes. I was interested in the hack of being able to emit error codes, but have no great desire to follow through and actually annotate them all (and I should be upfront about that). This seems like it adds a lot of work for only moderate benefit. From a technical point-of-view it's very easy work (say, for a newcomer???) - just very tedious.

This is something we need to do anyway if we want to be able to run rustc's testsuite as I mentionned. So I won't mind going through them and adding them as we go :) Plus, we can do that incrementally thanks to the overload.

how would the rustc developers feel about gccrs piggybacking off of their codes?

Very good question. We can ask on the internal forums to make sure it's all good, but I don't foresee this being an issue. Unless there are some concerns that I'm not aware of.

[philberty]

@davidmalcolm we have completed a merge from upstream GCC as of baa3ffb

Would you like to try and rebase your changes and see if they merge better? I would love to merge this code change in we have improved a lot of error handling in coercions and casts so we have simpler entry points for when we should emit errors.

[CohenArthur]

@davidmalcolm I took the liberty of rebasing your changes and fixing the conflicts, as well as formatting the code. Feel free to delete these commits if you'd like to do them differently :)

[philberty]

Maybe i am missing something but is this used? Just wondering should it just use a std::string instead of relying on xstrdup which will leak.

[philberty]

LGTM after reviewing this since make_description is an abstract method which is required in the core GCC code.

[CohenArthur]

bors r+

[bors]

Build succeeded:

• build-and-check-ubuntu-64bit