Give all fatals, errors, and warnings unique diagnostic codes #15336
Conversation
|
I'm really excited to see something like this land. |
| use syntax::parse::token; | ||
|
|
||
| local_data_key!(registered_diagnostics: RefCell<HashMap<Name, Option<Name>>>) | ||
| local_data_key!(used_diagnostics: RefCell<HashMap<Name, Span>>) |
There was a problem hiding this comment.
What are the alternatives to using local data here? rustc is already filled with global state and it would be better not to make the situation worse.
There was a problem hiding this comment.
Indeed, I wasn't too happy about this either. One way would be to extend the plugin infrastructure to allow plugins to carry state around. But I'm not sure if that's necessarily better than keeping it in TLS.
Another way would be to have the diagnostic table compile down to a struct definition:
struct X {
A0001: &'static str
}which still preserves the static checks but the error messages for say duplicate or unknown diagnostic codes would be pretty terrible.
|
This looks pretty awesome, but can you explain the architecture some? In particular I don't understand the role of libdiagnostics as a separate crate. It looks like a plugin, but then rustc includes a number of files from the other crate directly. |
|
This appears to load a set of diagnostics specifically for |
|
Oh, I see you addressed the libdiagnostics oddities in the OP. FWIW I also think that doing any built-in functionality as a plugin would be a bootstrapping nightmare. |
|
OK, after reading over this a number of times, I'm quite happy with it. My qualms are:
|
|
Oh, also, by registering three of these macros at rustc startup, AIUI that means they will be exposed to all crates, not just syntax and rustc. We need to avoid that. |
|
Oh, hm, I guess there are a few ways to look at the organization of error codes. With the organization you've laid out here one can put error code definitions near their use, but that doesn't mean every module's errors need to be namespaced (i.e. even if we put diagnostics defs in many places they can all be called "E####"). Still though, I tend to think putting the defs in one place will make their maintenance easier. I hope this information can be filled in by casual and newbie developers, and having to look all over the compiler for the right place to document an error seems more difficult than opening up the one canonical diagnostics file. |
|
@brson Thanks for the review! I agree it's pretty confusing to have libdiagnostics and not have it be a proper crate. I think I could just move all of its contents into libsyntax. |
|
@brson I suppose the bootstrapping process could stay the same if libdiagnostics was only linked in to librustc at stage1 and higher. At stage0 rustc could always use dummy macros. That would solve both the crate separation and the macro visibility problems. |
|
@brson Okay, I moved libdiagnostics into libsyntax and also made rustc only have one diagnostic table for the whole crate. The reliance on TLS is still there. I'm currently looking into if we could still turn it into a dynamically loaded plugin with some changes to the build system so that the macros are not visible to the user but maybe it shouldn't block this PR? |
|
I suppose a temporary solution to the visibility problem is that the driver could have an "are we compiling rustc" guard for those macros (and no, not anything like Ken Thompson's compiler hack :-)) but that to me is slightly more terrible than maybe just namespacing them. |
|
@jakub- Thanks and sorry for the delay. If you rebase, and prefix the macros with double-underscore as a temporary 'dont-use-this' indicator, I'll be happy. |
|
@brson Done! |
|
Just to be 100% safe, could you put these behind some oddly named feature gate as well? Other than that, this looks good to me! |
|
@alexcrichton I would love to! However, feature gates live in rustc and syntax extensions only depend on libsyntax. Should libsyntax (and ParseSess, specifically) also know about feature gates, you think? |
|
@jakub- you can check in a similar manner to how the (You could also just conditionally load the macros based on the feature gate.) |
|
@huonw Right, of course. I ended up following your last suggestion and also added some tests. |
|
@jakub- If you want to continue this line of development you might consider #8161 |
This is a continuation of @brson's work from #12144. This implements the minimal scaffolding that allows mapping diagnostic messages to alpha-numeric codes, which could improve the searchability of errors. In addition, there's a new compiler option, `--explain {code}` which takes an error code and prints out a somewhat detailed explanation of the error. Example: ```rust fn f(x: Option<bool>) { match x { Some(true) | Some(false) => (), None => (), Some(true) => () } } ``` ```shell [~/rust]$ ./build/x86_64-apple-darwin/stage2/bin/rustc ./diagnostics.rs --crate-type dylib diagnostics.rs:5:3: 5:13 error: unreachable pattern [E0001] (pass `--explain E0001` to see a detailed explanation) diagnostics.rs:5 Some(true) => () ^~~~~~~~~~ error: aborting due to previous error [~/rust]$ ./build/x86_64-apple-darwin/stage2/bin/rustc --explain E0001 This error suggests that the expression arm corresponding to the noted pattern will never be reached as for all possible values of the expression being matched, one of the preceeding patterns will match. This means that perhaps some of the preceeding patterns are too general, this one is too specific or the ordering is incorrect. ``` I've refrained from migrating many errors to actually use the new macros as it can be done in an incremental fashion but if we're happy with the approach, it'd be good to do all of them sooner rather than later. Originally, I was going to make libdiagnostics a separate crate but that's posing some interesting challenges with semi-circular dependencies. In particular, librustc would have a plugin-phase dependency on libdiagnostics, which itself depends on librustc. Per my conversation with @alexcrichton, it seems like the snapshotting process would also have to change. So for now the relevant modules from libdiagnostics are included using `#[path = ...] mod`.
This is a continuation of @brson's work from #12144.
This implements the minimal scaffolding that allows mapping diagnostic messages to alpha-numeric codes, which could improve the searchability of errors. In addition, there's a new compiler option,
--explain {code}which takes an error code and prints out a somewhat detailed explanation of the error. Example:I've refrained from migrating many errors to actually use the new macros as it can be done in an incremental fashion but if we're happy with the approach, it'd be good to do all of them sooner rather than later.
Originally, I was going to make libdiagnostics a separate crate but that's posing some interesting challenges with semi-circular dependencies. In particular, librustc would have a plugin-phase dependency on libdiagnostics, which itself depends on librustc. Per my conversation with @alexcrichton, it seems like the snapshotting process would also have to change. So for now the relevant modules from libdiagnostics are included using
#[path = ...] mod.