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

Fix some clippy lints #138

Merged
merged 11 commits into from
Oct 22, 2018
Merged

Fix some clippy lints #138

merged 11 commits into from
Oct 22, 2018

Conversation

alanhdu
Copy link
Contributor

@alanhdu alanhdu commented Oct 16, 2018

I went ahead and fixed all the clippy lints (there were a couple I thought would be better unfixed and added cfg statements to allow them) and also re-enabled clippy and rustfmt in CI.

They were disabled with no time to explain, disable clippy checks, so hopefully this won't go against whatever the reason at the time was 😆.

One question about the CI though: right now, it's an allowed failure that runs against the latest nightly each time. Would it be better to pin it to a specific nightly (or use the beta versions) to lower the churn?

@matklad
Copy link
Member

matklad commented Oct 16, 2018

They were disabled with no time to explain, disable clippy checks, so hopefully this won't go against whatever the reason at the time was laughing.

At that time, I was transitioning to the to the new implementation of the syntax tree, so that code churn rate was high enough for clippy & rustfmt being a significant problem.

Overall, I am torn about this, curious what other folks think. Here are my rough thoughts about the issue.

I definitely don't want to gate PRs on clippy and rustfmt: to me, additional per-PR code improvements that these tools bring feel much less important that annoyance and additional waiting time for the PR.

I do want to use rustfmt and, to a lesser extend, clippy. These tools do improve code quality in minor-to-moderate ways. At the same time, I am not too keen on keeping a clean git history, especially since GitHub implemented the feature to get the previous blame. So applying suggestions manually from time to time seems fine by me.

I am annoyed by some of the clippy suggestions, which make the code worse :) In particular:

  • needless_pass_by_value is often very intentional and important for correctness. For example, when passing Senders around the moment when the last Sender is dropped is important, because it tears down the receiver and allows to wait for it.

  • Minor changes to boolean expressions like !(idx < container.len()) to idx >= container.len() could make the code less readable. In this case, idx < container.len() is the usual invariant, and ! very cleary states "invariant does not hold".

  • The .map(|&it| it) to .cloned() changes obscure the fact that it is almost-zero const operation, because the type is Copy.

I am also not thrilled by the idea of disabling clippy lints on a case-by-case bases. Is there perhaps some kind of conservative unopinionated clippy profile, that could be applied on the project level?

@DJMcNab
Copy link
Contributor

DJMcNab commented Oct 16, 2018

I think we should enforce rustfmt in ci, but then just run clippy every so often if we find a good set of options.

@matklad
Copy link
Member

matklad commented Oct 16, 2018

I think we should enforce rustfmt in ci,

@DJMcNab why would be more useful than, say, running rusfmt once a week and adding

add this to .git/hooks/pre-commit to run rustfmt automatically on save

cargo fmt-changes

where fmt-changes is a command which applies rustfmt to changed files?

I think it's a bad contributor experience when you PR fails just because you've forgotten a trailing comma, or use a different version of rustfmt.

@DJMcNab
Copy link
Contributor

DJMcNab commented Oct 16, 2018

Different rustfmt version is a good point. I guess I've been spoiled by the ecosystem around nodejs, especially prettier combined with husky and vscode that I think of formatting as a non-issue.

The Rust formatting ecosystem isn't quite there yet, unfortunately, so maybe it would be better to format every so often. I wonder if bors can handle rustfmting automatically somehow...

@CAD97
Copy link
Contributor

CAD97 commented Oct 16, 2018

You can (but shouldn't) make CI commit and push to the repo.

Maybe someone could set up a bot that does fmt PRs?

(Personally, I enforce a pinned stable rustfmt. I think it's better to require standard formatting always, which keeps git-blame accurate. The CI job is named so it's simple to see why it failed.)

@alanhdu
Copy link
Contributor Author

alanhdu commented Oct 17, 2018

Definitely here you about some of the lints -- I've rebased this branch and only cherry-picked the Clippy lints that I think are probably unobjectionable.

Is there perhaps some kind of conservative unopinionated clippy profile, that could be applied on the project level?

I think so, but it looks like there's no way to configure it on a per-repo level and you need to add [cfg] statements per-crate -- surprisingly, it doesn't look like there's a way to do per-repo configuration via a toml file somewhere (at least based on rust-lang/cargo#5034 and rust-lang/rust-clippy#1313).

I think not-having clippy in CI and only doing it "once in a while" is probably a good choice until lints can be configured centrally and not per-crate. I think it's reasonable to do rustfmt in CI, but don't feel strongly (for the Rust projects I work on, we have a "blessed" version of Rust to develop on, and we use the corresponding rustfmt that's rustup distributes with that version).

@alanhdu alanhdu changed the title Turn clippy and rustfmt on in CI again Fix some clippy lints Oct 17, 2018
@alanhdu
Copy link
Contributor Author

alanhdu commented Oct 18, 2018

I guess we could also configure clippy through the command line:

cargo +nightly clippy -- -A clippy::style -A clippy::nonminimal_bool

seems to work fine.

@matklad
Copy link
Member

matklad commented Oct 19, 2018

I think not-having clippy in CI and only doing it "once in a while" is probably a good choice until lints can be configured centrally and not per-crate. I think it's reasonable to do rustfmt in CI, but don't feel strongly (for the Rust projects I work on, we have a "blessed" version of Rust to develop on, and we use the corresponding rustfmt that's rustup distributes with that version).

Don't have time for a thorough respose, but if that could make work via a script in tasks which makes sure the relevant version of rustfmt is installed, etc, such that the end user can

$ git clone rust-analyzer && cd rust-analyzer
$ cargo reformat

and that there's a test in ci/tests, which fails during cargo test, that could work, yeah.

@@ -88,7 +88,7 @@ pub struct ScopeEntry {
}

impl ScopeEntry {
fn new(pat: ast::BindPat) -> Option<ScopeEntry> {
fn new_opt(pat: ast::BindPat) -> Option<ScopeEntry> {
Copy link
Member

Choose a reason for hiding this comment

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

Let's use new to name such methods? It's what stdlib does, so it looks like a bug in clippy:

https://doc.rust-lang.org/std/ffi/struct.CString.html#method.new

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this will be fixed with rust-lang/rust-clippy#3338

@@ -11,7 +11,7 @@ pub struct JobToken {
}

impl JobHandle {
pub fn new() -> (JobHandle, JobToken) {
pub fn new_pair() -> (JobHandle, JobToken) {
Copy link
Member

Choose a reason for hiding this comment

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

Agree that new probably isn't the best name here, the whole JobHandle infra will be removed once we finish migration to salsa.

@matklad
Copy link
Member

matklad commented Oct 22, 2018

bors r+

Thanks!

bors bot added a commit that referenced this pull request Oct 22, 2018
138: Fix some clippy lints r=matklad a=alanhdu

I went ahead and fixed all the clippy lints (there were a couple I thought would be better unfixed and added `cfg` statements to allow them) and also re-enabled clippy and rustfmt in CI.

They were disabled with `no time to explain, disable clippy checks`, so hopefully this won't go against whatever the reason at the time was 😆.

One question about the CI though: right now, it's an allowed failure that runs against the latest nightly each time. Would it be better to pin it to a specific nightly (or use the `beta` versions) to lower the churn?

Co-authored-by: Alan Du <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 22, 2018

Build succeeded

@bors bors bot merged commit dc9ce8f into rust-lang:master Oct 22, 2018
@alanhdu alanhdu deleted the clippy branch October 22, 2018 23:55
@matklad
Copy link
Member

matklad commented Oct 23, 2018

Filed #155 for rustfmt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants