rust-lang / backtrace-rs

Backtraces in Rust
https://docs.rs/backtrace
Other
532 stars 245 forks source link

ignore clippy lints in `symbolize/gimli/stash.rs` #586

Closed onur-ozkan closed 6 months ago

onur-ozkan commented 7 months ago

This is a blocker for https://github.com/rust-lang/rust/pull/121543

workingjubilee commented 7 months ago

@onur-ozkan I do not intend to add clippy to the backtrace CI. Is there an alternative?

onur-ozkan commented 7 months ago

I do not intend to add clippy to the backtrace CI.

I see. But we already run clippy (ref https://github.com/rust-lang/rust/blob/a655e648a9f94d74263108366b83e677af56e35d/src/ci/docker/host-x86_64/mingw-check/Dockerfile#L45) for the tree and that checks backtrace as well.

Is there an alternative?

Other option would be applying this patch:

diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs
index c6cd2c6786a..7ff0e5f4846 100644
--- a/library/std/src/lib.rs
+++ b/library/std/src/lib.rs
@@ -655,7 +655,7 @@ pub mod arch {
 mod panicking;

 #[path = "../../backtrace/src/lib.rs"]
-#[allow(dead_code, unused_attributes, fuzzy_provenance_casts)]
+#[allow(dead_code, unused_attributes, fuzzy_provenance_casts, clippy::correctness)]
 mod backtrace_rs;

 // Re-export macros defined in core.

But it will completely suppress correctness lint for this crate and I think that doesn't seem like a good alternative. What do you think?

workingjubilee commented 7 months ago

I see. But we already run clippy (ref https://github.com/rust-lang/rust/blob/a655e648a9f94d74263108366b83e677af56e35d/src/ci/docker/host-x86_64/mingw-check/Dockerfile#L45) for the tree and that checks backtrace as well.

I do not see how that is possible, or this would already have blocked forward motion on this, and not just 2 weeks ago, because this code has been here for 4 years.

onur-ozkan commented 7 months ago

I do not see how that is possible, or this would already have blocked forward motion on this, and not just 2 weeks ago, because this code has been here for 4 years.

From https://github.com/rust-lang/rust/pull/121543#issuecomment-1962389452:

"Now clippy complains a lot of stuff because the -D rules have never been functional before."

workingjubilee commented 7 months ago

I see. I would not call that "running clippy", if clippy simply does not work as expected, but aside from being slightly confusing to discuss, I suppose that's besides the point.

onur-ozkan commented 7 months ago

Sorry for confusion.

workingjubilee commented 7 months ago

@onur-ozkan I agree that #[allow(clippy::correctness)] in the middle of std is not an ideal patch.

Many of these modules already feature #![allow(bad_style)], from before the nonstandard_style alias came about. Obviously, if the code here knows it is bad, and must allow itself to be bad, then I certainly do not need the wisdom of the paperclip to try to convince me that the code here is bad. So I think I would prefer #![allow(clippy::all)] in the relevant target-specific modules.

github-actions[bot] commented 7 months ago

Code size changes for a hello-world Rust program linked with libstd with backtrace:

On platform windows-latest:

github-actions[bot] commented 7 months ago

Code size changes for a hello-world Rust program linked with libstd with backtrace:

On platform windows-latest: