new lint: manual_c_str_literals

y21 · y21 · commit d66b5aee4163 · 2023-12-04T08:22:21.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5220,6 +5220,7 @@ Released 2018-09-13
 [`manual_assert`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert
 [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
 [`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits
+[`manual_c_str_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_c_str_literals
 [`manual_clamp`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp
 [`manual_filter`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter
 [`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map
diff --git a/clippy_config/src/msrvs.rs b/clippy_config/src/msrvs.rs
@@ -16,6 +16,7 @@ macro_rules! msrv_aliases {
 
 // names may refer to stabilized feature flags or library items
 msrv_aliases! {
+    1,76,0 { C_STR_LITERALS }
     1,71,0 { TUPLE_ARRAY_CONVERSIONS, BUILD_HASHER_HASH_ONE }
     1,70,0 { OPTION_IS_SOME_AND, BINARY_HEAP_RETAIN }
     1,68,0 { PATH_MAIN_SEPARATOR_STR }
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -379,6 +379,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::ITER_SKIP_ZERO_INFO,
     crate::methods::ITER_WITH_DRAIN_INFO,
     crate::methods::JOIN_ABSOLUTE_PATHS_INFO,
+    crate::methods::MANUAL_C_STR_LITERALS_INFO,
     crate::methods::MANUAL_FILTER_MAP_INFO,
     crate::methods::MANUAL_FIND_MAP_INFO,
     crate::methods::MANUAL_NEXT_BACK_INFO,
diff --git a/clippy_lints/src/methods/manual_c_str_literals.rs b/clippy_lints/src/methods/manual_c_str_literals.rs
@@ -0,0 +1,114 @@
+use clippy_config::msrvs::{self, Msrv};
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::get_parent_expr;
+use clippy_utils::source::snippet;
+use rustc_ast::{LitKind, StrStyle};
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, QPath, TyKind};
+use rustc_lint::LateContext;
+use rustc_span::{sym, Span};
+
+use super::MANUAL_C_STR_LITERALS;
+
+pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, func: &Expr<'_>, args: &[Expr<'_>], msrv: &Msrv) {
+    if let ExprKind::Path(QPath::TypeRelative(cstr, fn_name)) = &func.kind
+        && let TyKind::Path(QPath::Resolved(_, ty_path)) = &cstr.kind
+        && cx.tcx.lang_items().c_str() == ty_path.res.opt_def_id()
+        && let [arg] = args
+        && msrv.meets(msrvs::C_STR_LITERALS)
+    {
+        match fn_name.ident.name.as_str() {
+            name @ ("from_bytes_with_nul" | "from_bytes_with_nul_unchecked")
+                if !arg.span.from_expansion()
+                    && let ExprKind::Lit(lit) = arg.kind
+                    && let LitKind::ByteStr(_, StrStyle::Cooked) = lit.node =>
+            {
+                check_from_bytes(cx, expr, arg, name);
+            },
+            "from_ptr" => check_from_ptr(cx, expr, arg),
+            _ => {},
+        }
+    }
+}
+
+/// Checks `CStr::from_bytes_with_nul(b"foo\0")`
+fn check_from_ptr(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>) {
+    if let ExprKind::MethodCall(method, lit, [], _) = peel_ptr_cast(arg).kind
+        && method.ident.name == sym::as_ptr
+        && !lit.span.from_expansion()
+        && let ExprKind::Lit(lit) = lit.kind
+        && let LitKind::ByteStr(_, StrStyle::Cooked) = lit.node
+    {
+        span_lint_and_sugg(
+            cx,
+            MANUAL_C_STR_LITERALS,
+            expr.span,
+            "calling `CStr::from_ptr` with a byte string literal",
+            r#"use a `c""` literal"#,
+            rewrite_as_cstr(cx, lit.span),
+            Applicability::MachineApplicable,
+        );
+    }
+}
+
+/// Checks `CStr::from_ptr(b"foo\0".as_ptr().cast())`
+fn check_from_bytes(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, method: &str) {
+    let (span, applicability) = if let Some(parent) = get_parent_expr(cx, expr)
+        && let ExprKind::MethodCall(method, ..) = parent.kind
+        && [sym::unwrap, sym::expect].contains(&method.ident.name)
+    {
+        (parent.span, Applicability::MachineApplicable)
+    } else if method == "from_bytes_with_nul_unchecked" {
+        // `*_unchecked` returns `&CStr` directly, nothing needs to be changed
+        (expr.span, Applicability::MachineApplicable)
+    } else {
+        // User needs to remove error handling, can't be machine applicable
+        (expr.span, Applicability::MaybeIncorrect)
+    };
+
+    span_lint_and_sugg(
+        cx,
+        MANUAL_C_STR_LITERALS,
+        span,
+        "calling `CStr::new` with a byte string literal",
+        r#"use a `c""` literal"#,
+        rewrite_as_cstr(cx, arg.span),
+        applicability,
+    );
+}
+
+/// Rewrites a byte string literal to a c-str literal.
+/// `b"foo\0"` -> `c"foo"`
+pub fn rewrite_as_cstr(cx: &LateContext<'_>, span: Span) -> String {
+    let mut sugg = String::from("c") + snippet(cx, span.source_callsite(), "..").trim_start_matches('b');
+
+    // NUL byte should always be right before the closing quote.
+    // (Can rfind ever return `None`?)
+    if let Some(quote_pos) = sugg.rfind('"') {
+        // Possible values right before the quote:
+        // - literal NUL value
+        if sugg.as_bytes()[quote_pos - 1] == b'\0' {
+            sugg.remove(quote_pos - 1);
+        }
+        // - \x00
+        else if sugg[..quote_pos].ends_with("\\x00") {
+            sugg.replace_range(quote_pos - 4..quote_pos, "");
+        }
+        // - \0
+        else if sugg[..quote_pos].ends_with("\\0") {
+            sugg.replace_range(quote_pos - 2..quote_pos, "");
+        }
+    }
+
+    sugg
+}
+
+/// `x.cast()` -> `x`
+/// `x as *const _` -> `x`
+fn peel_ptr_cast<'tcx>(e: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
+    match &e.kind {
+        ExprKind::MethodCall(method, receiver, [], _) if method.ident.as_str() == "cast" => peel_ptr_cast(receiver),
+        ExprKind::Cast(expr, _) => peel_ptr_cast(expr),
+        _ => e,
+    }
+}
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -50,6 +50,7 @@ mod iter_skip_zero;
 mod iter_with_drain;
 mod iterator_step_by_zero;
 mod join_absolute_paths;
+mod manual_c_str_literals;
 mod manual_next_back;
 mod manual_ok_or;
 mod manual_saturating_arithmetic;
@@ -3752,6 +3753,33 @@ declare_clippy_lint! {
     "using `Option.map_or(Err(_), Ok)`, which is more succinctly expressed as `Option.ok_or(_)`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for calls to `CStr::from_ptr` and `CStr::from_bytes_with_nul` with byte string literals as arguments.
+    ///
+    /// ### Why is this bad?
+    /// This can be written more concisely using `c"str"` literals and is also less error-prone,
+    /// because the compiler checks for interior nul bytes.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// # use std::ffi::CStr;
+    /// # fn needs_cstr(_: &CStr) {}
+    /// needs_cstr(CStr::from_bytes_with_nul(b":)").unwrap());
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// # #![feature(c_str_literals)]
+    /// # use std::ffi::CStr;
+    /// # fn needs_cstr(_: &CStr) {}
+    /// needs_cstr(c":)");
+    /// ```
+    #[clippy::version = "1.76.0"]
+    pub MANUAL_C_STR_LITERALS,
+    pedantic,
+    r#"creating a `CStr` through functions when `c""` literals can be used"#
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -3903,6 +3931,7 @@ impl_lint_pass!(Methods => [
     UNNECESSARY_FALLIBLE_CONVERSIONS,
     JOIN_ABSOLUTE_PATHS,
     OPTION_MAP_OR_ERR_OK,
+    MANUAL_C_STR_LITERALS,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -3930,6 +3959,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             hir::ExprKind::Call(func, args) => {
                 from_iter_instead_of_collect::check(cx, expr, args, func);
                 unnecessary_fallible_conversions::check_function(cx, expr, func);
+                manual_c_str_literals::check(cx, expr, func, args, &self.msrv);
             },
             hir::ExprKind::MethodCall(method_call, receiver, args, _) => {
                 let method_span = method_call.ident.span;
diff --git a/tests/ui/manual_c_str_literals.fixed b/tests/ui/manual_c_str_literals.fixed
@@ -0,0 +1,51 @@
+#![feature(c_str_literals)] // TODO: remove in the next sync
+#![warn(clippy::manual_c_str_literals)]
+#![allow(clippy::no_effect)]
+
+use std::ffi::CStr;
+
+macro_rules! cstr {
+    ($s:literal) => {
+        CStr::from_bytes_with_nul(concat!($s, "\0").as_bytes()).unwrap()
+    };
+}
+
+macro_rules! macro_returns_c_str {
+    () => {
+        CStr::from_bytes_with_nul(b"foo\0").unwrap();
+    };
+}
+
+macro_rules! macro_returns_byte_string {
+    () => {
+        b"foo\0"
+    };
+}
+
+#[clippy::msrv = "1.75.0"]
+fn pre_stabilization() {
+    CStr::from_bytes_with_nul(b"foo\0");
+}
+
+#[clippy::msrv = "1.76.0"]
+fn post_stabilization() {
+    c"foo";
+}
+
+fn main() {
+    c"foo";
+    c"foo";
+    c"foo";
+    c"foo\\0sdsd";
+    CStr::from_bytes_with_nul(br"foo\\0sdsd\0").unwrap();
+    CStr::from_bytes_with_nul(br"foo\x00").unwrap();
+    CStr::from_bytes_with_nul(br##"foo#a\0"##).unwrap();
+
+    unsafe { c"foo" };
+    unsafe { c"foo" };
+
+    // Macro cases, don't lint:
+    cstr!("foo");
+    macro_returns_c_str!();
+    CStr::from_bytes_with_nul(macro_returns_byte_string!()).unwrap();
+}
diff --git a/tests/ui/manual_c_str_literals.rs b/tests/ui/manual_c_str_literals.rs
@@ -0,0 +1,51 @@
+#![feature(c_str_literals)] // TODO: remove in the next sync
+#![warn(clippy::manual_c_str_literals)]
+#![allow(clippy::no_effect)]
+
+use std::ffi::CStr;
+
+macro_rules! cstr {
+    ($s:literal) => {
+        CStr::from_bytes_with_nul(concat!($s, "\0").as_bytes()).unwrap()
+    };
+}
+
+macro_rules! macro_returns_c_str {
+    () => {
+        CStr::from_bytes_with_nul(b"foo\0").unwrap();
+    };
+}
+
+macro_rules! macro_returns_byte_string {
+    () => {
+        b"foo\0"
+    };
+}
+
+#[clippy::msrv = "1.75.0"]
+fn pre_stabilization() {
+    CStr::from_bytes_with_nul(b"foo\0");
+}
+
+#[clippy::msrv = "1.76.0"]
+fn post_stabilization() {
+    CStr::from_bytes_with_nul(b"foo\0");
+}
+
+fn main() {
+    CStr::from_bytes_with_nul(b"foo\0");
+    CStr::from_bytes_with_nul(b"foo\x00");
+    CStr::from_bytes_with_nul(b"foo\0").unwrap();
+    CStr::from_bytes_with_nul(b"foo\\0sdsd\0").unwrap();
+    CStr::from_bytes_with_nul(br"foo\\0sdsd\0").unwrap();
+    CStr::from_bytes_with_nul(br"foo\x00").unwrap();
+    CStr::from_bytes_with_nul(br##"foo#a\0"##).unwrap();
+
+    unsafe { CStr::from_ptr(b"foo\0".as_ptr().cast()) };
+    unsafe { CStr::from_ptr(b"foo\0".as_ptr() as *const _) };
+
+    // Macro cases, don't lint:
+    cstr!("foo");
+    macro_returns_c_str!();
+    CStr::from_bytes_with_nul(macro_returns_byte_string!()).unwrap();
+}
diff --git a/tests/ui/manual_c_str_literals.stderr b/tests/ui/manual_c_str_literals.stderr
@@ -0,0 +1,47 @@
+error: calling `CStr::new` with a byte string literal
+  --> $DIR/manual_c_str_literals.rs:32:5
+   |
+LL |     CStr::from_bytes_with_nul(b"foo\0");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a `c""` literal: `c"foo"`
+   |
+   = note: `-D clippy::manual-c-str-literals` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::manual_c_str_literals)]`
+
+error: calling `CStr::new` with a byte string literal
+  --> $DIR/manual_c_str_literals.rs:36:5
+   |
+LL |     CStr::from_bytes_with_nul(b"foo\0");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a `c""` literal: `c"foo"`
+
+error: calling `CStr::new` with a byte string literal
+  --> $DIR/manual_c_str_literals.rs:37:5
+   |
+LL |     CStr::from_bytes_with_nul(b"foo\x00");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a `c""` literal: `c"foo"`
+
+error: calling `CStr::new` with a byte string literal
+  --> $DIR/manual_c_str_literals.rs:38:5
+   |
+LL |     CStr::from_bytes_with_nul(b"foo\0").unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a `c""` literal: `c"foo"`
+
+error: calling `CStr::new` with a byte string literal
+  --> $DIR/manual_c_str_literals.rs:39:5
+   |
+LL |     CStr::from_bytes_with_nul(b"foo\\0sdsd\0").unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a `c""` literal: `c"foo\\0sdsd"`
+
+error: calling `CStr::from_ptr` with a byte string literal
+  --> $DIR/manual_c_str_literals.rs:44:14
+   |
+LL |     unsafe { CStr::from_ptr(b"foo\0".as_ptr().cast()) };
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a `c""` literal: `c"foo"`
+
+error: calling `CStr::from_ptr` with a byte string literal
+  --> $DIR/manual_c_str_literals.rs:45:14
+   |
+LL |     unsafe { CStr::from_ptr(b"foo\0".as_ptr() as *const _) };
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use a `c""` literal: `c"foo"`
+
+error: aborting due to 7 previous errors
+
diff --git a/tests/ui/strlen_on_c_strings.fixed b/tests/ui/strlen_on_c_strings.fixed
@@ -1,5 +1,5 @@
 #![warn(clippy::strlen_on_c_strings)]
-#![allow(dead_code)]
+#![allow(dead_code, clippy::manual_c_str_literals)]
 #![feature(rustc_private)]
 extern crate libc;
 
diff --git a/tests/ui/strlen_on_c_strings.rs b/tests/ui/strlen_on_c_strings.rs
@@ -1,5 +1,5 @@
 #![warn(clippy::strlen_on_c_strings)]
-#![allow(dead_code)]
+#![allow(dead_code, clippy::manual_c_str_literals)]
 #![feature(rustc_private)]
 extern crate libc;
 
