-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Move error codes #66314
Move error codes #66314
Conversation
Some changes occurred in diagnostic error codes |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -1,7 +1,7 @@ | |||
#[macro_export] |
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.
The remainder of this file should probably be moved to librustc_errors
?
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.
They seem to be "independent"? A bit weird...
@@ -16,6 +16,8 @@ use std::fmt::{self, Display}; | |||
use syntax::{attr, symbol::sym}; | |||
use syntax_pos::Span; | |||
|
|||
use rustc_error_codes::*; |
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.
Hmm; why do this in each of the files? Shouldn't we change the diagnostics macros to not use constants at all but rather to just do the stringification? Tidy can do the validation later to make sure that there are no unused error codes or references to codes not registered. This way we don't have to change any of the Cargo.toml
files here except for librustc_interface.
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.
I don't agree with this: it allows us to be sure that the used error codes exist at compile time. If we switch to strings, we won't be able to have this check anymore (or at least, not at compile time).
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.
I'm confused... In the issue you noted that doing what you are doing here would make the recompilation issue worse, so what changed? -- Tidy is part of "compile time" for the purposes of CI so moving the checks there still means problems are rejected. Moreover, eventually and ideally we want to make this crate only have rustc_driver as the only reverse dependency and maybe even link this in dynamically so that it can be swapped for something else at runtime (e.g. for Spanish translations or whatnot).
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.
The problem is that, if we do it like this, the only way to check that a used error code exists will be to add another check after the compiler is built. I prefer re-building some dependencies rather than having to wait the whole build to see if I didn't forget to create a new error 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.
If that's an issue then we can move the tidy checks (or this specific check) early or something and always execute them on e.g. ./x.py test -i src/test/ui --stage 1 --pass check
which is the common command to do. We could also execute tidy on ./x.py check
. cc @Mark-Simulacrum
The way you've implemented this now, if I were to e.g. add a new error code I would need to rebuild pretty much everything, including librustc
, instead of just rebuilding librustc_interface
, which goes quickly. This would be painful if all you want to do is to --bless
some UI tests.
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.
Ok, then I suggest this: let's open an issue about this once this PR is merged and handle it into another PR. I think this one is already *huge* enough.
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.
Fair enough. :)
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.
I am against adding anything to x.py check or x.py test that runs by default and isn't tidy. You can always run tidy first if you want.
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.
@Mark-Simulacrum What about always running tidy on said commands? -- I personally have been bitten many times by forgetting to do so since I typically never use the full ./x.py test
and I always forget to run tidy manually.
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.
I am against running tidy implicitly unless we can make it much faster; currently it's 1-3 seconds of overhead which is way to much for a "always run" type command.
|
||
#[macro_export] | ||
macro_rules! register_diagnostics { | ||
($($ecode:ident: $message:expr,)*) => ( |
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 matcher looks unused now.
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.
It's still being used actually. (it's the separation between long and short error codes) :)
$( (stringify!($ecode), $message), )* | ||
]; | ||
|
||
$( |
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.
These constants shouldn't be necessary if we unlink things from the various crates as noted in a different comment of mine.
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.
Like I said, if we decide to do it, let's make it into another PR. I think this one is already big enough.
//! being to make their maintenance easier. | ||
|
||
#[macro_export] | ||
macro_rules! register_diagnostics { |
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.
And then this macro can be inlined to where it is defined.
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.
Again: I prefer to keep them separated, makes the reading simpler.
|
||
mod error_codes; | ||
|
||
pub use error_codes::*; |
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.
Could just export DIAGNOSTICS
now that it is the only item of interest; alternatively, since it is the only item, we could inline the whole error_codes
file into lib.rs
.
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.
I'd prefer not. The file is big enough on its own so let's keep what's error code specific outside if possible.
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.
It becomes small after using include!(...)
s. ;)
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.
(unless the error codes long explanation are moved into files.
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.
Even after the change, I still think it's better to keep it in its own file. :)
This comment has been minimized.
This comment has been minimized.
71aa96e
to
c51ebb8
Compare
This comment has been minimized.
This comment has been minimized.
c51ebb8
to
ba775ee
Compare
So it seems like all is done and that we need to open some issues? What do you think @Centril ? |
Adding a checkbox to the existing issue would be enough I think. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d077c6c
to
fab24f0
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #66366) made this pull request unmergeable. Please resolve the merge conflicts. |
cb69c8b
to
aa615f5
Compare
@Centril It's getting a bit complicated to follow with the rebases. A little review please so I can move forward? :) |
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 |
src/test/ui/explain.stdout
Outdated
@@ -1,4 +1,4 @@ | |||
Per [RFC 401][rfc401], if you have a function declaration `foo`: | |||
er [RFC 401][rfc401], if you have a function declaration `foo`: |
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.
Oops?
@@ -1,5 +1,4 @@ | |||
{"message":"mismatched types","code":{"code":"E0308","explanation":" | |||
This error occurs when the compiler was unable to infer the concrete type of a | |||
{"message":"mismatched types","code":{"code":"E0308","explanation":"This error occurs when the compiler was unable to infer the concrete type of a |
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.
Intentional?
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.
Must be part of a rebase I suppose?
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.
I've no idea myself. :)
r=me with ^-- addressed somehow and when you think this is landable. @bors rollup=never p=100 |
aa615f5
to
52e2eb6
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 |
Finally! Let's quickly move this in then! @bors: r=Centril |
d7dedf6
to
b5b2a89
Compare
Another try... @bors: r=Centril |
📌 Commit b5b2a89 has been approved by |
Move error codes Works towards #66210. r? @Centril Oh btw, for the ones interested, I used this python script to get all error codes content sorted into one final file: <details> ```python from os import listdir from os.path import isdir, isfile, join def get_error_codes(error_codes, f_path): with open(f_path) as f: short_mode = False lines = f.read().split("\n") i = 0 while i < len(lines): line = lines[i] if not short_mode and line.startswith("E0") and line.endswith(": r##\""): error = line error += "\n" i += 1 while i < len(lines): line = lines[i] error += line if line.endswith("\"##,"): break error += "\n" i += 1 error_codes["long"].append(error) elif line == ';': short_mode = True elif short_mode is True and len(line) > 0 and line != "}": error_codes["short"].append(line) while i + 1 < len(lines): line = lines[i + 1].strip() if not line.startswith("//"): break parts = line.split("//") if len(parts) < 2: break if parts[1].strip().startswith("E0"): break error_codes["short"][-1] += "\n" error_codes["short"][-1] += lines[i + 1] i += 1 i += 1 def loop_dirs(error_codes, cur_dir): for entry in listdir(cur_dir): f = join(cur_dir, entry) if isfile(f) and entry == "error_codes.rs": get_error_codes(error_codes, f) elif isdir(f) and not entry.startswith("librustc_error_codes"): loop_dirs(error_codes, f) def get_error_code(err): x = err.split(",") if len(x) < 2: return err x = x[0] if x.strip().startswith("//"): x = x.split("//")[1].strip() return x.strip() def write_into_file(error_codes, f_path): with open(f_path, "w") as f: f.write("// Error messages for EXXXX errors. Each message should start and end with a\n") f.write("// new line, and be wrapped to 80 characters. In vim you can `:set tw=80` and\n") f.write("// use `gq` to wrap paragraphs. Use `:set tw=0` to disable.\n\n") f.write("syntax::register_diagnostics! {\n\n") error_codes["long"].sort() for i in error_codes["long"]: f.write(i) f.write("\n\n") f.write(";\n") error_codes["short"] = sorted(error_codes["short"], key=lambda err: get_error_code(err)) for i in error_codes["short"]: f.write(i) f.write("\n") f.write("}\n") error_codes = { "long": [], "short": [] } loop_dirs(error_codes, "src") write_into_file(error_codes, "src/librustc_error_codes/src/error_codes.rs") ``` </details> And to move the error codes into their own files: <details> ```python import os try: os.mkdir("src/librustc_error_codes/error_codes") except OSError: print("Seems like folder already exist, moving on!") data = '' with open("src/librustc_error_codes/error_codes.rs") as f: x = f.read().split('\n') i = 0 short_part = False while i < len(x): line = x[i] if short_part is False and line.startswith('E0') and line.endswith(': r##"'): err_code = line.split(':')[0] i += 1 content = '' while i < len(x): if x[i] == '"##,': break content += x[i] content += '\n' i += 1 f_path = "src/librustc_error_codes/error_codes/{}.md".format(err_code) with open(f_path, "w") as ff: ff.write(content) data += '{}: include_str!("./error_codes/{}.md"),'.format(err_code, err_code) elif short_part is False and line == ';': short_part is True data += ';\n' else: data += line data += '\n' i += 1 with open("src/librustc_error_codes/error_codes.rs", "w") as f: f.write(data) ``` </details>
☀️ Test successful - checks-azure |
Works towards #66210.
r? @Centril
Oh btw, for the ones interested, I used this python script to get all error codes content sorted into one final file:
And to move the error codes into their own files: