New lint and cargotest


#1

I improved a lint to warn unused trait imports. This failed to merge, because the lint triggered for unused trait imports in cargo as designed, which causes cargotest to fail because of #[deny(unused)]. As far as I can tell there is no way to tell cargo to pass --cap-lints to rustc. So I am not sure how to proceed.

As is, this means new lint can’t be merged until it is fixed in all cargotest crates, as well as existing requirement that it is fixed in all of rustc and rustdoc.


#2

This might sound insensitive, but this is exactly why cargotest exists! If this feature is important you’ll need to convince reviewers that it’s worth breaking cargo for, then fix cargo and update cargotest. Sorry not sorry :slight_smile:


#3

Improving lints is not considered “breaking” as per rfc1193, so technically the fact that cargotest has #[deny(...)] attributes is the issue.


#4

Okay, changing the requirement from “fix all of rustc and rustdoc” to “fix all of rustc and rustdoc and cargo” is not that a big deal. Still, it is a noticable change, because Cargo is in the different repository.

But cargotest includes projects other than Cargo, which is just Iron at the moment. As I understand, more will be added in the future. Is the intention that landing new lint should be blocked on fixing Iron and additional crates to be determined in the future?


#5

I think crates in the cargotest list should not have deny or forbid attributes. Instead their CI should set that flag through RUSTFLAGS or similar


#6

I think crates in the cargotest list should not have deny or forbid attributes. Instead their CI should set that flag through RUSTFLAGS or similar.

I am not sure how breaking CI is any better than breaking build. @alexcrichton expressed the same opinion at https://github.com/rust-lang/cargo/pull/2622, which also suggested “deny in CI”.


#7

It’s mine yeah. Adding crates to cargotest shouldn’t be done lightly.