note individual lint name in messages set via lint group attribute #38103

zackmdavis · 2016-12-01T06:11:33Z

Resolves #36846.

Update 16 December (new commits):

zackmdavis · 2016-12-14T02:16:19Z

@jonathandturner are you interested in reviewing (the reason I r?'d you is because you seemed to express interest in the issue comments), or should we tag someone else (perhaps @brson, the issue filer)?

sophiajt · 2016-12-14T02:21:29Z

@zackmdavis - sorry for the delay. I'm currently on vacation so I haven't been doing much reviewing.

Just taking a glance, I'd move that new message to a note (without a span) possibly. Cool that it's there now, though.

zackmdavis · 2016-12-15T02:29:01Z

I'm currently on vacation so

Oh, sorry to bother you! (I can only hope that my bad luck of accidentally bothering people on vacation does not become a trend.)

I'd move that new message to a note (without a span)

And presumably also the #[{}({})] on by default message on which it is based. Yes, I can do that (on the timescale of some days; I unfortunately don't have a development environment handy right now).

zackmdavis · 2016-12-17T05:01:51Z

Updated:

9603a5f moves the #[level(individual_lint)] on by default and #[level(lint_group)] implies #[level(individual_lint)] into a span-less note, rather than being part of the main lint message, as Jonathan suggests.
bb685b5 slightly broadens the scope of this pull request, but to a degree that I argue is entirely appropriate: it differentiates between an individual lint vs. a lint group having been set on the command line (analogously to the case of lints set via attributes, which inspired this endeavor)

zackmdavis · 2016-12-17T16:56:33Z

and 7ba8b4d to update the UI tests because somehow make check weren't running them for me earlier (?!) although ./x.py test src/test/ui/ works

zackmdavis · 2016-12-17T18:14:17Z

r? @brson

aturon · 2016-12-29T21:50:14Z

cc @nikomatsakis

nikomatsakis · 2016-12-30T19:18:45Z

I don't mind reviewing this.

@zackmdavis -- I would rather expect these clarifications to appear on the "lint level defined here" note, rather than in the main heading. What do you think?

nikomatsakis · 2016-12-30T19:18:53Z

r? @nikomatsakis

nikomatsakis · 2016-12-30T19:19:13Z

oh, I missed the updated pics.

nikomatsakis

This looks pretty good! It does seem like there are a few scenarios not tested though. Can you add UI tests for these cases:

using command-line flags (-W etc) that affect a lint indirectly (and hence trigger the "implies" message)
using each of the various settings (-W etc)

You can customize a test to include compilation flags by putting a // compile-flags: ... annotation in the test (ripgrep around for examples).

Thanks!

nikomatsakis · 2017-01-03T18:56:23Z

src/test/compile-fail/lint-group-style.rs

-        struct snake_case; //~ WARN type `snake_case` should have a camel case name
+        struct snake_case;
+        //~^ WARN type `snake_case` should have a camel case name
+        //~| NOTE #[warn(bad_style)] implies #[warn(non_camel_case_types)]


I think these tests should be made into "ui" tests -- see https://github.com/rust-lang/rust/blob/master/src/test/ui/README.md for some tips on how to work with those.

zackmdavis · 2017-01-05T17:12:40Z

@nikomatsakis moved make-lint-group-style.rs to be a UI test rather than a compile fail test as you suggested (b4e7128), added UI tests for command-line -A/-W/-D/-F with a lint group (fc692f5), had to merge in master to get Travis to pass (babce29, 7e50b02)

nikomatsakis

Looking really good. =) I have a few more suggestions, curious as to your opinion.

Oh, and one other thing: can you rebase atop rust-lang/master (and squash, probably), rather than merging master into your branch? Just keeps the history a little cleaner. Thanks.

nikomatsakis · 2017-01-05T20:29:11Z

src/test/ui/span/multispan-import-lint.stderr

  --> $DIR/multispan-import-lint.rs:11:16
   |
 11 | use std::cmp::{Eq, Ord, min, PartialEq, PartialOrd};
   |                ^^  ^^^       ^^^^^^^^^  ^^^^^^^^^^
+   |
+   = note: #[warn(unused_imports)] on by default


I'm very happy to see this moved into an auxiliary note. I've always been annoyed by having it on the main message line :)

nikomatsakis · 2017-01-05T20:29:53Z

src/test/ui/span/issue-24690.stderr

@@ -4,6 +4,7 @@ error: unused variable: `theOtherTwo`
 20 |     let theOtherTwo = 2;
   |         ^^^^^^^^^^^
   |
+   = note: #[deny(warnings)] implies #[deny(unused_variables)]


it feels like this should be attached to the message below (lint level defined here), and also that it should only appear when that message appears (meaning only once), what do you think?

I agree that this is desirable, but doing it properly will involve addressing the FIXME about generalizing one-time diagnostics beyond span_note, which looks sufficiently complicated that I'd prefer to do it in a separate PR.

(The most obvious thing to do would be to extract the body of what is now diag_span_note_once into a more general diag_once that takes an enum argument to decide which method to call (span_note or note or warn or &c.) on the diagnostic builder ... or are macros the way to go here? I think this is out-of-scope for this PR.)

nikomatsakis · 2017-01-05T20:30:37Z

src/test/ui/lint/command-line-lint-group-forbid.stderr

+14 |     let _InappropriateCamelCasing = true;
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-F bad-style` implies `-F non-snake-case`


I keep being surprised by the ordering here. What do you think about saying "-F non-snake-case implied by -F bad-style or something?

zackmdavis · 2017-01-06T05:05:59Z

@nikomatsakis

rebased and squashed previous work into 3 commits (down from 8); "implies" vs. "implied by" comment addressed in 5bceb47; prefer to defer the work to make implied-by-lint-group notes one-time

nikomatsakis · 2017-01-06T20:05:10Z

@bors r+

bors · 2017-01-06T20:05:10Z

📌 Commit 5bceb47 has been approved by nikomatsakis

nikomatsakis · 2017-01-06T20:05:14Z

@zackmdavis great! :)

bors · 2017-01-07T08:02:11Z

⌛ Testing commit 5bceb47 with merge 91d81d6...

bors · 2017-01-07T09:00:22Z

💔 Test failed - status-appveyor

This little patch arises from the maelstrom of my purity born of pain. It's the pain of seeing rust-lang/rust#38103 in its perfect crystalline beauty waste away on page four of https://github.com/rust-lang/rust/pulls, waiting, ready, itching to land, dying with anticipation to bring the light of clearer lint group error messages to Rust users of all creeds and nations, only for its promise to be cruelly blocked by the fateful, hateful hand of circular dependency. For it is written in src/tools/cargotest/main.rs that the Cargo tests must pass before the PR can receive Appveyor's blessing, but the Cargo tests could not pass (because they depend on fine details of the output that the PR is meant to change), and the Cargo tests could not be changed (because updating the test expectation to match the proposed new compiler output, would fail with the current compiler). The Gordian knot is cut in the bowels of cargotest's very notion of comparison (of JSON objects) itself, by means of introducing a magic string literal `"{...}"`, which can server as a wildcard for any JSON sub-object. And so it will be for the children, and the children's children, and unto the 1.17.0 and 1.18.0 releases, that Cargo's build test expectations will faithfully expect the exact JSON output by Cargo itself, but the string literal `"{...}"` shall be a token upon the JSON output by rustc, and when I see `"{...}"`, I will pass over you, and the failure shall not be upon you. And this day shall be unto you for a memorial.

@alexcrichton

…ain, r=alexcrichton make build tests not depend on minutiae of rustc output This little patch arises from the maelstrom of my purity born of pain. It's the pain of seeing rust-lang/rust#38103 in its perfect crystalline beauty waste away on page four of https://github.com/rust-lang/rust/pulls, waiting, ready, itching to land, dying with anticipation to bring the light of clearer lint group error messages to Rust users of all creeds and nations, only for its promise to be cruelly blocked by the fateful, hateful hand of circular dependency. For it is written in src/tools/cargotest/main.rs that the Cargo tests must pass before the PR can receive Appveyor's blessing, but the Cargo tests could not pass (because they depend on fine details of the output that the PR is meant to change), and the Cargo tests could not be changed (because updating the test expectation to match the proposed new compiler output, would fail with the current compiler). The Gordian knot is cut in the bowels of cargotest's very notion of comparison (of JSON objects) itself, by means of introducing a magic string literal `"{...}"`, which can server as a wildcard for any JSON sub-object. And so it will be for the children, and the children's children, and unto the 1.17.0 and 1.18.0 releases, that Cargo's build test expectations will faithfully expect the exact JSON output by Cargo itself, but the string literal `"{...}"` shall be a token upon the JSON output by rustc, and when I see `"{...}"`, I will pass over you, and the failure shall not be upon you. And this day shall be unto you for a memorial. supersedes #3513 r? @alexcrichton

zackmdavis · 2017-02-03T07:15:51Z

(force-pushed) @nikomatsakis Ready!! 🏃‍♀️ 🏁 💖

zackmdavis · 2017-02-03T15:28:19Z

I almost want to ask if this can get p=1 just because it's been so long, but that's probably not fair

nikomatsakis · 2017-02-03T20:50:51Z

@zackmdavis older PRs already get priority =)

nikomatsakis · 2017-02-03T20:51:20Z

@bors r+

bors · 2017-02-03T20:51:20Z

📌 Commit 82e42d5 has been approved by nikomatsakis

zackmdavis · 2017-02-03T22:12:30Z

👀 glances at queue

auuugh 82e42d5 is going to collide with #36320's b7f1d7a

😿 😭 💀 sobs in existential despair

bors · 2017-02-04T04:03:47Z

🔒 Merge conflict

bors · 2017-02-04T04:09:29Z

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

Warning or error messages set via a lint group attribute (e.g. `#[deny(warnings)]`) should still make it clear which individual lint (by name) was triggered, similarly to how we include "on by default" language for default lints. This—and, while we're here, the existing "on by default" language—can be tucked into a note rather than cluttering the main error message. This occasions the slightest of refactorings (we now have to get the diagnostic-builder with the main message first, before matching on the lint source). This is in the matter of rust-lang#36846.

Previously, the note/message for the source of a lint being the command line unconditionally named the individual lint, even if the actual command specified a lint group (e.g., `-D warnings`); here, we take note of the actual command options so we can be more specific. This remains in the matter of rust-lang#36846.

As suggested by Niko Matsakis in review (rust-lang#38103 (comment)) regarding the endeavor prompted by rust-lang#36846.

alexcrichton · 2017-02-04T21:52:50Z

@bors: r=nikomatsakis

bors · 2017-02-04T21:52:51Z

📌 Commit 72af42e has been approved by nikomatsakis

bors · 2017-02-05T01:22:58Z

⌛ Testing commit 72af42e with merge d7777ae...

…ups_or_warnings_meta-lint_obscure_the_original_lint_name, r=nikomatsakis note individual lint name in messages set via lint group attribute ![lint_errors_resulting_from_lint_groups_obscure](https://cloud.githubusercontent.com/assets/1076988/20783614/c107d5c8-b749-11e6-85de-eada7f67c986.png) Resolves #36846. r? @jonathandturner ----- ***Update*** 16 December (new commits): ![lint_group_makeover_party](https://cloud.githubusercontent.com/assets/1076988/21284540/ff1ae2fc-c3d2-11e6-93be-d0689f5fa7a8.png)

bors · 2017-02-05T04:00:22Z

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

This little patch arises from the maelstrom of my purity born of pain. It's the pain of seeing rust-lang/rust#38103 in its perfect crystalline beauty waste away on page four of https://github.com/rust-lang/rust/pulls, waiting, ready, itching to land, dying with anticipation to bring the light of clearer lint group error messages to Rust users of all creeds and nations, only for its promise to be cruelly blocked by the fateful, hateful hand of circular dependency. For it is written in src/tools/cargotest/main.rs that the Cargo tests must pass before the PR can receive Appveyor's blessing, but the Cargo tests could not pass (because they depend on fine details of the output that the PR is meant to change), and the Cargo tests could not be changed (because updating the test expectation to match the proposed new compiler output, would fail with the current compiler). The Gordian knot is cut in the bowels of cargotest's very notion of comparison (of JSON objects) itself, by means of introducing a magic string literal `"{...}"`, which can server as a wildcard for any JSON sub-object. And so it will be for the children, and the children's children, and unto the 1.17.0 and 1.18.0 releases, that Cargo's build test expectations will faithfully expect the exact JSON output by Cargo itself, but the string literal `"{...}"` shall be a token upon the JSON output by rustc, and when I see `"{...}"`, I will pass over you, and the failure shall not be upon you. And this day shall be unto you for a memorial.

From review discussion on rust-lang#38103 (rust-lang#38103 (comment)).

@nikomatsakis

…the_first_time, r=eddyb make lint on-by-default/implied-by messages appear only once From review discussion on rust-lang#38103 (rust-lang#38103 (comment)). ![we_heard](https://user-images.githubusercontent.com/1076988/27564103-6284b78e-5a8a-11e7-9d35-f7f297ca9573.png) r? @nikomatsakis

rust-highfive assigned sophiajt Dec 1, 2016

rust-highfive assigned brson and unassigned sophiajt Dec 17, 2016

rust-highfive assigned nikomatsakis and unassigned brson Dec 30, 2016

nikomatsakis requested changes Jan 3, 2017

View reviewed changes

nikomatsakis requested changes Jan 5, 2017

View reviewed changes

zackmdavis force-pushed the lint_errors_resulting_from_lint_groups_or_warnings_meta-lint_obscure_the_original_lint_name branch from 7e50b02 to 5bceb47 Compare January 6, 2017 04:05

nikomatsakis approved these changes Jan 6, 2017

View reviewed changes

zackmdavis force-pushed the lint_errors_resulting_from_lint_groups_or_warnings_meta-lint_obscure_the_original_lint_name branch from 5bceb47 to 82e42d5 Compare February 3, 2017 07:13

zackmdavis added 4 commits February 4, 2017 10:44

make lint-group-style test a UI rather than a compile-fail test

778958f

As suggested by Niko Matsakis in review (rust-lang#38103 (comment)) regarding the endeavor prompted by rust-lang#36846.

note wording: lint implied by lint group, not lint group implies lint

72af42e

zackmdavis force-pushed the lint_errors_resulting_from_lint_groups_or_warnings_meta-lint_obscure_the_original_lint_name branch from 82e42d5 to 72af42e Compare February 4, 2017 18:52

bors merged commit 72af42e into rust-lang:master Feb 5, 2017

zackmdavis added a commit to zackmdavis/rust that referenced this pull request Jun 26, 2017

make lint on-by-default/implied-by messages appear only once

32b8579

From review discussion on rust-lang#38103 (rust-lang#38103 (comment)).

zackmdavis mentioned this pull request Jun 26, 2017

make lint on-by-default/implied-by messages appear only once #42919

Merged

zackmdavis deleted the lint_errors_resulting_from_lint_groups_or_warnings_meta-lint_obscure_the_original_lint_name branch January 13, 2018 07:42

note individual lint name in messages set via lint group attribute #38103

note individual lint name in messages set via lint group attribute #38103

Conversation

zackmdavis commented Dec 1, 2016 • edited

zackmdavis commented Dec 14, 2016

sophiajt commented Dec 14, 2016

zackmdavis commented Dec 15, 2016

zackmdavis commented Dec 17, 2016

zackmdavis commented Dec 17, 2016

zackmdavis commented Dec 17, 2016

aturon commented Dec 29, 2016

nikomatsakis commented Dec 30, 2016

nikomatsakis commented Dec 30, 2016

nikomatsakis commented Dec 30, 2016

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Jan 3, 2017

Choose a reason for hiding this comment

zackmdavis commented Jan 5, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Jan 5, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 5, 2017

Choose a reason for hiding this comment

zackmdavis Jan 6, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 5, 2017

Choose a reason for hiding this comment

zackmdavis commented Jan 6, 2017

nikomatsakis commented Jan 6, 2017

bors commented Jan 6, 2017

nikomatsakis commented Jan 6, 2017

bors commented Jan 7, 2017

bors commented Jan 7, 2017

zackmdavis commented Feb 3, 2017

zackmdavis commented Feb 3, 2017

nikomatsakis commented Feb 3, 2017

nikomatsakis commented Feb 3, 2017

bors commented Feb 3, 2017

zackmdavis commented Feb 3, 2017

bors commented Feb 4, 2017

bors commented Feb 4, 2017

alexcrichton commented Feb 4, 2017

bors commented Feb 4, 2017

bors commented Feb 5, 2017

bors commented Feb 5, 2017

zackmdavis commented Dec 1, 2016 •

edited