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.

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

lint reasons (RFC 2383, part 1) #54683

Merged
merged 4 commits into from
Oct 28, 2018

Conversation

zackmdavis
Copy link
Member

This implements the reason = functionality described in the RFC under a lint_reasons feature gate.

lint_reasons_pt_1

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2018
ast::MetaItemKind::NameValue(ref name_value) => {
let name_ident = item.ident.segments[0].ident;
let name = name_ident.name.as_str();
if name == "reason" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, you can just use item.ident == "reason".

//~^ ERROR malformed lint attribute
#![warn(elided_lifetimes_in_paths, reason("disrespectful to ancestors", "irresponsible to heirs"))]
//~^ ERROR malformed lint attribute
#![warn(ellipsis_inclusive_range_patterns, reason)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a couple more cases:
#[warn(lint1, reason = "zzz", reason = "yyy")] - duplicated reason (probably an error).
#[warn(lint1, reason = "zzz", lint2)] - reason is not the last (an error? just to be conservative)

Copy link
Member Author

@zackmdavis zackmdavis Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reason is not the last (an error? just to be conservative)

Sure. I trust this is compliant with the spirit of the RFC? @Centril @myrrlyn

This is also nice from an implementer's perspective—in the first revision of this PR, I felt bad about doing a entire extra iteration over the meta items just to pick up the reason before entering the loop that processes the lint names themselves, but with this restriction, we can just peek at the end.

@@ -245,11 +246,33 @@ impl<'a> LintLevelsBuilder<'a> {
}
} else {
let mut err = bad_attr(name_value.span);
err.help("reason must be a string literal");
if let ast::LitKind::ByteStr(_) = name_value.node {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No code is good, code is evil.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this case will ever be hit realistically, so I see this as an immediate technical debt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have known sin 😰 💀

@petrochenkov petrochenkov 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 2, 2018
@TimNN
Copy link
Contributor

TimNN commented Oct 9, 2018

Ping from triage @zackmdavis. It looks like some changes have been requested to your PR.

Is this PR blocked on #54926?

@zackmdavis
Copy link
Member Author

@TimNN No, I just suffer from a psychological defect in which composing shiny new PRs is more fun than addressing reviews of outstanding PRs (and the microincentives of which tasks are more fun play a disproportionate role in determining what humans will actually accomplish in general, not just as an open-source volunteer); give me a few more days

@bors
Copy link
Contributor

bors commented Oct 11, 2018

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

@zackmdavis
Copy link
Member Author

(My local build is mysteriously failing right now.)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:20:02]    Compiling unwind v0.0.0 (/checkout/src/libunwind)
[00:20:09]    Compiling compiler_builtins v0.0.0 (/checkout/src/rustc/compiler_builtins_shim)
[00:20:09]    Compiling cmake v0.1.33
[00:20:09]    Compiling alloc_jemalloc v0.0.0 (/checkout/src/liballoc_jemalloc)
[00:20:10] error: unused import: `prelude::v1::*`
[00:20:10]     |
[00:20:10]     |
[00:20:10] 135 | use prelude::v1::*;
[00:20:10]     |
[00:20:10]     = note: `-D unused-imports` implied by `-D warnings`
[00:20:10] 
[00:20:10] error: unused macro definition
[00:20:10] error: unused macro definition
[00:20:10]   --> libcore/num/wrapping.rs:16:1
[00:20:10]    |
[00:20:10] 16 | / macro_rules! sh_impl_signed {
[00:20:10] 17 | |     ($t:ident, $f:ident) => (
[00:20:10] 18 | |         #[stable(feature = "rust1", since = "1.0.0")]
[00:20:10] 19 | |         impl Shl<$f> for Wrapping<$t> {
[00:20:10] 63 | |     )
[00:20:10] 64 | | }
[00:20:10]    | |_^
[00:20:10]    |
[00:20:10]    |
[00:20:10]    = note: `-D unused-macros` implied by `-D warnings`
[00:20:10] 
[00:20:10] error: unused macro definition
[00:20:10]    --> libcore/ops/arith.rs:636:1
[00:20:10]     |
[00:20:10] 636 | / macro_rules! neg_impl_unsigned {
[00:20:10] 637 | |     ($($t:ty)*) => {
[00:20:10] 638 | |         neg_impl_core!{ x => {
[00:20:10] 639 | |             !x.wrapping_add(1)
[00:20:10] 640 | |         }, $($t)*} }
[00:20:10]     | |_^
[00:20:10] 
[00:20:10] error: unused macro definition
[00:20:10]    --> libcore/lib.rs:237:1
[00:20:10]    --> libcore/lib.rs:237:1
[00:20:10]     |
[00:20:10] 237 | macro_rules! test_v16 { ($item:item) => {}; }
[00:20:10] 
[00:20:10] error: unused macro definition
[00:20:10]    --> libcore/lib.rs:239:1
[00:20:10]     |
[00:20:10]     |
[00:20:10] 239 | macro_rules! test_v32 { ($item:item) => {}; }
[00:20:10] 
[00:20:10] error: unused macro definition
[00:20:10]    --> libcore/lib.rs:241:1
[00:20:10]     |
[00:20:10]     |
[00:20:10] 241 | macro_rules! test_v64 { ($item:item) => {}; }
[00:20:10] 
[00:20:10] error: unused macro definition
[00:20:10]    --> libcore/lib.rs:243:1
[00:20:10]     |
[00:20:10]     |
[00:20:10] 243 | macro_rules! test_v128 { ($item:item) => {}; }
[00:20:10] 
[00:20:10] error: unused macro definition
[00:20:10]    --> libcore/lib.rs:245:1
[00:20:10]     |
[00:20:10]     |
[00:20:10] 245 | macro_rules! test_v256 { ($item:item) => {}; }
[00:20:10] 
[00:20:10] error: unused macro definition
[00:20:10]    --> libcore/lib.rs:247:1
[00:20:10]     |
[00:20:10]     |
[00:20:10] 247 | macro_rules! test_v512 { ($item:item) => {}; }
[00:20:10] 
[00:20:10] error: unused macro definition
[00:20:10]    --> libcore/lib.rs:249:1
[00:20:10]     |
[00:20:10]     |
[00:20:10] 249 | macro_rules! vector_impl { ($([$f:ident, $($args:tt)*]),*) => { $($f!($($args)*);)* } }
[00:20:10] 
[00:20:10] 
[00:20:12] error: use of deprecated item 'str::LinesAny': use lines()/Lines instead now
[00:20:12]      |
[00:20:12]      |
[00:20:12] 1387 | impl<'a> Iterator for LinesAny<'a> {
[00:20:12]      |
[00:20:12]      = note: `-D deprecated` implied by `-D warnings`
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'str::LinesAny': use lines()/Lines instead now
[00:20:12]      |
[00:20:12]      |
[00:20:12] 1403 | impl<'a> DoubleEndedIterator for LinesAny<'a> {
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'str::LinesAny': use lines()/Lines instead now
[00:20:12]      |
[00:20:12]      |
[00:20:12] 1412 | impl FusedIterator for LinesAny<'_> {}
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/mod.rs:100:9
[00:20:12]     |
[00:20:12] 100 | pub use self::sip::SipHasher;
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher13': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/mod.rs:105:9
[00:20:12]     |
[00:20:12] 105 | pub use self::sip::SipHasher13;
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:144:6
[00:20:12] 144 | impl SipHasher {
[00:20:12]     |      ^^^^^^^^^
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher13': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:166:6
[00:20:12] 166 | impl SipHasher13 {
[00:20:12]     |      ^^^^^^^^^^^
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:252:24
[00:20:12]     |
[00:20:12] 252 | impl super::Hasher for SipHasher {
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher13': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:265:24
[00:20:12]     |
[00:20:12] 265 | impl super::Hasher for SipHasher13 {
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher24': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]   --> libcore/hash/sip.rs:62:22
[00:20:12]    |
[00:20:12] 62 | pub struct SipHasher(SipHasher24);
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'str::LinesAny': use lines()/Lines instead now
[00:20:12]      |
[00:20:12]      |
[00:20:12] 2852 |     pub fn lines_any(&self) -> LinesAny {
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'str::LinesAny': use lines()/Lines instead now
[00:20:12]      |
[00:20:12]      |
[00:20:12] 2853 |         LinesAny(self.lines())
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:150:21
[00:20:12]     |
[00:20:12] 150 |     pub fn new() -> SipHasher {
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:151:9
[00:20:12]     |
[00:20:12] 151 |         SipHasher::new_with_keys(0, 0)
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:159:51
[00:20:12]     |
[00:20:12] 159 |     pub fn new_with_keys(key0: u64, key1: u64) -> SipHasher {
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:160:9
[00:20:12]     |
[00:20:12] 160 |         SipHasher(SipHasher24 {
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher24': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:160:19
[00:20:12]     |
[00:20:12] 160 |         SipHasher(SipHasher24 {
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher13': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:172:21
[00:20:12]     |
[00:20:12] 172 |     pub fn new() -> SipHasher13 {
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher13': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:173:9
[00:20:12]     |
[00:20:12] 173 |         SipHasher13::new_with_keys(0, 0)
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher13': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:181:51
[00:20:12]     |
[00:20:12] 181 |     pub fn new_with_keys(key0: u64, key1: u64) -> SipHasher13 {
[00:20:12] 
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher13': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12]    --> libcore/hash/sip.rs:182:9
[00:20:12] 182 |         SipHasher13 {
[00:20:12]     |         ^^^^^^^^^^^
[00:20:12] 
[00:20:12]    Compiling std v0.0.0 (/checkout/src/libstd)
[00:20:12]    Compiling std v0.0.0 (/checkout/src/libstd)
[00:20:12] error: use of deprecated item 'str::LinesAny': use lines()/Lines instead now
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher13': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher24': use `std::collections::hash_map::DefaultHasher` instead
[00:20:12] 
[00:20:12] error: use of deprecated item 'hash::sip::SipHasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:14]    Compiling rustc_msan v0.0.0 (/checkout/src/librustc_msan)
[00:20:14]    Compiling rustc_lsan v0.0.0 (/checkout/src/librustc_lsan)
[00:20:14]    Compiling rustc_tsan v0.0.0 (/checkout/src/librustc_tsan)
[00:20:15]    Compiling rustc_asan v0.0.0 (/checkout/src/librustc_asan)
[00:20:15]    Compiling rustc_asan v0.0.0 (/checkout/src/librustc_asan)
[00:20:25] error: use of deprecated item 'hash::sip::SipHasher::new_with_keys': use `std::collections::hash_map::DefaultHasher` instead
[00:20:25]    --> libcore/hash/sip.rs:151:9
[00:20:25]     |
[00:20:25] 151 |         SipHasher::new_with_keys(0, 0)
[00:20:25] 
[00:20:25] 
[00:20:25] error: use of deprecated item 'hash::sip::SipHasher24::hasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:25]    --> libcore/hash/sip.rs:161:13
[00:20:25]     |
[00:20:25] 161 |             hasher: Hasher::new_with_keys(key0, key1)
[00:20:25] 
[00:20:25] 
[00:20:25] error: use of deprecated item 'hash::sip::SipHasher13::new_with_keys': use `std::collections::hash_map::DefaultHasher` instead
[00:20:25]    --> libcore/hash/sip.rs:173:9
[00:20:25]     |
[00:20:25] 173 |         SipHasher13::new_with_keys(0, 0)
[00:20:25] 
[00:20:25] 
[00:20:25] error: use of deprecated item 'hash::sip::SipHasher13::hasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:25]    --> libcore/hash/sip.rs:183:13
[00:20:25]     |
[00:20:25] 183 |             hasher: Hasher::new_with_keys(key0, key1)
[00:20:25] 
[00:20:25] 
[00:20:25] error: use of deprecated item 'hash::sip::SipHasher24::hasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:25]    --> libcore/hash/sip.rs:255:9
[00:20:25]     |
[00:20:25] 255 |         self.0.hasher.write(msg)
[00:20:25] 
[00:20:25] 
[00:20:25] error: use of deprecated item 'hash::sip::SipHasher24::hasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:25]    --> libcore/hash/sip.rs:260:9
[00:20:25]     |
[00:20:25] 260 |         self.0.hasher.finish()
[00:20:25] 
[00:20:25] 
[00:20:25] error: use of deprecated item 'hash::sip::SipHasher13::hasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:25]    --> libcore/hash/sip.rs:268:9
[00:20:25]     |
[00:20:25] 268 |         self.hasher.write(msg)
[00:20:25] 
[00:20:25] 
[00:20:25] error: use of deprecated item 'hash::sip::SipHasher13::hasher': use `std::collections::hash_map::DefaultHasher` instead
[00:20:25]    --> libcore/hash/sip.rs:273:9
[00:20:25] 273 |         self.hasher.finish()
[00:20:25]     |         ^^^^^^^^^^^
[00:20:25] 
/modules/compiler-rt/objects
---
travis_time:end:09fe1344:start=1539331646700284145,finish=1539331646705078475,duration=4794330
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0668a818
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:09e83770
travis_time:start:09e83770
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-g

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 @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Oct 14, 2018

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

@zackmdavis
Copy link
Member Author

(My local build is mysteriously failing right now.)

"Mysterious", bah! (This was actually a really idiotic bug where the reslicing that I meant to do when we found the reason = meta-item was being done unconditionally.)

@zackmdavis
Copy link
Member Author

@petrochenkov updated 🏁

@zackmdavis zackmdavis added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2018
} else {
reason = Some(rationale);
}
let tail_li = &metas[metas.len()-1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will panic on #[allow()], no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... right. 😰 😥 💀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this trigger unused_attributes? (As recently discussed for empty derives and inappropriate #[must_use])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels reasonable to me to trigger unused_attributes for empty allow; esp. if we do it for empty derives.

#![warn(keyword_idents, reason = "root in rubble", macro_use_extern_crate)]
//~^ ERROR malformed lint attribute
//~| HELP reason in lint attribute must come last
#![warn(missing_copy_implementations, reason)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with #[allow(reason = "foo")]?
I.e. no lints to allow, only the reason.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's make this an error (it would have been a no-op if you hadn't pointed this out).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... well, actually, I guess I could see a case that, if empty lint attributes (#[allow()] as discussed above) aren't an error (the stable behavior), then we should also allow reason-only lint attributes for consistency/uniformity.

(Presumably the only sane reason for empty lint attributes to exist is for the sake of the base case of some recursive macro, and if we care about that, then we should also care about a macro that's otherwise identical but also includes a reason.)

@petrochenkov petrochenkov 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 15, 2018
@zackmdavis
Copy link
Member Author

I took a shot at implementing unused-attributes linting for empty and reason-only lint attributes, but I didn't understand some of the behavior I was seeing (the first empty lint attribute in a program was getting linted twice for some reason), so I've elected to defer that, filing an issue (#55112) and leaving FIXME comments. (I could have hacked around the duplication with one-time-diagnostics, but that would be bad; if you want to write correct software and you don't understand something, don't guess.)

In this revision, we don't panic on #[level()] (which is already a no-op on the stable compiler), and #[level(reason = "foo")] is, with some reluctance, allowed (for the reasons mentioned above). A run-pass UI test is added to document these.

... I could be persuaded to go with my first instinct and make #[level(reason = "foo")] an error. The "it's a generalization of #[level()] and needs to have the same behavior" argument seems strong to me, but another sort of conservatism argues in favor of not accepting new crazy code (even as backwards-compatibility forces our hand to accept old kinds of crazy code).

@petrochenkov what do you think??

@zackmdavis zackmdavis added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 16, 2018
@bors
Copy link
Contributor

bors commented Oct 18, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 18, 2018
@kennytm
Copy link
Member

kennytm commented Oct 18, 2018

Failed a cargo fix test https://github.com/rust-lang/cargo/blob/35cd245ee8522f28db62a65bfba6f8d32972d699/tests/testsuite/fix.rs#L54.

[01:53:47] ---- fix::broken_fixes_backed_out stdout ----
[01:53:47] running `C:\projects\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\cargo.exe build`
[01:53:47] running `C:\projects\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\cargo.exe fix --allow-no-vcs`
[01:53:47] thread 'fix::broken_fixes_backed_out' panicked at '
[01:53:47] Expected: execs
[01:53:47]     but: expected to find:
[01:53:47] warning: failed to automatically apply fixes suggested by rustc to crate `bar`
[01:53:47] 
[01:53:47] after fixes were automatically applied the compiler reported errors within these files:
[01:53:47] 
[01:53:47]   * src/lib.rs
[01:53:47] 
[01:53:47] This likely indicates a bug in either rustc or cargo itself,
[01:53:47] and we would appreciate a bug report! You're likely to see 
[01:53:47] a number of compiler warnings after this message which cargo
[01:53:47] attempted to fix but failed. If you could open an issue at
[01:53:47] https://github.com/rust-lang/cargo/issues
[01:53:47] quoting the full output of this command we'd be very appreciative!
[01:53:47] 
[01:53:47] did not find in output:
[01:53:47]    Compiling bar v0.1.0 (C:\projects\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\cit\t563\foo\bar)
[01:53:47] error: expected one of `!` or `::`, found `rust`
[01:53:47]  --> src\lib.rs:1:5
[01:53:47]   |
[01:53:47] 1 | not rust code
[01:53:47]   |     ^^^^ expected one of `!` or `::` here
[01:53:47] 
[01:53:47] error: aborting due to previous error
[01:53:47] 
[01:53:47] error: Could not compile `bar`.
[01:53:47] warning: build failed, waiting for other jobs to finish...
[01:53:47]       Fixing src\lib.rs (1 fix)
[01:53:47] error: build failed
[01:53:47] ', tools\cargo\tests\testsuite\support\mod.rs:741:13
[01:53:47] note: Run with `RUST_BACKTRACE=1` for a backtrace.

@kennytm 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 18, 2018
@bors
Copy link
Contributor

bors commented Oct 25, 2018

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

This is just for the `reason =` name-value meta-item; the
`#[expect(lint)]` attribute also described in the RFC is a problem for
another day.

The place where we were directly calling `emit()` on a match block
(whose arms returned a mutable reference to a diagnostic-builder) was
admittedly cute, but no longer plausibly natural after adding the
if-let to the end of the `LintSource::Node` arm.

This regards rust-lang#54503.
We take stability seriously, so we shy away from making even seemingly
"trivial" features insta-stable.
Vadim Petrochenkov suggested this in review ("an error? just to be
conservative"), and it turns out to be convenient from the
implementer's perspective: in the initial proposed implementation (or
`HEAD~2`, as some might prefer to call it), we were doing an entire
whole iteration over the meta items just to find the reason (before
iterating over them to set the actual lint levels). This way, we can
just peek at the end rather than adding that extra loop (or
restructuring the existing code). The RFC doesn't seem to take a
position on this, and there's some precedent for restricting things to
be at the end of a sequence (we only allow `..` at the end of a struct
pattern, even if it would be possible to let it appear anywhere in the
sequence).
We avoid an ICE by checking for an empty meta-item list before we
index into the meta-items, and leave commentary about where we'd like
to issue unused-attributes lints in the future. Note that empty lint
attributes are already accepted by the stable compiler; generalizing
this to weird reason-only lint attributes seems like the
conservative/consilient generalization.
@zackmdavis
Copy link
Member Author

Failed a cargo fix test

@kennytm I can't reproduce this (Ubuntu 16.04). (At least not consistently—I remember seeing it once while trying to reproduce the other week, while also struggling with a flaky build, but it's passing for me now, as illustrated by output below.) Please advise?

$ RUSTC=/home/zmd/Code/rust/build/x86_64-unknown-linux-gnu/stage2/bin/rustc cargo test broken_fixes_backed_out
   Compiling atty v0.2.11
   Compiling env_logger v0.5.13
   Compiling clap v2.32.0
   Compiling cargo v0.32.0 (file:///home/zmd/Code/cargo)
    Finished dev [unoptimized + debuginfo] target(s) in 20.96s
     Running target/debug/deps/cargo-7423faeb2a614ee4

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 23 filtered out

     Running target/debug/deps/testsuite-76f90d822272ea40

running 1 test
test fix::broken_fixes_backed_out ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 1456 filtered out

@zackmdavis zackmdavis removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 27, 2018
@kennytm
Copy link
Member

kennytm commented Oct 27, 2018

@zackmdavis maybe try to test on windows?

@zackmdavis
Copy link
Member Author

... I don't have a Windows machine handy. I would be pretty surprised if there was really a Windows-specific bug to find here (what Windows-specific API could this patch be using??), as opposed to something about the cargo fix test being spuriously nondeterministic (which would be bad, but not something we can fix in this pull request). Is it OK if we try one more time?

@bors r=petrochenkov

(feel free to r- if you disagree with my judgement)

@bors
Copy link
Contributor

bors commented Oct 27, 2018

📌 Commit f66ea66 has been approved by petrochenkov

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 27, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Oct 27, 2018
…r=petrochenkov

lint reasons (RFC 2883, part 1)

This implements the `reason =` functionality described in [the RFC](https://github.com/rust-lang/rfcs/blob/master/text/2383-lint-reasons.md) under a `lint_reasons` feature gate.

![lint_reasons_pt_1](https://user-images.githubusercontent.com/1076988/46252097-eed51000-c418-11e8-8212-939d3f02f95d.png)
@bors
Copy link
Contributor

bors commented Oct 28, 2018

⌛ Testing commit f66ea66 with merge 18311a6...

bors added a commit that referenced this pull request Oct 28, 2018
lint reasons (RFC 2883, part 1)

This implements the `reason =` functionality described in [the RFC](https://github.com/rust-lang/rfcs/blob/master/text/2383-lint-reasons.md) under a `lint_reasons` feature gate.

![lint_reasons_pt_1](https://user-images.githubusercontent.com/1076988/46252097-eed51000-c418-11e8-8212-939d3f02f95d.png)
@bors
Copy link
Contributor

bors commented Oct 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 18311a6 to master...

@bors bors merged commit f66ea66 into rust-lang:master Oct 28, 2018
@zackmdavis zackmdavis deleted the critique_of_pure_lints branch October 28, 2018 03:12
@8573
Copy link

8573 commented Jun 7, 2022

Could the RFC number in the ticket title be corrected for ease of searching?

Also, is it right that this remains labelled as "S-waiting-on-bors"?

@bjorn3 bjorn3 changed the title lint reasons (RFC 2883, part 1) lint reasons (RFC 2383, part 1) Jun 7, 2022
@bjorn3
Copy link
Member

bjorn3 commented Jun 7, 2022

Edited the title. As for S-waiting-on-bors, it seems bors keeps it on every merged PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants