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

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 `match`es
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:
  * rust-lang/rust-clippy#5004
* `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 `Result`s *might as well be a newtyped `Option`*
* `clippy::nonminimal_bool`: had to be worked around by introducing
extra `if`s (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*
<sub>(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...)</sub>

Fixes #13

src/cfg.rs

@@ -149,7 +149,7 @@ impl ControlFlowGraph {
 
     fn traverse(
         &self,
-        func_at_region: FuncAt<ControlRegion>,
+        func_at_region: FuncAt<'_, ControlRegion>,
         incoming_edge_counts: &mut EntityOrientedDenseMap<ControlRegion, IncomingEdgeCount>,
         pre_order_visit: &mut impl FnMut(ControlRegion),
         post_order_visit: &mut impl FnMut(ControlRegion),
@@ -743,7 +743,7 @@ impl<'a> Structurizer<'a> {
                 ControlInstKind::Branch => {
                     assert_eq!((inputs.len(), child_regions.len()), (0, 1));
 
-                    Ok(child_regions.into_iter().nth(0).unwrap())
+                    Ok(child_regions.into_iter().next().unwrap())
                 }
 
                 ControlInstKind::SelectBranch(kind) => {
@@ -770,8 +770,7 @@ impl<'a> Structurizer<'a> {
                         inputs: [].into_iter().collect(),
                         children: EntityList::empty(),
                         outputs: [].into_iter().collect(),
-                    }
-                    .into(),
+                    },
                 );
                 self.func_def_body
                     .unstructured_cfg
@@ -1235,8 +1234,7 @@ impl<'a> Structurizer<'a> {
                             inputs: [].into_iter().collect(),
                             children: EntityList::empty(),
                             outputs: [].into_iter().collect(),
-                        }
-                        .into(),
+                        },
                     );
                     control_source = Some(new_empty_region);
                     Some((new_empty_region, [].into_iter().collect()))

src/context.rs

@@ -365,6 +365,7 @@ impl<K: Copy + Eq + Hash, V: Default> SmallFxHashMap<K, V> {
     }
 
     fn get(&self, k: K) -> Option<&V> {
+        #[allow(clippy::match_same_arms)]
         match self {
             Self::Empty => None,
             Self::One(old_k, old_v) if *old_k == k => Some(old_v),
@@ -374,6 +375,7 @@ impl<K: Copy + Eq + Hash, V: Default> SmallFxHashMap<K, V> {
     }
 
     fn get_mut(&mut self, k: K) -> Option<&mut V> {
+        #[allow(clippy::match_same_arms)]
         match self {
             Self::Empty => None,
             Self::One(old_k, old_v) if *old_k == k => Some(old_v),

src/func_at.rs

@@ -8,6 +8,9 @@
 //!     without going through `P` (as `EntityDefs` requires keys for any access)
 //! * they're dedicated types with inherent methods and trait `impl`s
 
+// NOTE(eddyb) wrong wrt lifetimes (https://github.com/rust-lang/rust-clippy/issues/5004).
+#![allow(clippy::should_implement_trait)]
+
 use crate::{
     Context, ControlNode, ControlNodeDef, ControlRegion, ControlRegionDef, DataInst, DataInstDef,
     EntityDefs, EntityList, EntityListIter, FuncDefBody, Type, Value,
@@ -59,7 +62,7 @@ impl<'a> IntoIterator for FuncAt<'a, EntityList<ControlNode>> {
 impl<'a> Iterator for FuncAt<'a, EntityListIter<ControlNode>> {
     type Item = FuncAt<'a, ControlNode>;
     fn next(&mut self) -> Option<Self::Item> {
-        let (next, rest) = self.position.split_first(&self.control_nodes)?;
+        let (next, rest) = self.position.split_first(self.control_nodes)?;
         self.position = rest;
         Some(self.at(next))
     }
@@ -82,7 +85,7 @@ impl<'a> IntoIterator for FuncAt<'a, EntityList<DataInst>> {
 impl<'a> Iterator for FuncAt<'a, EntityListIter<DataInst>> {
     type Item = FuncAt<'a, DataInst>;
     fn next(&mut self) -> Option<Self::Item> {
-        let (next, rest) = self.position.split_first(&self.data_insts)?;
+        let (next, rest) = self.position.split_first(self.data_insts)?;
         self.position = rest;
         Some(self.at(next))
     }
@@ -167,7 +170,7 @@ impl<'a> FuncAtMut<'a, EntityList<ControlNode>> {
 // HACK(eddyb) can't implement `Iterator` because `next` borrows `self`.
 impl FuncAtMut<'_, EntityListIter<ControlNode>> {
     pub fn next(&mut self) -> Option<FuncAtMut<'_, ControlNode>> {
-        let (next, rest) = self.position.split_first(&self.control_nodes)?;
+        let (next, rest) = self.position.split_first(self.control_nodes)?;
         self.position = rest;
         Some(self.reborrow().at(next))
     }
@@ -190,7 +193,7 @@ impl<'a> FuncAtMut<'a, EntityList<DataInst>> {
 // HACK(eddyb) can't implement `Iterator` because `next` borrows `self`.
 impl FuncAtMut<'_, EntityListIter<DataInst>> {
     pub fn next(&mut self) -> Option<FuncAtMut<'_, DataInst>> {
-        let (next, rest) = self.position.split_first(&self.data_insts)?;
+        let (next, rest) = self.position.split_first(self.data_insts)?;
         self.position = rest;
         Some(self.reborrow().at(next))
     }
@@ -204,7 +207,7 @@ impl<'a> FuncAtMut<'a, DataInst> {
 
 impl FuncDefBody {
     /// Start immutably traversing the function at `position`.
-    pub fn at<'a, P: Copy>(&'a self, position: P) -> FuncAt<'a, P> {
+    pub fn at<P: Copy>(&self, position: P) -> FuncAt<'_, P> {
         FuncAt {
             control_regions: &self.control_regions,
             control_nodes: &self.control_nodes,
@@ -214,7 +217,7 @@ impl FuncDefBody {
     }
 
     /// Start mutably traversing the function at `position`.
-    pub fn at_mut<'a, P: Copy>(&'a mut self, position: P) -> FuncAtMut<'a, P> {
+    pub fn at_mut<P: Copy>(&mut self, position: P) -> FuncAtMut<'_, P> {
         FuncAtMut {
             control_regions: &mut self.control_regions,
             control_nodes: &mut self.control_nodes,
@@ -224,12 +227,12 @@ impl FuncDefBody {
     }
 
     /// Shorthand for `func_def_body.at(func_def_body.body)`.
-    pub fn at_body<'a>(&'a self) -> FuncAt<'a, ControlRegion> {
+    pub fn at_body(&self) -> FuncAt<'_, ControlRegion> {
         self.at(self.body)
     }
 
     /// Shorthand for `func_def_body.at_mut(func_def_body.body)`.
-    pub fn at_mut_body<'a>(&'a mut self) -> FuncAtMut<'a, ControlRegion> {
+    pub fn at_mut_body(&mut self) -> FuncAtMut<'_, ControlRegion> {
         self.at_mut(self.body)
     }
 }

src/lib.rs

@@ -89,31 +89,12 @@
 // END - Embark standard lints v6 for Rust 1.55+
 // crate-specific exceptions:
 #![allow(
-    // FIXME(eddyb) review all of these (some of them are intentional, sadly).
-    clippy::collapsible_if,
-    clippy::comparison_to_empty,
-    clippy::get_first,
-    clippy::iter_nth_zero,
-    clippy::map_err_ignore,
-    clippy::match_same_arms,
-    clippy::match_wildcard_for_single_variants,
-    clippy::needless_borrow,
-    clippy::needless_lifetimes,
-    clippy::nonminimal_bool,
-    clippy::redundant_closure,
-    clippy::redundant_closure_call,
-    clippy::redundant_field_names,
-    clippy::semicolon_if_nothing_returned,
-    clippy::should_implement_trait,
-    clippy::single_match,
+    // NOTE(eddyb) ignored for readability (`match` used when `if let` is too long).
     clippy::single_match_else,
+
+    // NOTE(eddyb) ignored because it's misguided to suggest `let mut s = ...;`
+    // and `s.push_str(...);` when `+` is equivalent and does not require `let`.
     clippy::string_add,
-    clippy::unimplemented,
-    clippy::unnested_or_patterns,
-    clippy::unused_self,
-    clippy::useless_conversion,
-    clippy::vtable_address_comparisons,
-    elided_lifetimes_in_paths,
 )]
 // NOTE(eddyb) this is stronger than the "Embark standard lints" above, because
 // we almost never need `unsafe` code and this is a further "speed bump" to it.

src/passes/link.rs

@@ -37,10 +37,8 @@ pub fn minimize_exports(module: &mut Module, is_root: impl Fn(&ExportKey) -> boo
         seen_funcs: FxHashSet::default(),
     };
     for (export_key, &exportee) in &module.exports {
-        if is_root(export_key) {
-            if collector.live_exports.insert(export_key.clone()) {
-                exportee.inner_visit_with(&mut collector);
-            }
+        if is_root(export_key) && collector.live_exports.insert(export_key.clone()) {
+            exportee.inner_visit_with(&mut collector);
         }
     }
     module.exports = collector

src/print/mod.rs

@@ -319,8 +319,9 @@ impl<'a> Plan<'a> {
         let (_, node_defs) = self.per_version_name_and_node_defs.last_mut().unwrap();
         match node_defs.entry(node) {
             Entry::Occupied(entry) => {
+                let dyn_data_ptr = |r| (r as *const dyn DynNodeDef<'_>).cast::<()>();
                 assert!(
-                    std::ptr::eq(*entry.get(), node_def),
+                    std::ptr::eq(dyn_data_ptr(*entry.get()), dyn_data_ptr(node_def)),
                     "print: same `{}` node has multiple distinct definitions in `Plan`",
                     node.category().unwrap_or_else(|s| s)
                 );
@@ -351,24 +352,18 @@ impl<'a> Visitor<'a> for Plan<'a> {
     }
 
     fn visit_global_var_use(&mut self, gv: GlobalVar) {
-        match self.current_module {
-            Some(module) => {
-                self.use_node(Node::GlobalVar(gv), &module.global_vars[gv]);
-            }
-
+        if let Some(module) = self.current_module {
+            self.use_node(Node::GlobalVar(gv), &module.global_vars[gv]);
+        } else {
             // FIXME(eddyb) should this be a hard error?
-            None => {}
         }
     }
 
     fn visit_func_use(&mut self, func: Func) {
-        match self.current_module {
-            Some(module) => {
-                self.use_node(Node::Func(func), &module.funcs[func]);
-            }
-
+        if let Some(module) = self.current_module {
+            self.use_node(Node::Func(func), &module.funcs[func]);
+        } else {
             // FIXME(eddyb) should this be a hard error?
-            None => {}
         }
     }
 
@@ -455,7 +450,7 @@ pub enum Versions<PF> {
 }
 
 impl fmt::Display for Versions<pretty::FragmentPostLayout> {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             Self::Single(fragment) => fragment.fmt(f),
             Self::Multiple {
@@ -851,7 +846,7 @@ impl<'a> Printer<'a> {
                 });
 
             for func_def_body in func_def_bodies_across_versions {
-                let visit_region = |func_at_region: FuncAt<ControlRegion>| {
+                let visit_region = |func_at_region: FuncAt<'_, ControlRegion>| {
                     let region = func_at_region.position;
 
                     define_label_or_value(Use::ControlRegionLabel(region));
@@ -921,10 +916,14 @@ impl<'a> Printer<'a> {
     pub fn cx(&self) -> &'a Context {
         self.cx
     }
+}
 
-    // Styles for a variety of syntactic categories.
-    // FIXME(eddyb) this is a somewhat inefficient way of declaring these.
-
+// Styles for a variety of syntactic categories.
+// FIXME(eddyb) this is a somewhat inefficient way of declaring these.
+//
+// NOTE(eddyb) these methods take `self` so they can become configurable in the future.
+#[allow(clippy::unused_self)]
+impl Printer<'_> {
     fn error_style(&self) -> pretty::Styles {
         pretty::Styles::color(pretty::palettes::simple::MAGENTA)
     }
@@ -969,7 +968,9 @@ impl<'a> Printer<'a> {
             ..Default::default()
         }
     }
+}
 
+impl<'a> Printer<'a> {
     /// Pretty-print a `: T` style "type ascription" suffix.
     ///
     /// This should be used everywhere some type ascription notation is needed,
@@ -1191,7 +1192,7 @@ impl Use {
                     let func_category = func.category();
                     let func_idx = match printer.use_styles[&func] {
                         UseStyle::Anon { idx, .. } => idx,
-                        _ => unreachable!(),
+                        UseStyle::Inline => unreachable!(),
                     };
                     format!("{func_category}{func_idx}.{name}")
                 } else {
@@ -1340,7 +1341,7 @@ impl Print for Plan<'_> {
             });
 
         // Unversioned, flatten the nodes.
-        if num_versions == 1 && self.per_version_name_and_node_defs[0].0 == "" {
+        if num_versions == 1 && self.per_version_name_and_node_defs[0].0.is_empty() {
             Versions::Single(pretty::Fragment::new(
                 per_node_versions_with_repeat_count
                     .map(|mut versions_with_repeat_count| {
@@ -1383,7 +1384,7 @@ impl Print for Module {
                     .iter()
                     .map(|(export_key, exportee)| {
                         pretty::Fragment::new([
-                            export_key.print(printer).into(),
+                            export_key.print(printer),
                             ": ".into(),
                             exportee.print(printer),
                         ])
@@ -1995,31 +1996,27 @@ impl Print for ConstDef {
 
         AttrsAndDef {
             attrs: attrs.print(printer),
-            def_without_name: if let Some(def) = compact_def {
-                def.into()
-            } else {
-                match *ctor {
-                    ConstCtor::PtrToGlobalVar(gv) => {
-                        pretty::Fragment::new(["&".into(), gv.print(printer)])
-                    }
-                    ConstCtor::SpvInst(spv::Inst { opcode, ref imms }) => printer.pretty_spv_inst(
-                        printer.declarative_keyword_style(),
-                        opcode,
-                        imms,
-                        ctor_args,
-                        Print::print,
-                        Some(*ty),
-                    ),
-                    ConstCtor::SpvStringLiteralForExtInst(s) => pretty::Fragment::new([
-                        printer.declarative_keyword_style().apply("OpString"),
-                        "<".into(),
-                        printer
-                            .string_literal_style()
-                            .apply(format!("{:?}", &printer.cx[s])),
-                        ">".into(),
-                    ]),
+            def_without_name: compact_def.unwrap_or_else(|| match *ctor {
+                ConstCtor::PtrToGlobalVar(gv) => {
+                    pretty::Fragment::new(["&".into(), gv.print(printer)])
                 }
-            },
+                ConstCtor::SpvInst(spv::Inst { opcode, ref imms }) => printer.pretty_spv_inst(
+                    printer.declarative_keyword_style(),
+                    opcode,
+                    imms,
+                    ctor_args,
+                    Print::print,
+                    Some(*ty),
+                ),
+                ConstCtor::SpvStringLiteralForExtInst(s) => pretty::Fragment::new([
+                    printer.declarative_keyword_style().apply("OpString"),
+                    "<".into(),
+                    printer
+                        .string_literal_style()
+                        .apply(format!("{:?}", &printer.cx[s])),
+                    ">".into(),
+                ]),
+            }),
         }
     }
 }
@@ -2130,7 +2127,7 @@ impl Print for FuncDecl {
 
         let def_without_name = match def {
             DeclDef::Imported(import) => {
-                pretty::Fragment::new([sig, " = ".into(), import.print(printer).into()])
+                pretty::Fragment::new([sig, " = ".into(), import.print(printer)])
             }
 
             // FIXME(eddyb) this can probably go into `impl Print for FuncDefBody`.
@@ -2560,7 +2557,7 @@ impl Print for cfg::ControlInst {
 
             cfg::ControlInstKind::Branch => {
                 assert_eq!((targets.len(), inputs.len()), (1, 0));
-                targets.nth(0).unwrap()
+                targets.next().unwrap()
             }
 
             cfg::ControlInstKind::SelectBranch(kind) => {