2010YOUY01 opened a new issue, #18503: URL: https://github.com/apache/datafusion/issues/18503
### Is your feature request related to a problem or challenge? `clippy::needless_pass_by_value` is a non-default clippy lint rule, however this rule is quite useful for preventing certain unnecessary clones. See more about non-default `Clippy` rules in https://github.com/apache/datafusion/issues/18467 The initial PR to enforce this rule in `datafusion-common` crate: https://github.com/apache/datafusion/pull/18468 # Task: Enforce `needless_pass_by_value` rule to all crates TODO: list the packages / find some suitable packages to start with # Rationale `clippy::needless_pass_by_value`'s description can be found in https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value. It is non-default lint rule probably it's too annoying for the general cases. Specifically, it has the Clippy category `pedantic`, and its description is lints which are rather strict or have occasional false positives` from https://doc.rust-lang.org/nightly/clippy However, the violation of this rule might further introduce unnecessary clones, and `datafusion` has been suffering from the performance issue caused by redundant clones for quite some time. (TODO: maybe we can find some issue links here) ### Example: violation of `needless_pass_by_value` that leads to an unnecessary clone ```rust #[allow(clippy::needless_pass_by_value)] fn print_len(v: Vec<i32>) { println!("{}", v.len()); } fn main() { let data = vec![1, 2, 3, 4]; // We need to CLONE because `print_len` takes ownership. print_len(data.clone()); println!("Still need data: {:?}", data); } ``` If the linter can catch such issues, there will be no unnecessary copies ```rust fn print_len(v: &[i32]) { println!("{}", v.len()); } fn main() { let data = vec![1, 2, 3, 4]; print_len(&data); // no clone, just borrow println!("Still need data: {:?}", data); } ``` ### Describe the solution you'd like Initial/example PR: https://github.com/apache/datafusion/pull/18468 1. For a certain package, add this extra lint rule in `Cargo.toml` ```toml [lints] # Overrides / extends workspace.lints [lints.clippy] # Avoid unnecessary clones for performance, okay to ignore for tests or intentional # moves. # Reference: <https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value> needless_pass_by_value = "deny" ``` If a package is too large, we can carry it out module by module like https://github.com/apache/datafusion/blob/7b5685baa7043f8a99ee6613050eeaaa575a50dc/datafusion/common/src/lib.rs#L23-L25 2. Fix violations if they might be on the performance critical path. For the following cases, it's okay to suppress lint violations using `#[expect(clippy::needless_pass_by_value)]`: - Tests - Intentional moves - Public APIs - Clearly not performance critical and also very hard to fix ### Describe alternatives you've considered _No response_ ### Additional context The individual tasks are quite AI-friendly, but make sure you understand the rationale and do a self-review before sending a PR; otherwise, the code review process can become painful. See datafusion AI PR policy for details: https://datafusion.apache.org/contributor-guide/index.html#ai-assisted-contributions -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
