Rustc explain #48337

GuillaumeGomez · 2018-02-18T23:04:24Z

To make the review easier, I separated tests update to code update. Also, I used this script to generate new ui tests stderr:

from os import listdir
from os.path import isdir, isfile, join

PATH = "src/test/ui"

def do_something(path):
    files = [join(path, f) for f in listdir(path)]

    for f in files:
        if isdir(f):
            do_something(f)
            continue
        if not isfile(f) or not f.endswith(".stderr"):
            continue
        x = open(f, "r")
        content = x.read().strip()
        if "error[E" not in content:
            continue
        errors = dict()
        for y in content.splitlines():
            if y.startswith("error[E"):
                errors[y[6:11]] = True
        errors = sorted(errors.keys())
        if len(errors) < 1:
            print("weird... {}".format(f))
            continue
        if len(errors) > 1:
            content += "\n\nYou've got a few errors: {}".format(", ".join(errors))
            content += "\nIf you want more information on an error, try using \"rustc --explain {}\"".format(errors[0])
        else:
            content += "\n\nIf you want more information on this error, try using \"rustc --explain {}\"".format(errors[0])
        content += "\n"
        x = open(f, "w")
        x.write(content)

do_something(PATH)

rust-highfive · 2018-02-18T23:04:34Z

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

oli-obk · 2018-02-19T09:17:53Z

src/librustc_errors/emitter.rs

+                          \"rustc --explain {}\"",
+                         &error_codes[0]).expect("failed to give tips...");
+            } else {
+                writeln!(self.dst,


Should this message show up for errors that don't show any extra information if --explain is run?

I'm not sure to understand your point...

Sorry I confused this with the teach flag

GuillaumeGomez · 2018-02-19T09:22:27Z

The travis failure doesn't make any sense. When I run rustc on the tests, I don't get the same output, even with the same parameters. Did I miss something (it's just like the code is run in parts and then the new error message gets printed after each of them)?

michaelwoerister · 2018-02-19T09:30:47Z

r? @estebank

oli-obk · 2018-02-19T09:31:08Z

Did I miss something (it's just like the code is run in parts and then the new error message gets printed after each of them)?

That might be because ui tests are run with --error-format=json and the rendered field of all diagnostics are concatenated.

GuillaumeGomez · 2018-02-19T13:15:56Z

Should I update how it's done then? Because right now, as I said, it has nothing in common with the actual output...

GuillaumeGomez · 2018-02-19T20:53:19Z

Should be done now.

estebank · 2018-02-20T04:10:04Z

Still failing :-/

Minimal difference due to JSON encoding:

[01:01:33] ---- [ui] ui/lint/use_suggestion_json.rs stdout ----
[01:01:33] 	diff of stderr:
[01:01:33] 
[01:01:33] 384	   |
[01:01:33] 385	and 8 other candidates
[01:01:33] 386	
[01:01:33] -	If you want more information on this error, try using "rustc --explain E0412"
[01:01:33] +	If you want more information on this error, try using /"rustc --explain E0412/"
[01:01:33] 388	"
[01:01:33] 389	}
[01:01:33] 390	{

oli-obk

The windows/linux normalizer changes \ to /, previously the json decoder removed escape backslashes during string decoding

oli-obk · 2018-02-20T08:31:58Z

src/tools/compiletest/src/runtest.rs

@@ -1629,8 +1632,8 @@ impl<'test> TestCx<'test> {
                .iter()
                .any(|s| s.starts_with("--error-format"))
            {
-                rustc.args(&["--error-format", "json"]);
-            },
+                //rustc.args(&["--error-format", "json"]);


leftover comment.

GuillaumeGomez · 2018-02-21T17:36:08Z

It passed tests. \o/

estebank

After addressing the nitpick, r=me.

estebank · 2018-02-21T18:22:21Z

src/librustc_errors/emitter.rs

+        if !self.short_message && !self.error_codes.is_empty() {
+            let mut error_codes = self.error_codes.clone().into_iter().collect::<Vec<_>>();
+            error_codes.sort();
+            if error_codes.len() > 1 {


Should this restrict the error list to some reasonable amount? I'm ok with any amount, but I'd constrain it to at most two full 80 col lines.

Does it really matter? I can do it but I'm not sure if this is very useful... You're not supposed to try to have as much errors as possible when writing code haha.

It should be rare, but I see no reason for us not to gracefully handle that case :)

It won't ever be possible for a given project to have more than a subset of the ~600 errors the compiler has, but regardless, it is worthless to list the error codes, once the user understands how to use the flag, it makes more sense to scroll to the error that they want more information on and copy the error code from there, as the error code on it's own is meaningless.

Ok, I'll limit then.

estebank · 2018-02-21T23:36:06Z

I still think there's value in limiting the amount of error codes listed to a reasonable amount, but it is silly to block on it.

Also, it would also be nice to have a way to disable this output, but I won't block this PR on this. We need to add a compiler config file to handle this and other cases (--teach, coloring, one line output, etc.).

@bors r+

bors · 2018-02-21T23:36:06Z

📌 Commit 8dbdae2 has been approved by estebank

bors · 2018-02-22T14:25:36Z

☔ The latest upstream changes (presumably #48399) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2018-02-26T09:07:52Z

Damn, it was new tests...

GuillaumeGomez · 2018-02-26T11:52:59Z

CI passed so let's r+ again (with a higher priority to avoid being outdated once again)...

@bors: r=estebank p=10

bors · 2018-02-26T11:53:00Z

📌 Commit ce6429a has been approved by estebank

bors · 2018-02-26T12:34:59Z

⌛ Testing commit ce6429a with merge bedbad6...

Rustc explain Fixes #48041. To make the review easier, I separated tests update to code update. Also, I used this script to generate new ui tests stderr: ```python from os import listdir from os.path import isdir, isfile, join PATH = "src/test/ui" def do_something(path): files = [join(path, f) for f in listdir(path)] for f in files: if isdir(f): do_something(f) continue if not isfile(f) or not f.endswith(".stderr"): continue x = open(f, "r") content = x.read().strip() if "error[E" not in content: continue errors = dict() for y in content.splitlines(): if y.startswith("error[E"): errors[y[6:11]] = True errors = sorted(errors.keys()) if len(errors) < 1: print("weird... {}".format(f)) continue if len(errors) > 1: content += "\n\nYou've got a few errors: {}".format(", ".join(errors)) content += "\nIf you want more information on an error, try using \"rustc --explain {}\"".format(errors[0]) else: content += "\n\nIf you want more information on this error, try using \"rustc --explain {}\"".format(errors[0]) content += "\n" x = open(f, "w") x.write(content) do_something(PATH) ```

bors · 2018-02-26T15:27:09Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing bedbad6 to master...

petrochenkov · 2018-02-26T17:48:13Z

src/tools/compiletest/src/runtest.rs

@@ -2544,6 +2545,7 @@ impl<'test> TestCx<'test> {
            }
        }
        if !explicit {
+            let proc_res = self.compile_test(&["--error-format", "json"]);


Yeah, let's just compile all the tests twice.
Who cares about time, certainly not Appveyor.

zackmdavis · 2018-02-26T20:08:37Z

@GuillaumeGomez

If you want more information on an error, try using "rustc --explain {}"

I know I'm late, but I would argue that this would be better phrased as something more like:

For more information about an error, try `rustc --explain {}`

(avoids informal-sounding second person, omits needless word 'using', backticks instead of quotation marks for code).

GuillaumeGomez · 2018-02-26T20:55:03Z

@zackmdavis: A bit yes. Let me update then...

zackmdavis · 2018-02-28T09:00:24Z

src/librustc_errors/emitter.rs

@@ -115,6 +116,33 @@ struct FileWithAnnotatedLines {
    multiline_depth: usize,
 }

+impl Drop for EmitterWriter {
+    fn drop(&mut self) {


@GuillaumeGomez (more belated commentary in light of #48550) What was the motivation for putting the this in drop()? Semantically, printing the message notifying the user about --explain does not seem like a "cleanup" action. As Niko suggested, I would kind of expect this to go with the "aborting due to previous error" message in Handler.abort_if_errors—and it looks like the Handler already knows which codes have been emitted, too.

Hum, if this is true, it might allow to make my task for #48562.

This reverts commit 1dc2015, which switched to compiling the UI tests twice rather than extracting the humanized output from a field in the JSON output. A conflict in this revert commit had to be fixed manually where changes introduced in rust-lang#48449 collide with the change we're trying to revert (from rust-lang#48337). This is in the matter of rust-lang#48550. Conflicts: src/tools/compiletest/src/runtest.rs

This reverts commit 9b597a1. That commit added a message informing users about the `rustc --explain` functionality inside of a `Drop` implementation for `EmitterWriter`. In addition to exhibiting questionable semantics (printing a hopefully-helpful message for the user does not seem like a "cleanup" action), this resulted in divergent behavior between humanized output and JSON output, because the latter actually instantiates an `EmitterWriter` for every diagnostic. Several conflicts in this revert commit had to be fixed manually, again owing to code-area collision between rust-lang#48449 and rust-lang#48337. This is in the matter of rust-lang#48550. Conflicts: src/librustc_errors/emitter.rs

zackmdavis · 2018-03-01T17:40:45Z

@GuillaumeGomez One last comment (sorry if it seems like I've been overly critical with respect to this feature; sometimes good code requires a lot of discussion, and sometimes I have a lot of things to say), but arguably the --explain usage message should come before the "aborting due to n errors" message (so that we're aborting immediately after we say that we're aborting).

matklad · 2018-03-05T14:09:36Z

TIL that you can not only rustc --explain CODE, but cargo --explain CODE as well: rust-lang/cargo#2551

The commit message on that PR implies the idea was to make rustc print just run --explain CODE and not run rustc --explain CODE or something like this (I am not sure I understand this entirely).

rust-highfive assigned michaelwoerister Feb 18, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 18, 2018

oli-obk reviewed Feb 19, 2018

View reviewed changes

rust-highfive assigned estebank and unassigned michaelwoerister Feb 19, 2018

GuillaumeGomez force-pushed the rustc-explain branch from 0d4682c to a317e7a Compare February 19, 2018 20:53

GuillaumeGomez force-pushed the rustc-explain branch from a317e7a to 4c63eea Compare February 19, 2018 22:44

oli-obk reviewed Feb 20, 2018

View reviewed changes

GuillaumeGomez force-pushed the rustc-explain branch 6 times, most recently from 2818fe4 to 8dbdae2 Compare February 21, 2018 15:25

estebank approved these changes Feb 21, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 22, 2018

Fix new tests

ce6429a

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 26, 2018

bors merged commit ce6429a into rust-lang:master Feb 26, 2018

GuillaumeGomez deleted the rustc-explain branch February 26, 2018 16:34

petrochenkov reviewed Feb 26, 2018

View reviewed changes

petrochenkov mentioned this pull request Feb 26, 2018

Most of UI tests are now compiled twice, once with --error-format json and once with --error-format human #48550

Closed

This was referenced Feb 26, 2018

Update rustc --explain sentence #48559

Closed

Remove unneeded error check in ui tests #48562

Closed

zackmdavis reviewed Feb 28, 2018

View reviewed changes

zackmdavis mentioned this pull request Mar 3, 2018

Put back ui json check #48684

Merged

ehuss mentioned this pull request Dec 5, 2020

Add explanations and suggestions to irrefutable_let_patterns lint #79747

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustc explain #48337

Rustc explain #48337

GuillaumeGomez commented Feb 18, 2018 •

edited

Loading

rust-highfive commented Feb 18, 2018

oli-obk Feb 19, 2018

GuillaumeGomez Feb 19, 2018

oli-obk Feb 19, 2018

GuillaumeGomez commented Feb 19, 2018

michaelwoerister commented Feb 19, 2018

oli-obk commented Feb 19, 2018 •

edited

Loading

GuillaumeGomez commented Feb 19, 2018

GuillaumeGomez commented Feb 19, 2018

estebank commented Feb 20, 2018

oli-obk left a comment

oli-obk Feb 20, 2018

GuillaumeGomez commented Feb 21, 2018

estebank left a comment

estebank Feb 21, 2018

GuillaumeGomez Feb 21, 2018

estebank Feb 21, 2018

GuillaumeGomez Feb 22, 2018

estebank commented Feb 21, 2018

bors commented Feb 21, 2018

bors commented Feb 22, 2018

GuillaumeGomez commented Feb 26, 2018

GuillaumeGomez commented Feb 26, 2018

bors commented Feb 26, 2018

bors commented Feb 26, 2018

bors commented Feb 26, 2018

petrochenkov Feb 26, 2018

zackmdavis commented Feb 26, 2018

GuillaumeGomez commented Feb 26, 2018

zackmdavis Feb 28, 2018

GuillaumeGomez Feb 28, 2018

zackmdavis commented Mar 1, 2018

matklad commented Mar 5, 2018

Rustc explain #48337

Rustc explain #48337

Conversation

GuillaumeGomez commented Feb 18, 2018 • edited Loading

rust-highfive commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 19, 2018

michaelwoerister commented Feb 19, 2018

oli-obk commented Feb 19, 2018 • edited Loading

GuillaumeGomez commented Feb 19, 2018

GuillaumeGomez commented Feb 19, 2018

estebank commented Feb 20, 2018

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 21, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Feb 21, 2018

bors commented Feb 21, 2018

bors commented Feb 22, 2018

GuillaumeGomez commented Feb 26, 2018

GuillaumeGomez commented Feb 26, 2018

bors commented Feb 26, 2018

bors commented Feb 26, 2018

bors commented Feb 26, 2018

Choose a reason for hiding this comment

zackmdavis commented Feb 26, 2018

GuillaumeGomez commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackmdavis commented Mar 1, 2018

matklad commented Mar 5, 2018

GuillaumeGomez commented Feb 18, 2018 •

edited

Loading

oli-obk commented Feb 19, 2018 •

edited

Loading