Manual clippy fixes by lionel- · Pull Request #1111 · posit-dev/ark

lionel- · 2026-03-12T10:10:54Z

Branched from #1108
Progress towards #1104

Main lints fixed:

Too many arguments
Unnecessary try_ conversions (can just use non-failing conversions)
Large enum variants. Sometimes fixed by boxing, sometimes by disabling the lint.
Use &str instead of &String, &Path instead of &PathBuf, &[] instead of &Vec
Clippy doesn't like uppercase variants like EOF, so they've become e.g. EndOfFile
Removed common prefixes/suffixes from enum variants. I added an opt out for the LSP notification enum so we can keep the Did prefix, e.g. KernelNotification::DidOpenVirtualDocument.
into_ methods must take self. Renamed those that didn't, e.g. Dap::make_variables() which is not really an "into" method strictly speaking.
Simplified unnecessary uses of match
Use .is_some() or is_err() methods when applicable
Use ? instead of explicit return when applicable
Build regex in lazy globals

lionel- · 2026-03-12T10:12:02Z

crates/amalthea/src/kernel.rs

 }

+/// Handler implementations provided by the language runtime.
+pub struct Handlers {


These new structs fix the "too many arguments" lint.

DavisVaughan · 2026-03-13T20:13:37Z

crates/amalthea/src/socket/comm.rs

    }

    /// Send an outgoing comm message through IOPub.
+    #[allow(clippy::result_large_err)]


This is probably one I would perma-allow in Cargo.toml

Like this https://github.com/astral-sh/ruff/blob/3f96e100c8b1d5d026405db3ff5a7642c2d14763/Cargo.toml#L237

Looks like individual crates can re-opt into lints disabled at workspace level if needed, so that approach sounds good to me

DavisVaughan · 2026-03-13T20:18:52Z

crates/amalthea/src/comm.rs

+#[allow(clippy::large_enum_variant)]
 pub mod data_explorer_comm;


As of late last year, clippy has gained expect() rather than allow() which I think is quite nice.
https://users.rust-lang.org/t/dont-allow-expect-and-be-reasonable/136078

allow() says "if there is a problem, ignore it"

expect() says "there is a problem, ignore it, but also let me know if it isnt there anymore", which allows you to remove the expect() if it goes away at some point in the future

oh that's nice!

Switching to expect() already allowed finding some dangling opt-outs for deref lints, following the SEXP repr PR!

DavisVaughan · 2026-03-13T20:20:23Z

crates/amalthea/src/kernel.rs

-    iopub_tx: Sender<IOPubMessage>,
-    iopub_rx: Receiver<IOPubMessage>,
-    comm_event_rx: Receiver<CommEvent>,
-    // Receiver channel for the stdin socket; when input is needed, the
-    // language runtime can request it by sending an StdInRequest::Input to
-    // this channel. The frontend will prompt the user for input and
-    // the reply will be delivered via `stdin_reply_tx`.
-    // https://jupyter-client.readthedocs.io/en/stable/messaging.html#messages-on-the-stdin-router-dealer-channel.
-    // Note that we've extended the StdIn socket to support synchronous requests
-    // from a comm, see `StdInRequest::Comm`.
-    stdin_request_rx: Receiver<StdInRequest>,
-    // Transmission channel for StdIn replies
-    stdin_reply_tx: Sender<crate::Result<InputReply>>,
+    channels: ConnectionChannels,
 ) -> Result<(), Error> {
+    let Handlers {
+        shell_handler,
+        control_handler,
+        server_handlers,
+    } = handlers;
+    let ConnectionChannels {
+        iopub_tx,
+        iopub_rx,
+        comm_event_rx,
+        stdin_request_rx,
+        stdin_reply_tx,
+    } = channels;


To me, packing and immediately unpacking is more noise, and I'd just ignore clippy here.

I don't know, if feels a bit more tidy to me.

Although the unpacking is unnecessary, I'll remove it

DavisVaughan · 2026-03-13T20:22:36Z

crates/ark/src/console/console_repl.rs

        R_INIT.set(()).expect("`R_INIT` can only be set once");
    }

+    #[allow(clippy::too_many_arguments)]


I would personally perma ignore, I can decide if it is too many arguments or not 😛

I don't feel strongly either way so I'll perma-ignore.

I do like packed arguments in the kernel connection entry points as it makes call sites easier to read and understand. But I can see cases where that doesn't make sense.

DavisVaughan · 2026-03-13T20:25:49Z

crates/ark/src/data_explorer/format.rs

-    NULL,
-    NA,
-    NaN,
+    Null,
+    Na,
+    Nan,


Booo I like mimicking the R values!

DavisVaughan · 2026-03-13T20:26:34Z

crates/ark/src/lsp/traits/cursor.rs

+    // /// Move this cursor to the node that is at or closest to (but not after)
+    // /// the requested point.
+    // ///
+    // /// Returns `false` if the cursor is after the `point`.
+    // ///
+    // /// TODO: It would be nice to use this, but there is a bug in tree sitter
+    // /// that makes it impossible right now:
+    // /// https://github.com/tree-sitter/tree-sitter/issues/2767
+    // /// For now, use `node.find_closest_node_for_point()`.


I think you meant to remove a / not add more

nope I actually commented out the docstring

DavisVaughan · 2026-03-13T20:28:03Z

crates/ark/src/lsp/traits/point.rs

+// Point is a small Copy type, so it's fine to pass by value
+#[allow(clippy::wrong_self_convention)]


it is a little weird for is_ methods though

DavisVaughan · 2026-03-13T20:28:58Z

crates/ark/src/lsp/diagnostics.rs

-        NodeType::UnaryOperator(op) => match op {
-            UnaryOperatorType::Tilde => recurse_formula(node, context, diagnostics),
-            _ => recurse_default(node, context, diagnostics),
+        NodeType::UnaryOperator(UnaryOperatorType::Tilde) => {
+            recurse_formula(node, context, diagnostics)
        },
+        NodeType::UnaryOperator(_) => recurse_default(node, context, diagnostics),


im not really sure if i like that

DavisVaughan · 2026-03-13T20:34:24Z

crates/harp/src/size.rs

    sizeof_node: usize,
    sizeof_vector: usize,
    seen: &mut HashSet<SEXP>,
-    depth: u32,


Should this have been used?

In lobstr it's only used in debugging print statements that are commented out.

DavisVaughan · 2026-03-13T20:37:20Z

crates/libr/src/functions_variadic.rs

@@ -45,7 +46,7 @@ macro_rules! generate {
                    $(#[doc=$doc])*
                    $(#[cfg($cfg)])*
                    pub fn $name() -> bool {
-                        unsafe { matches!([<$name _opt>], Some(_)) }
+                        unsafe { (*std::ptr::addr_of!([<$name _opt>])).is_some() }


What is clippy upset about here? This seems fine to me.

Is it possible to leave this and functions.rs as is for now?

The current versions are accessing static mut references, which the Rust community has been trying to phase out (rust-lang/rust#53639). The new ones are creating raw pointers instead, to be accessed by unsafe code.

Since you wrote these macros I've reverted, but I'd have kept the new versions I think.

Actually the switch to is_some() creates a reference that is linted by rustc, not just clippy. I'd keep the new versions.

As of late last year, clippy has gained expect() rather than allow() which I think is quite nice. https://users.rust-lang.org/t/dont-allow-expect-and-be-reasonable/136078 allow() says "if there is a problem, ignore it" expect() says "there is a problem, ignore it, but also let me know if it isnt there anymore", which allows you to remove the expect() if it goes away at some point in the future

Fixes: warning: creating a shared reference to mutable static --> crates/libr/src/functions.rs:46:25 | 46 | [<$name _opt>].is_some() | ^^^^^^^^^^^^^^ shared reference to mutable static | ::: crates/libr/src/r.rs:21:1 | 21 | / functions::generate! { 22 | | pub fn R_NewEnv(enclos: SEXP, hash: std::ffi::c_int, size: std::ffi::c_int) -> SEXP; 23 | | 24 | | pub fn R_ParentEnv(x: SEXP) -> SEXP; ... | 444 | | } | |_- in this macro invocation

lionel- commented Mar 12, 2026

View reviewed changes

lionel- force-pushed the task/clippy-manual branch from e48aaf4 to 6b7a977 Compare March 12, 2026 10:50

lionel- mentioned this pull request Mar 12, 2026

Auto-fix clippy lints with --all-targets #1112

Merged

lionel- requested a review from DavisVaughan March 12, 2026 16:54

DavisVaughan mentioned this pull request Mar 13, 2026

Fix clippy notes about safety #1108

Merged

DavisVaughan approved these changes Mar 13, 2026

View reviewed changes

lionel- force-pushed the task/clippy-unsafe branch from 491100b to 7de2cc8 Compare March 16, 2026 10:49

Base automatically changed from task/clippy-unsafe to main March 16, 2026 11:09

lionel- added 22 commits March 16, 2026 12:11

Fix simple clippy issues

bcdded9

Loop over enumerate()

abb5964

Fix indentation issues in doc

86164b4

Use $crate in macro

07df502

Simplify type with alias

8a37c0d

Add missing is_empty() method

13ef615

Prefix unused parameter with _

f7e99e6

Fix large enum variant issues by boxing

c82e93d

Fix number of arguments lint

9e80902

Fix fallible conversions

6f5dff1

Remove unnecessary let ... else

419cb0a

Silence remaining large enum variant issues

b5fc60f

Use is_some() and is_err() methods

6886e09

Fix documentation issues

09c1e10

More clippy fixes

901da71

Use addr_of

082aeef

Take slices instead of Vec references

cc7553e

Fix &str and &Path references

ae2d48f

Use matches!()

a4d2b8f

Use ..Default::default()

a3b1f64

Use while let

701d74d

Remove unnecessary match

8fa8239

lionel- added 12 commits March 16, 2026 12:15

Avoid common suffix in enums

e38be76

Fix remaining clippy issues

1f4ce06

Avoid into_ prefix for non-self method

a69b8c9

Remove unnecessary unsafe blocks after rebase

1e91878

Allow large errors at workspace level

9030346

Remove dangling clippy opt-outs

4db10ab

Don't unpack right away

ca3a723

Allow large function signatures

07eedd5

Fix semantic conflicts after rebase

62aadfa

Fix safety of r_symbol! macro following rebase

fbb5fbd

lionel- force-pushed the task/clippy-manual branch from 6b7a977 to 529fc05 Compare March 16, 2026 12:28

lionel- merged commit ba23fce into main Mar 16, 2026
12 checks passed

lionel- deleted the task/clippy-manual branch March 16, 2026 12:52

github-actions bot locked and limited conversation to collaborators Mar 16, 2026

		#[allow(clippy::large_enum_variant)]
		pub mod data_explorer_comm;

		// Point is a small Copy type, so it's fine to pass by value
		#[allow(clippy::wrong_self_convention)]

Conversation

lionel- commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lionel- Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lionel- Mar 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lionel- commented Mar 12, 2026 •

edited

Loading

lionel- Mar 16, 2026 •

edited

Loading

lionel- Mar 14, 2026 •

edited

Loading