Report lint names in json diagnostics #45484

oli-obk · 2017-10-24T06:40:51Z

This allows tools like rustfix to have whitelists for what to automatically apply and what not.

rust-highfive · 2017-10-24T06:40:55Z

r? @eddyb

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

eddyb · 2017-10-24T06:42:00Z

r? @nikomatsakis

arielb1 · 2017-10-24T13:59:39Z

src/librustc_errors/emitter.rs

@@ -893,7 +893,8 @@ impl EmitterWriter {
        } else {
            buffer.append(0, &level.to_string(), Style::Level(level.clone()));
            match code {
-                &Some(ref code) => {
+                // only render error codes, not lint codes
+                &Some(ref code) if code.starts_with("E") && code.len() == 5 => {


Might be better to check for the regex E[0-9]+ - I could imagine someone using a string "Exlint" and getting confused about the weird rendering.

You can't do that. they are lowercase by definition (autogenerated from the global-name in the lint declaration macro)

@oli-obk

You can't do that. they are lowercase by definition (autogenerated from the global-name in the lint declaration macro)

Wait, I'm confused -- what are you saying here? The error codes are lowercase? You're already checking for E, so that can't be it.

Still, this kinda' feels like a hack to me. Why not add another field to the diagnostic, like lint_name or something? It seems pretty odd that our final JSON winds up with "explanation":"", which has nothing to do with lints.

I meant lint names are always lowercase, that's how we can distinguish them.

That said, I can replace the code field (currently a String) with an

enum DiagnosticId { Error(String), Lint(String), }

It seems pretty odd that our final JSON winds up with "explanation":"", which has nothing to do with lints.

The explanation being empty is an implementation detail. It was nontrivial to add and I didn't want to mess up the PR with unrelated changes. The issue is that the array with the error explanations is filled in before the lints are loaded into the session and then frozen by creating the error emitter. So once the lints are loaded (and thus their explanations are available), we can't place them into the emitter.

It just requires some refactoring to get done.

I found it odder to have two Option fields which are mutually exclusive than a temporarily empty explanation string.

killercup

This will make rustfix very happy!

killercup · 2017-10-24T13:59:44Z

src/librustc_errors/emitter.rs

@@ -893,7 +893,8 @@ impl EmitterWriter {
        } else {
            buffer.append(0, &level.to_string(), Style::Level(level.clone()));
            match code {
-                &Some(ref code) => {
+                // only render error codes, not lint codes
+                &Some(ref code) if code.starts_with("E") && code.len() == 5 => {


This is kinda why I suggested to not use the code field for the lint name

nikomatsakis · 2017-10-25T12:50:45Z

cc @estebank -- JSON format discussion

estebank · 2017-10-27T21:37:36Z

I'm slightly worried about people getting confused with DiagnosticId::Lint and Level::Warning, but otherwise I'm happy with the change as it is now.

@bors r+

bors · 2017-10-27T21:37:37Z

📌 Commit 181054a has been approved by estebank

estebank · 2017-10-27T21:39:24Z

@bors r-

I thought @nikomatsakis had r'd me, it was a cc instead.

bors · 2017-10-28T18:49:55Z

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

nikomatsakis · 2017-10-30T14:30:17Z

src/librustc_errors/diagnostic.rs

@@ -81,7 +87,7 @@ impl Diagnostic {
        Diagnostic::new_with_code(level, None, message)
    }

-    pub fn new_with_code(level: Level, code: Option<String>, message: &str) -> Self {
+    pub fn new_with_code(level: Level, code: Option<DiagnosticId>, message: &str) -> Self {


Do we want to guarantee that lints cannot have diagnostic codes? That doesn't seem like an obviously good thing... although I guess that the lint name can serve as a diagnostic code, which is kind of nice.

Diagnostic codes are for errors. Everything else is silenceable in some way and thus a lint. Clippy has a lint list like the error code list of rustc. I don't think having two different identifiers for a lint would be useful. At least I can't think of any use

nikomatsakis · 2017-10-30T14:36:08Z

@oli-obk I'm curious, what's the case for using the code field to store the lint name? (As opposed to adding some other field?)

We don't really have a strong back-compat story around JSON, so it seems like we'd be pretty committed to this format if we decide to do it. I'd like the reasoning and longer term plan to be a bit more "well documented" here.

oli-obk · 2017-10-30T15:00:04Z

I'm curious, what's the case for using the code field to store the lint name? (As opposed to adding some other field?)

Having two different fields feels like both of them could be non-null. But that would only make sense if lints additionally had an error code. I don't think we'd be able to do that, since clippy and rustc would have to agree on error codes. Lint names on the other hand are less likely to collide, and lints already have an identifier: the lint name.

I'll add some documentation.

nikomatsakis · 2017-10-31T14:34:29Z

I've come around; I think re-using the same code field makes sense. The way I look at it, diagnostic codes have two purposes:

for giving to --explain
for better google searching

Both make sense for lints, but then it seems that lint names can serve both purposes perfectly well. So I think I agree with you that we don't need to have two fields here.

(To be honest, I am not 100% convinced that diagnostic codes serve that much value. If we reworked --explain so that it gave explanations "inline", that may suffice. I'm not sure if the googling thing is important or not.)

nikomatsakis · 2017-11-01T13:40:40Z

@oli-obk needs rebase but r=me or r=estebank

oli-obk · 2017-11-02T09:20:23Z

rebased

nikomatsakis · 2017-11-02T13:41:48Z

@bors r+

bors · 2017-11-02T13:41:49Z

📌 Commit 6ae440e has been approved by nikomatsakis

bors · 2017-11-03T00:42:17Z

⌛ Testing commit 6ae440e with merge 9f3b091...

Report lint names in json diagnostics This allows tools like `rustfix` to have whitelists for what to automatically apply and what not.

bors · 2017-11-03T03:20:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 9f3b091 to master...

rust-highfive assigned eddyb Oct 24, 2017

rust-highfive assigned nikomatsakis and unassigned eddyb Oct 24, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2017

arielb1 reviewed Oct 24, 2017

View reviewed changes

killercup approved these changes Oct 24, 2017

View reviewed changes

oli-obk force-pushed the lint_names branch 2 times, most recently from db21fe9 to 181054a Compare October 27, 2017 09:39

kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2017

nikomatsakis reviewed Oct 30, 2017

View reviewed changes

oli-obk added 2 commits November 2, 2017 10:19

Report lint names in json diagnostics

88fb4c4

Make the difference between lint codes and error codes explicit

6ae440e

oli-obk force-pushed the lint_names branch from 181054a to 6ae440e Compare November 2, 2017 09:20

kennytm 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 Nov 2, 2017

bors added a commit that referenced this pull request Nov 3, 2017

Auto merge of #45484 - oli-obk:lint_names, r=nikomatsakis

9f3b091

Report lint names in json diagnostics This allows tools like `rustfix` to have whitelists for what to automatically apply and what not.

bors merged commit 6ae440e into rust-lang:master Nov 3, 2017

oli-obk deleted the lint_names branch November 3, 2017 07:04

killercup mentioned this pull request Nov 3, 2017

Apply only certain fixes rust-lang/rustfix#36

Closed

estebank mentioned this pull request Jan 24, 2018

Lints could have error codes #24900

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report lint names in json diagnostics #45484

Report lint names in json diagnostics #45484

oli-obk commented Oct 24, 2017

rust-highfive commented Oct 24, 2017

eddyb commented Oct 24, 2017

arielb1 Oct 24, 2017

oli-obk Oct 24, 2017

nikomatsakis Oct 25, 2017 •

edited

Loading

oli-obk Oct 25, 2017

killercup left a comment

killercup Oct 24, 2017 •

edited

Loading

nikomatsakis commented Oct 25, 2017

estebank commented Oct 27, 2017

bors commented Oct 27, 2017

estebank commented Oct 27, 2017

bors commented Oct 28, 2017

nikomatsakis Oct 30, 2017

oli-obk Oct 30, 2017

nikomatsakis commented Oct 30, 2017

oli-obk commented Oct 30, 2017

nikomatsakis commented Oct 31, 2017

nikomatsakis commented Nov 1, 2017

oli-obk commented Nov 2, 2017

nikomatsakis commented Nov 2, 2017

bors commented Nov 2, 2017

bors commented Nov 3, 2017

bors commented Nov 3, 2017

Report lint names in json diagnostics #45484

Report lint names in json diagnostics #45484

Conversation

oli-obk commented Oct 24, 2017

rust-highfive commented Oct 24, 2017

eddyb commented Oct 24, 2017

arielb1 Oct 24, 2017

Choose a reason for hiding this comment

oli-obk Oct 24, 2017

Choose a reason for hiding this comment

nikomatsakis Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

oli-obk Oct 25, 2017

Choose a reason for hiding this comment

killercup left a comment

Choose a reason for hiding this comment

killercup Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

nikomatsakis commented Oct 25, 2017

estebank commented Oct 27, 2017

bors commented Oct 27, 2017

estebank commented Oct 27, 2017

bors commented Oct 28, 2017

nikomatsakis Oct 30, 2017

Choose a reason for hiding this comment

oli-obk Oct 30, 2017

Choose a reason for hiding this comment

nikomatsakis commented Oct 30, 2017

oli-obk commented Oct 30, 2017

nikomatsakis commented Oct 31, 2017

nikomatsakis commented Nov 1, 2017

oli-obk commented Nov 2, 2017

nikomatsakis commented Nov 2, 2017

bors commented Nov 2, 2017

bors commented Nov 3, 2017

bors commented Nov 3, 2017

nikomatsakis Oct 25, 2017 •

edited

Loading

killercup Oct 24, 2017 •

edited

Loading