Make Clippy happier

[hunger]

This gets us down from 688 warnings to less than 50. Lots of changes, but all pretty straight forward and localized.

• 2 missing Safety sections for private crates
• Several API naming issues like "from_* should not take self" and "Should implement default() for Backend that implements new()", etc.
• stray main functions in doctests (which we want as that documents how to use things)
• two redundant clones (which I think are not redundant enough;-)
• Holding on to a RefCell across await points in the LSP and in compiler/passes.rs
• Functions with too many arguments (should we just silence that warning?)
• A match that could be written as a let statement (except that we need to clone then to keep a reference alive)
• Very complex types being used (should we silence that warning?)

That's it... The RefCall across await points is something we should eventually look into, but nothing I want to do close to release:-)

We are getting close to a point where it makes sense to run clippy without drowning in noise!

[hunger]

Still 12 warnings to go, mostly related to function naming conventions and the RefCell over await point issues:

warning: methods called `from_*` usually take no `self`
  --> internal\core\translations.rs:15:23
   |
15 |         fn from_index(&self, index: usize) -> Option<Self::Output<'_>>;
   |                       ^^^^^
   |
   = help: consider choosing a less ambiguous name
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention


warning: methods called `from_*` usually take no `self`
  --> internal\core\translations.rs:16:22
   |
16 |         fn from_name(&self, _name: &str) -> Option<Self::Output<'_>> {
   |                      ^^^^^
   |
   = help: consider choosing a less ambiguous name
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention


warning: methods called `new` usually return `Self`
   --> internal\backends\winit\winitwindowadapter.rs:132:5
    |
132 | /     pub(crate) fn new<R: WinitCompatibleRenderer + 'static>(
133 | |         #[cfg(target_arch = "wasm32")] canvas_id: &str,
134 | |     ) -> Result<Rc<dyn WindowAdapter>, PlatformError> {
135 | |         let (renderer, winit_window) = Self::window_builder(
...   |
173 | |         Ok(self_rc as _)
174 | |     }
    | |_____^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_ret_no_self
    = note: `#[warn(clippy::new_ret_no_self)]` on by default


warning: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value
   --> internal\core\api.rs:142:29
    |
142 |     pub(crate) fn to_euclid(&self) -> crate::lengths::LogicalSize {
    |                             ^^^^^
    |
    = help: consider choosing a less ambiguous name
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention


warning: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value
  --> internal\core\api.rs:47:29
   |
47 |     pub(crate) fn to_euclid(&self) -> crate::lengths::LogicalPoint {
   |                             ^^^^^
   |
   = help: consider choosing a less ambiguous name
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
   = note: `#[warn(clippy::wrong_self_convention)]` on by default


warning: this `RefCell` reference is held across an `await` point
    --> tools\lsp\server_loop.rs:1244:31
     |
1244 |     let document_cache = &mut ctx.document_cache.borrow_mut();
     |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: ensure the reference is dropped before calling `await`
note: these are all the `await` points this reference is held through
    --> tools\lsp\server_loop.rs:1244:5
     |
1244 | /     let document_cache = &mut ctx.document_cache.borrow_mut();
1245 | |     for v in r {
1246 | |         if let Some(o) = v.as_object() {
1247 | |             if let Some(ip) = o.get("includePath").and_then(|v| v.as_array()) {
...    |
1273 | |     Ok(())
1274 | | }
     | |_^
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_refcell_ref
     = note: `#[warn(clippy::await_holding_refcell_ref)]` on by default


warning: this `RefCell` reference is held across an `await` point
   --> tools\lsp\main.rs:316:22
    |
316 |                 &mut ctx.document_cache.borrow_mut(),
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: ensure the reference is dropped before calling `await`
note: these are all the `await` points this reference is held through
   --> tools\lsp\main.rs:311:13
    |
311 | /             reload_document(
312 | |                 &ctx.server_notifier,
313 | |                 params.text_document.text,
314 | |                 params.text_document.uri,
...   |
317 | |             )
318 | |             .await?;
    | |____________________^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_refcell_ref


warning: this `RefCell` reference is held across an `await` point
  --> internal\compiler\passes.rs:84:23
   |
84 |     for component in (root_component.used_types.borrow().sub_components.iter())
   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: ensure the reference is dropped before calling `await`
note: these are all the `await` points this reference is held through
  --> internal\compiler\passes.rs:84:5
   |
84 | /     for component in (root_component.used_types.borrow().sub_components.iter())
85 | |         .chain(std::iter::once(root_component))
86 | |     {
87 | |         compile_paths::compile_paths(component, &doc.local_registry, diag);
...  |
95 | |         lower_text_input_interface::lower_text_input_interface(component);
96 | |     }
   | |_____^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_refcell_ref
   = note: `#[warn(clippy::await_holding_refcell_ref)]` on by default


warning: this expression creates a reference which is immediately dereferenced by the compiler
   --> internal\core\translations.rs:187:44
    |
187 |     write!(output, "{}", formatter::format(&translated, &WithPlural(arguments, n))).unwrap();
    |                                            ^^^^^^^^^^^ help: change this to: `translated`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
    = note: `#[warn(clippy::needless_borrow)]` on by default


warning: this match could be written as a `let` statement
   --> internal\compiler\passes\binding_analysis.rs:308:5
    |
308 | /     let depends_on_external = match prop
309 | |         .prop
310 | |         .element()
311 | |         .borrow()
...   |
320 | |         }
321 | |     };
    | |______^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#match_single_binding
    = note: `#[warn(clippy::match_single_binding)]` on by default
help: consider using a `let` statement
    |
308 ~     let a = prop
309 +         .prop
310 +         .element()
311 +         .borrow()
312 +         .property_analysis
313 +         .borrow_mut()
314 +         .entry(prop.prop.name().into())
315 +         .or_default();
316 +     let depends_on_external = {
317 +         a.is_read = true;
318 +         DependsOnExternal(prop.elements.is_empty() && a.is_set_externally)
319 +     };
    |


warning: you should consider adding a `Default` implementation for `Backend`
   --> internal\backends\qt\lib.rs:133:5
    |
133 | /     pub fn new() -> Self {
134 | |         #[cfg(not(no_qt))]
135 | |         {
136 | |             use cpp::cpp;
...   |
143 | |         Self {}
144 | |     }
    | |_____^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
    = note: `#[warn(clippy::new_without_default)]` on by default
help: try adding this
    |
132 + impl Default for Backend {
133 +     fn default() -> Self {
134 +         Self::new()
135 +     }
136 + }
    |


warning: you should consider adding a `Default` implementation for `Backend`
   --> internal\backends\winit\lib.rs:147:5
    |
147 | /     pub fn new() -> Self {
148 | |         Self::new_with_renderer_by_name(None)
149 | |     }
    | |_____^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
    = note: `#[warn(clippy::new_without_default)]` on by default
help: try adding this
    |
144 + impl Default for Backend {
145 +     fn default() -> Self {
146 +         Self::new()
147 +     }
148 + }
    |

Note that changing

warning: this expression creates a reference which is immediately dereferenced by the compiler
   --> internal\core\translations.rs:187:44

results in a build failure :-/

[hunger]

It would be great if there was a cross-workspace way to disable some lints. There is not though, so tweaking clippy.toml to make two tests less strict is the best I can do project wide.

[hunger]

I think I fixed all the non-clippy related things that were commented on. I am not sure what to do about the things clippy actually complains about and that were not universally liked: There is not that much I can do about that:-/

We can disable individual warnings, but only on a per-crate basis. I really do not want to go around and add the same set disable-clippy-flags into all our crates. I also do not want to disable clippy for individual statements: The boilerplate is more ugly than the change clippy encourages.

I also am no fan of some script that runs clippy with special flags: cargo clippy should just-work(TM).

[tronical]

Both of my comments are marked as resolved but the code is still exactly the same and I don't see a response. Did you choose to ignore them?

[hunger]

@tronical: I can make either clippy happy or you:-( So I ignored them, even though I should change that conditional as you suggested. I'll do so right away.

If the goal is to make clippy happy, then I have the following options:

• Change as clippy suggested. That will make a few places worse, but overall I think the clippy lints are helpful.
• Keep the code as is and put a #[allow(clippy::specific_lint_name)] in front. That is really ugly and will annoy all of us whenever we see it.
• Disable a specific clippy lint for an entire crate: Lots of #[allow(...)] boilerplate in all the lib.rs/main.rs files. Less annoying, but not really an option for lints that are overall helpful but fail in a few specific cases.

The first option is the least annoying overall for me.

About tweaking clippy for our project: There is no good way to set up clippy for a workspace at this time. There are tons of issues open with clippy, cargo and rustc, but no good solution.

The best option right now seems to hard-code compiler flags into the project-wide .cargo/config file. That can cause issues though as the compiler will error out when presented with unknown options. So we can not disable any clippy lint that are unknown to our MSRV.

I already did a bit of tweaking with .clippy.toml. That does support to tweak some settings used when running lints (I did that to silence warning about functions taking too many arguments already), but does not allow to disable certain lints entirely.

We could have a script to run clippy with the configuration we want. I do not like that too much though as that does not help when somebody runs cargo clippy (and who checks docs to find scripts for such standard tasks?).

Finally we could copy/paste the same boilerplate clippy configuration into all our lib.rs/main.rs files. That will be annoying to do, but maybe some xtask could make that bearable?

[tronical]

@tronical: I can make either clippy happy or you:-( So I ignored them, even though I should change that conditional as you suggested. I'll do so right away.

I don't feel too strong about it, I was just curious what you thought about the comments. Silently resolving them gave a third state that I didn't expect.

I'm fine with keeping clippy happy.

[ogoffart]

There is the option to change the code in a way that's good while keeping clippy happy.
If that means using #[allow(clippy::...] , then do that

[hunger]

I do not think any of these makes the code worse, but some do not improve the code either.

I understood most comments here as clarification as to why clippy suggested a change and only a few as actual complaints about how the code looks like now.