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

Triage the list of allowed lints, addressing most of them. #14

Merged
merged 24 commits into from
Dec 14, 2022
Merged

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Dec 14, 2022

In no particular order, these are the suspicious and/or not-entirely-addressed lints:

  • clippy::single_match_else: permanently #![allow(…)]ed, as the style it denies is important for readability
  • clippy::string_add: permanently #![allow(…)]ed, as it's (IMO) misguided (and perhaps buggy?)
    • (...: String) + "..." is short for { let mut s = ...; s.push_str("..."); s }, not something else
  • clippy::match_same_arms: #[allow(…)] moved to specific matches where listing out the patterns individually is more important than what the actual values are (i.e. grouping by the output values is detrimental)
  • clippy::should_implement_trait: #![allow(…)] moved to specific module encountering this bug:
  • clippy::unused_self: #[allow(…)] moved to specific impl (where self may be used in the future)
  • clippy::redundant_closure_call: #[allow(…)] moved to specific macro using a closure as a try {...} block
  • clippy::map_err_ignore: had to be worked around because the map_err(|_| it complains about have no information in the error, i.e. those Results might as well be a newtyped Option
  • clippy::nonminimal_bool: had to be worked around by introducing extra ifs (which should make the logic clearer)
    • long-term, nonminimal_bool could be a real hazard, if it suggests e.g. rewriting !(foo && foo_bar) into !foo || !foo_bar, when the point of the && is to group a foo_bar check with its foo prerequisite
      (in fact, I need to comb through Rust-GPU looking for artifacts of this lint, because I've seen in the past conditions that were rendered unreadable by an outer ! being De Morgan'd into a &&, where the outer ! may have just been coincidental from e.g. a retain/filter predicate's keep/remove distinction, but with || the correlation between the two sides was erased...)

Fixes #13

cc @Jake-Shadle (this arguably counts as feedback for Embark's list of lints)

eddyb added 24 commits December 14, 2022 23:11
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.

Review and address clippy exceptions.
1 participant