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

Cleanup #69

Merged
merged 7 commits into from
Mar 27, 2022
Merged

Cleanup #69

merged 7 commits into from
Mar 27, 2022

Conversation

Erik1000
Copy link
Contributor

I wanted to contribute to this crate (#68) and while attempting to do so, rust-analyzer (with clippy) complaint. I thought, I could use this chance to fix the clippy errors and do some clean up.

@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #69 (0d38b9a) into main (4a29dfd) will increase coverage by 0.59%.
The diff coverage is 82.35%.

❗ Current head 0d38b9a differs from pull request most recent head 03ed365. Consider uploading reports for the commit 03ed365 to get more accurate results

@@            Coverage Diff             @@
##             main      #69      +/-   ##
==========================================
+ Coverage   76.71%   77.30%   +0.59%     
==========================================
  Files          16       16              
  Lines        3504     3494      -10     
==========================================
+ Hits         2688     2701      +13     
+ Misses        816      793      -23     
Impacted Files Coverage Δ
src/http_utils.rs 0.00% <0.00%> (ø)
src/user_info.rs 34.61% <33.33%> (ø)
src/core/jwk.rs 91.63% <66.66%> (+0.55%) ⬆️
src/registration.rs 67.02% <66.66%> (+0.14%) ⬆️
src/claims.rs 37.03% <100.00%> (+23.45%) ⬆️
src/core/crypto.rs 81.53% <100.00%> (ø)
src/verification.rs 86.18% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a29dfd...03ed365. Read the comment docs.

@Erik1000
Copy link
Contributor Author

CI fails because of ring (rust-lang/rust#95267)

@ramosbugs
Copy link
Owner

Thanks for the cleanup! I'll take a look once nightly gets fixed (sounds like early next week). My main concern about fixing clippy lints is that newer versions of clippy may suggest changes involving newer language features that break compatibility with the MSRV 1.45.0. As long as CI passes, the changes should be fine though.

Also, feel free to add fail-fast: false to the GitHub Actions job if you don't want to wait for the nightly fix. I think if I add that to the main branch right now it won't get pulled in unless you merge or rebase.

@Erik1000
Copy link
Contributor Author

error: use of `unwrap_or` followed by a function call
  --> src/http_utils.rs:17:31
   |
17 |             ct[..ct.find(';').unwrap_or(ct.len())].to_lowercase() == expected_essence.to_lowercase()
   |                               ^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| ct.len())`
   |
   = note: `-D clippy::or-fun-call` implied by `-D warnings`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call

We now have the situation that the clippy version from MSVC does not recognize that ct.len() is a const function (it is since 1.39, MSVC is 1.45) and therefore thinks that this is unwrap_or with a function call. The latest clippy however does recognize that and warns when using unwrap_or_else since it is not needed:

warning: unnecessary closure used to substitute value for `Option::None`
  --> src/http_utils.rs:17:18
   |
17 |             ct[..ct.find(';').unwrap_or_else(|| ct.len())].to_lowercase()
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `unwrap_or` instead: `ct.find(';').unwrap_or(ct.len())`
   |
   = note: `#[warn(clippy::unnecessary_lazy_evaluations)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations

Using unwrap_or is -- no matter the compiler version -- the right choice, but what should we do about the false positive with the old clippy version?

@Erik1000
Copy link
Contributor Author

Related to rust-lang/rust-clippy#6943

@ramosbugs
Copy link
Owner

I think let's go with unwrap_or and annotate the line with #[allow(clippy::or_fun_call)] so that clippy 1.45.0 doesn't complain. that should make CI happy (other than the known ring issue on nightly).

@Erik1000 Erik1000 mentioned this pull request Mar 27, 2022
@ramosbugs ramosbugs merged commit 9642f22 into ramosbugs:main Mar 27, 2022
@ramosbugs
Copy link
Owner

thanks!

@Erik1000 Erik1000 deleted the fix-clippy branch March 27, 2022 21:23
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.

2 participants