-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Make error codes into strings #67086
Make error codes into strings #67086
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I am opposed to just using strings here -- that seems really error prone, and also harder to search for in tidy and the like. I would prefer that we add a |
@Mark-Simulacrum It comes after a discussion with @eddyb and @Centril. I guess I'll let them explain in details why. ;) |
I am opposed to doing anything here unless explicitly sanctioned by @rust-lang/wg-diagnostics and I apologize if there was any confusion about this on Discord. |
Even if we wanted to stringify these (and I don't think we do), the diagnostic macros could accept the code without having to surround it with quotes. |
src/libsyntax/diagnostics/macros.rs
Outdated
$crate::diagnostic_used!($code); | ||
$session.span_fatal_with_code( | ||
$span, | ||
&format!($($message)*), | ||
$crate::errors::DiagnosticId::Error(stringify!($code).to_owned()), | ||
$crate::errors::DiagnosticId::Error($code.to_owned()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an unnecessary change; we can keep the identifiers and continue using stringify!($code)
. The only thing that's important from a recompilation POV is that diagnostic_used!(...)
be removed or that if it exists for the purposes of tidy, that it shouldn't do anything (i.e. have an empty expansion).
The only thing that's important to me here is that we remove the dependency of |
That would also be fine by me since it means that we can still remove the |
☔ The latest upstream changes (presumably #67104) made this pull request unmergeable. Please resolve the merge conflicts. |
We're evolving towards strings identifiers like "[LifetimeError.Mismatch]". Therefore, strings make more sense to prepare for this switch. |
@GuillaumeGomez Well there seems to be a lack of consensus in favor of strings, so could we limit this PR for now to just the tidy check and removing references to |
Euh we can't really. Currently it's using consts so we have to pick something in order to remove the dependency. |
src/libsyntax/diagnostics/macros.rs
Outdated
@@ -1,112 +1,112 @@ | |||
#[macro_export] | |||
macro_rules! diagnostic_used { | |||
($code:ident) => ( | |||
($code:expr) => ( | |||
let _ = $code; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GuillaumeGomez I don't think this macro has any function at all before or after the PR. These constants are all marked as pub
inside librustc_error_codes
so they will not be deemed unused if diagnostic_used!(...);
is missing. Therefore the macro can be removed wholesale and any calls to it. Also, all you've done here is to replace stringify!($some_ident)
with string literals instead. Keeping the identifiers and continuing to use stringify!(...)
should work just as well.
Ping from triage: @GuillaumeGomez this PR is idle for a month, could you address Centril's review? |
I need to update it, indeed! |
c866e5f
to
4a18062
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
4a18062
to
0c5462b
Compare
Updated: however one issue remains: since error codes are now strings, the JSON rendering changes. How can I implement |
I fixed the dependency issue in #68353. Beyond that, it's not really clear to me what benefits the stringification brings, and there seems to be a lack of consensus in favor of that change for now. So I'll r? @Mark-Simulacrum @oli-obk @estebank. |
@GuillaumeGomez That's because the trait is named |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@Centril It appeared that error codes are used by people. So a solution to improve their usability was to allow to have "error codes" looking like "lifetime-mismatch" instead of "E0XXX". If we handle error codes with strings instead of enums, we can then use them however we want. |
@GuillaumeGomez I mean, it could just as well look like |
Rereading from the conversation, I'm not even sure that @rust-lang/wg-diagnostics came to a conclusion on this. So let's wait to hear from them? Or maybe you had some news on your side @eddyb ? |
I am not involved in anything where this came up since our Discord discussion, so no. |
Note that @rust-lang/wg-diagnostics is not really a group that holds regular meetings, has a well-defined membership, and whatnot. For now it's just @oli-obk, @estebank, @davidtwco, and myself. |
I think moving away from numeric error codes is a good thing, I do worry about the checks for duplicate use and undocumented errors though. It seems the current impl is very hardcoded to things starting with |
@Centril noted elsewhere (not sure where) that the dependency breaking part of this has already been done. It is not clear to me that this PR makes things better wrt to stringification, as a result of that. I think my earlier comment (#67086 (comment)) still stands. Splitting out the unused/definitely used checking code into a separate PR (or making that the only change in this PR) seems good, in theory, though I have not reviewed whether:
|
Ok, let's wait and see then! |
Got the comments after commenting... @Mark-Simulacrum I can send the check for the double error codes usage in another PR if you want? |
Sure, that'd be great. |
@oli-obk We can store the error codes inside an hashmap and make this check when throwing one, and then we could use this hashmap as reference for the possible duplicates? (even though I'm not completely sure how to correctly handle it, i.e. differentiating "normal" strings and error codes) |
@Mark-Simulacrum Ok, sending the PR this evening or tomorrow. |
☔ The latest upstream changes (presumably #68635) made this pull request unmergeable. Please resolve the merge conflicts. |
0c5462b
to
397e941
Compare
ping @oli-obk |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #61812) made this pull request unmergeable. Please resolve the merge conflicts. |
Reading the discussion on zulip (https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/get.20rid.20of.20error.20codes/near/187901869) I don't think we're ready to do this right now. What we definitely don't want is to have quoted strings in the macro, since any non-numeric names will just be snake case names and thus valid idents that can be stringified internally. So basically
|
@@ -3855,7 +3855,10 @@ dependencies = [ | |||
"rustc_ast_pretty", | |||
"rustc_attr", | |||
"rustc_data_structures", | |||
<<<<<<< HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad merge
Triaged |
Part of #67061.
I used this script to convert all error codes into strings:
r? @Centril