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

Linting #1114

Closed
wants to merge 51 commits into from
Closed

Linting #1114

wants to merge 51 commits into from

Conversation

jhpratt
Copy link
Contributor

@jhpratt jhpratt commented Sep 2, 2019

Most of these lints are either built into rustc (and are allowed by default) or are available via clippy. A couple I've done by hand by using ripgrep to find instances (an example of this is use foo declarations, which isn't available via tooling).

Commit messages, which describe what was changed:

  • Remove unnecessary braces on import

  • Use shorter path where already imported
    In these situations, the relevant path is already imported into the current scope. As such, we can remove parts of the path without cost.

  • Remove use foo, which is a noop
    This is likely left over from the conversion to 2018, where extern crate would have been converted to this.

  • Use struct field shorthand where possible

  • Remove identity function
    .params() already returns an iterator

  • Restrict visibility where not externally visible
    Use crate over pub. This way, rustc can let us know if something is actually unused, rather than assuming it may be used publicly.

  • Remove self from path where possible
    Doing this is allowed in newer versions of the compiler.

schrieveslaach and others added 30 commits July 19, 2019 17:40
- Use hyper's MakeService implementation with futures API
- Use tokio runtime to serve HTTP backend
This lets us keep support for keep-alive and remote address while doing
other work on async, at the cost of TLS. Abstracting over the connection
type will be done more thoroughly later.
This requires some awkward channel and spawning work because
Body might contain borrowed data.
Adds body_string_wait and body_bytes_wait functions to LocalRequest for convenience.
This is required to be able to do anything useful with the body in the
outgoing response. Request fairings do not appear to need to be async
as everything on Data that returns a future moves self and on_request only
gets &Data, but the same change in this commit should work for on_request
if desired.
Like with response fairings, this is required to be able to do anything
useful with the body.
Also update 'static_files' example.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
This adds an unstable "async_test" function for use in tests, to ensure
they stay working while refactoring other APIs.
… operations.

Despite this change, using body_bytes_wait on (for example) a File will
still fail due to tokio-rs/tokio#1356.
jebrosen and others added 17 commits August 24, 2019 14:59
* body_string_wait and body_bytes_wait are removed; use `.await` instead
* `dispatch()` is now an async fn and must be .await-ed
* Add `#[rocket::async_test]` macro, similar in purpose to `tokio::test`
* Tests in core use either `rocket::async_test(async { })` or
  `#[rocket::async_test]` in order to `.await` the futures returned
  from `dispatch()` and `body_{string,bytes}()`

Broken:

* Cloned dispatch and mut_dispatch() with a live previous response now both fail, due to a (partial) check for mutable aliasing in LocalRequest.
…mples.

Some tests are still failing and need example-specific changes.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Additionally pin tokio to '=0.2.0-alpha.2'; before this change cargo
selects '0.2.0-alpha.3' which hyper does not compile against.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt
Doing this is allowed in newer versions of the compiler.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt
Use `crate` over `pub`. This way, rustc can let us know if something is
actually unused, rather than assuming it may be used publicly.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt
`.params()` already returns an iterator

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt
This is likely left over from the conversion to 2018, where `extern
crate` would have been converted to this.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt
In these situations, the relevant path is already imported into the
current scope. As such, we can remove parts of the path without cost.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt
Using an ident in a match arm binds the value, rather than matching on
type, which is what is wanted here. To match on type, we need to use the
path.
@jebrosen
Copy link
Collaborator

jebrosen commented Sep 2, 2019

rustc reported all branches other than the first as unreachable, with the reason being that it was matched on an ident.

The enum variants in question should still be in scope there, so that is concerning. Which lint says this?

Which lints were all of these, in general? We may want to #[warn()] on some of them more permanently, at least the ones don't require clippy.

Remove self from path where possible
Doing this is allowed in newer versions of the compiler.

Is it a newer version than we already require? I think it's the same version #964 bumped to (uniform_paths?), but I want to be sure.


I think I will want to hold off on some of the more disruptive changes until async is merged into master, to avoid making that task much harder. But a lot of these look fine.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt
@jhpratt
Copy link
Contributor Author

jhpratt commented Sep 2, 2019

The non-clippy lints come from this list. I enabled them one at a time to avoid information overload; there's a few false positives.

rustc lints

  • anonymous_parameters
    No warnings.

  • bare_trait_objects & elided_lifetimes_in_paths
    I believe this falls under rust_2018_idoms, which is already warn. As such, no warnings.

  • missing_copy_implementations
    3 warnings. I don't believe any of these strictly need copy, as they're only really used in situations where a specific structure is needed (and not shared).

  • missing_debug_implementations
    14 warnings. No harm in adding these in where possible, but I didn't do so.

  • missing_docs
    7 warnings. All are new in the async migration.

  • single_use_lifetimes
    Many warnings. After I tried a few situations, I believe all are false positives.

  • trivial_casts
    6 warnings, all false positives.

  • trivial_numeric_casts
    No warnings.

  • unreachable_pub
    Tons of warnings. This lint is one I definitely wish was enabled by default. This is where I replace pub with crate.

  • unsafe_code
    One warning, the usage of unsafe is documented.

  • unused_extern_crates
    No warnings, as extern crate is rarely used.

  • unused_import_braces
    Two warnings. Strictly stylistic, but trivial to fix.

  • unused_qualifications
    10 warnings. All of these are changed except the one inside a macro.

  • unused_results
    Plenty of warnings, I didn't bother to touch any of these.

  • variant_size_differences
    No warnings. Could be good to enable to ensure memory usage doesn't grow a lot unexpectedly.

clippy lints

Just including the ones that have warnings.

  • redundant_field_names
    Stylistic. Trivial to fix.

  • module_inception
    A few warnings, but I don't think it's a big deal.

  • large_enum_variant
    Apparently a different threshold than rustc.

  • identity_conversion
    Zero reason to not straight up deny this one, it's literally useless. One warning, which is fixed.

  • should_implement_trait
    Currently warning on an into_iter() method. While it wouldn't hurt to implement the trait, it would require a bit of work. There's also a from_str() method, though the return type is not the same.

  • if_same_then_else
    I don't think this is an issue, as the compiler almost certainly optimizes this away. It allows for clearer commenting.

  • redundant_pattern_matching
    Useless lint here, as it's exclusively because of pear's proc macros.

  • cast_lossless
    Wouldn't hurt to change this, though I didn't in this PR.


Is it a newer version than we already require? I think it's the same version #964 bumped to (uniform_paths?), but I want to be sure.

Nope. I don't remember the minimum nightly, I know we require at least an August compiler because of async/await stabilization, and I have used this before then on my own projects. #964 sounds right.

I think I will want to hold off on some of the more disruptive changes until async is merged into master, to avoid making that task much harder. But a lot of these look fine.

Fair. I'm sure it'll already be quite interesting to merge the branch in.

The enum variants in question should still be in scope there, so that is concerning. Which lint says this?

Looking into this again, I believe I was mistaken. I recall experimentally removing the use statement in the beginning of the file, which naturally would have taken them out of scope. I must have changed this before re-adding the use. I've just pushed a commit reverting that change.

@jhpratt
Copy link
Contributor Author

jhpratt commented Sep 9, 2019

Per a brief discussion on IRC/Matrix, this will be held off on until after merging async into master to make things a bit easier. At that point, I'll likely force push similar changes, if Jeb doesn't mind it being a single PR (something he mentioned). Otherwise, I'll split it apart and create multiple PRs.

@SergioBenitez
Copy link
Member

Closing this out to clear the queue. Let's resume the discussion once async is merged into master.

@SergioBenitez SergioBenitez added the pr: closed This pull request was not merged label Jun 22, 2020
@jhpratt
Copy link
Contributor Author

jhpratt commented Jun 22, 2020

Sounds good @SergioBenitez. Let me know when async gets merged and I'll create another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: closed This pull request was not merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants