-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
internal: fix most Clippy lints #16398
Conversation
The tests fail in CI, but not locally 🤔 |
|
I am on it |
let Some(expn) = db.lookup_intern_syntax_context(span.ctx).outer_expn else { | ||
return false; | ||
}; | ||
let expn = db.lookup_intern_macro_call(expn); | ||
// FIXME: Record allow_internal_unstable in the macro def (not been done yet because it | ||
// would consume quite a bit extra memory for all call locs...) | ||
// if let Some(features) = expn.def.allow_internal_unstable { | ||
// if features.iter().any(|&f| f == sym::edition_panic) { | ||
// span = expn.call_site; | ||
// continue; | ||
// } | ||
// } | ||
expn.def.edition >= Edition::Edition2021 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please undo this again, this is a loop because of the commented out code and so has meaning once thats fixed again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reset the branch to not include the manual changes. They will come in a separate PR.
crates/stdx/src/thread/pool.rs
Outdated
@@ -86,6 +86,7 @@ impl Pool { | |||
self.job_sender.send(job).unwrap(); | |||
} | |||
|
|||
#[allow(clippy::len_without_is_empty)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this is why we don't really run clippy on the code base 😅 the defaults are sometimes questionable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not Clippy's fault, I'd expect len
to return either the thread count or the pending queue size, not the number of running jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this is why we don't really run clippy on the code base 😅 the defaults are sometimes questionable
We can configure it. But I feel it is useful to catch a lot of small cleanups, which I believe is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall yes, I do want us to run clippy. I just don't like some of the default set of lints it has. Related #15918
crates/stdx/src/non_empty_vec.rs
Outdated
@@ -28,6 +28,7 @@ impl<T> NonEmptyVec<T> { | |||
} | |||
|
|||
#[inline] | |||
#[allow(clippy::len_without_is_empty)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could arguably return NonZeroUsize
, but I don't know if it silences the Clippy warning. Anyway, it's not even used, so..
df35dc2
to
255cde7
Compare
@bors r+ |
☀️ Test successful - checks-actions |
https://github.com/rust-lang/rust-analyzer/pull/16398/files#diff-fa039ab572a8591c57be2cea529bb1e287b071b8f30cd5d495da3df98d2a31d1L191-L193 |
Looks like some of the other tests in there might be affected too. |
Oh, that should definitely be reverted. Forgot we had these weird tests there |
@ivan thank you for spotting, created an issue for it |
This PR is the result of running
cargo clippy --fix && cargo fmt
in the root of the repository. I did not manually review all the changes, but just skimmed through a few of them. The tests still pass, so it seems fine.