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.

Sign up for GitHub

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

Jump to bottom

Accumulate all error codes / descriptions into one crate? #66210

Closed
1 task
Centril opened this issue Nov 8, 2019 · 11 comments
Closed
1 task

Accumulate all error codes / descriptions into one crate? #66210

Centril opened this issue Nov 8, 2019 · 11 comments
Assignees
@Centril
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Nov 8, 2019 •
edited by GuillaumeGomez
Loading

@ecstatic-morse noted on Zulip that:

Is there a reason that error codes are split across crates instead of combined into a single one e.g., librustc_errors? Seems like doing so would make this easier, as well as move some code out of librustc and libsyntax, which are on the critical path when building rustc.

Should we move all the error codes into a dedicated crate (librustc_error_codes) or into the existing librustc_errors? Advantages include:

  • Making other crates, especially librustc, smaller and thus improving pipelining.
  • All the codes and descriptions are in a single place, which should be easier to manage overall -- no need to move error codes when logic is moved between crates or when crates are split.

Disadvantages include:

  • Everything that uses error codes will need to be recompiled, in particular librustc, if you do introduce a new error code or modify a description. I don't think this happens all that frequently. We could also separate the codes from the descriptions.

cc @rust-lang/compiler @GuillaumeGomez @nnethercote

(My personal inclination is to introduce a new fresh crate dedicated to error codes.)

  • Need to clean up the macro usage and maybe debate about either we want to convert all error codes to strings directly and therefore move the unused/unexistent/duplicates check into tidy.
@Centril Centril added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 8, 2019
@GuillaumeGomez
Copy link
Member

The issue is that if you change this crate, you'll have to recompile everything everytime. I think the main reason why it hasn't been done this way is that it allows to prevent both the full re-compilation and to have HUGE files with hundreds of error codes in one place.

Also, small advantage but a nice one: it allows to quickly link an error code to where it's coming from in the code.

@Centril
Copy link
Contributor Author

Centril commented Nov 8, 2019

The issue is that if you change this crate, you'll have to recompile everything everytime.

Hmm; so I took a look at https://doc.rust-lang.org/nightly/nightly-rustc/src/syntax/diagnostics/macros.rs.html and it doesn't seem like there's actually an intrinsic reason why that would be the case: we could separate the registration of diagnostic codes and descriptions from the actual struct_span_err! macros that use the codes (it's just an identifier). Tidy can then make sure to catch any problems (e.g. registered but unused, used but not registered, ...). We should be able to improve recompilation actually.

and to have HUGE files with hundreds of error codes in one place.

That's fair, but note that even though the file would be large, it would also be regular and of "boring" content. The lion's share would just be a bunch of strings.

Also, small advantage but a nice one: it allows to quickly link an error code to where it's coming from in the code.

I had to do a lot of this when writing up #65324, I used the GitHub search functionality by pasting in the error code and it quickly found the usage.

@GuillaumeGomez
Copy link
Member

That would make the whole more difficult to understand. Why would we want to separate the declaration and the explanation?

I had to do a lot of this when writing up #65324, I used the GitHub search functionality by pasting in the error code and it quickly found the usage.

Wow, a brave one. I never use the github search; it's just useless most of the time. A git grep works way better. :)

@Centril
Copy link
Contributor Author

Centril commented Nov 8, 2019

Why would we want to separate the declaration and the explanation?

  • Better incremental and pipe-lining with ./x.py because only rustc_interface::util::diagnostics_registry needs to depend on those explanations. In fact, that function can be moved wholesale to the new crate. Also better pipe-lining because librustc and friends get thinner.

  • Better potential for internationalization -- maybe we should even move these descriptions to .md files, post-process, and load them in lazily at run-time?

  • Simpler dev UX: want to add an error description? there's just one place to append to instead of many more files.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 8, 2019 •
edited
Loading

If we do this right, the error codes crate could be "lazilly" linked -- i.e., only a dependency of librustc_driver. We could achieve this for example by using a query to fetch the details of the error code. Then there is no recompilation hazard. Ideally, the error descriptions themselves might even be in .md files or something that are include!'d in, to make them more viewable?

UPDATE: Er, right, @Centril was already proposing that md thing. I missed it in the last comment, though I saw it on Zulip. :) Well 👍 to that!

@GuillaumeGomez
Copy link
Member

Then just one question remain: will this lazy linking work with libcore?

@Centril
Copy link
Contributor Author

Centril commented Nov 11, 2019

@GuillaumeGomez Can you elaborate on that please? libcore doesn't have error codes...?

@GuillaumeGomez
Copy link
Member

Indeed, don't know why I recalled libcore having error codes...

@Centril Centril self-assigned this Nov 11, 2019
@Centril
Copy link
Contributor Author

Centril commented Nov 11, 2019

Assigning myself to do this at some point, but feel free to work-steal.

@GuillaumeGomez
Copy link
Member

I'll give it a try then. :)

@GuillaumeGomez GuillaumeGomez mentioned this issue Nov 11, 2019
Merged
bors added a commit that referenced this issue Nov 14, 2019
@bors
Auto merge of #66314 - GuillaumeGomez:move-error-codes, r=Centril
82cf3a4 
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>
@Centril Centril mentioned this issue Nov 15, 2019
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this issue Nov 17, 2019
@JohnTitor
Rollup merge of rust-lang#66456 - Centril:driver-codes, r=Mark-Simula…
6e6c46c 
…crum

Move `DIAGNOSTICS` usage to `rustc_driver`

Remove `rustc_interface`'s dependency on `rustc_error_codes` and centralize all usages of `DIAGNOSTICS` in `rustc_driver`. Once we remove all references to `rustc_error_codes` in all other crates but `rustc_driver`, this should allow for incremental recompilation of the compiler to be smoother when tweaking error codes. This works towards rust-lang#66210 (comment).

(May include traces of minor drive-by cleanup.)

r? @Mark-Simulacrum
@Centril
Copy link
Contributor Author

Centril commented Jan 30, 2020

We did this.

@Centril Centril closed this as completed Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Centril Centril

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nikomatsakis @Centril @GuillaumeGomez