Address review comments

1 files changed, 35 insertions, 49 deletions

diff --git a/compiler/rustc_lint/src/methods.rs b/compiler/rustc_lint/src/methods.rs
index 910e07154a7..7b595dd18ff 100644
--- a/compiler/rustc_lint/src/methods.rs
+++ b/compiler/rustc_lint/src/methods.rs
@@ -1,10 +1,10 @@
 use crate::LateContext;
 use crate::LateLintPass;
 use crate::LintContext;
-use rustc_hir::{Expr, ExprKind};
+use rustc_hir::{Expr, ExprKind, PathSegment};
 use rustc_middle::ty;
 use rustc_span::{
-    symbol::{sym, Symbol, SymbolStr},
+    symbol::{sym, Symbol},
     ExpnKind, Span,
 };
 
@@ -16,34 +16,6 @@ declare_lint! {
 
 declare_lint_pass!(TemporaryCStringAsPtr => [TEMPORARY_CSTRING_AS_PTR]);
 
-/// Returns the method names and argument list of nested method call expressions that make up
-/// `expr`. method/span lists are sorted with the most recent call first.
-pub fn method_calls<'tcx>(
-    expr: &'tcx Expr<'tcx>,
-    max_depth: usize,
-) -> (Vec<Symbol>, Vec<&'tcx [Expr<'tcx>]>, Vec<Span>) {
-    let mut method_names = Vec::with_capacity(max_depth);
-    let mut arg_lists = Vec::with_capacity(max_depth);
-    let mut spans = Vec::with_capacity(max_depth);
-
-    let mut current = expr;
-    for _ in 0..max_depth {
-        if let ExprKind::MethodCall(path, span, args, _) = &current.kind {
-            if args.iter().any(|e| e.span.from_expansion()) {
-                break;
-            }
-            method_names.push(path.ident.name);
-            arg_lists.push(&**args);
-            spans.push(*span);
-            current = &args[0];
-        } else {
-            break;
-        }
-    }
-
-    (method_names, arg_lists, spans)
-}
-
 fn in_macro(span: Span) -> bool {
     if span.from_expansion() {
         !matches!(span.ctxt().outer_expn_data().kind, ExpnKind::Desugaring(..))
@@ -52,47 +24,61 @@ fn in_macro(span: Span) -> bool {
     }
 }
 
+fn first_method_call<'tcx>(
+    expr: &'tcx Expr<'tcx>,
+) -> Option<(&'tcx PathSegment<'tcx>, &'tcx [Expr<'tcx>])> {
+    if let ExprKind::MethodCall(path, _, args, _) = &expr.kind {
+        if args.iter().any(|e| e.span.from_expansion()) { None } else { Some((path, *args)) }
+    } else {
+        None
+    }
+}
+
 impl<'tcx> LateLintPass<'tcx> for TemporaryCStringAsPtr {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if in_macro(expr.span) {
             return;
         }
 
-        let (method_names, arg_lists, _) = method_calls(expr, 2);
-        let method_names: Vec<SymbolStr> = method_names.iter().map(|s| s.as_str()).collect();
-        let method_names: Vec<&str> = method_names.iter().map(|s| &**s).collect();
-
-        if let ["as_ptr", "unwrap" | "expect"] = method_names.as_slice() {
-            lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]);
+        match first_method_call(expr) {
+            Some((path, args)) if path.ident.name == sym::as_ptr => {
+                let unwrap_arg = &args[0];
+                match first_method_call(unwrap_arg) {
+                    Some((path, args))
+                        if path.ident.name == sym::unwrap || path.ident.name == sym::expect =>
+                    {
+                        let source_arg = &args[0];
+                        lint_cstring_as_ptr(cx, source_arg, unwrap_arg);
+                    }
+                    _ => return,
+                }
+            }
+            _ => return,
         }
     }
 }
 
+const CSTRING_PATH: [Symbol; 4] = [sym::std, sym::ffi, sym::c_str, sym::CString];
+
 fn lint_cstring_as_ptr(
     cx: &LateContext<'_>,
-    expr: &rustc_hir::Expr<'_>,
     source: &rustc_hir::Expr<'_>,
     unwrap: &rustc_hir::Expr<'_>,
 ) {
     let source_type = cx.typeck_results().expr_ty(source);
     if let ty::Adt(def, substs) = source_type.kind {
-        if cx.tcx.is_diagnostic_item(Symbol::intern("result_type"), def.did) {
+        if cx.tcx.is_diagnostic_item(sym::result_type, def.did) {
             if let ty::Adt(adt, _) = substs.type_at(0).kind {
-                let path = [
-                    sym::std,
-                    Symbol::intern("ffi"),
-                    Symbol::intern("c_str"),
-                    Symbol::intern("CString"),
-                ];
-                if cx.match_def_path(adt.did, &path) {
-                    cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, expr.span, |diag| {
+                if cx.match_def_path(adt.did, &CSTRING_PATH) {
+                    cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, source.span, |diag| {
                         let mut diag = diag
-                            .build("you are getting the inner pointer of a temporary `CString`");
-                        diag.note("that pointer will be invalid outside this expression");
+                            .build("getting the inner pointer of a temporary `CString`");
+                        diag.span_label(source.span, "this pointer will be invalid");
                         diag.span_help(
                             unwrap.span,
-                            "assign the `CString` to a variable to extend its lifetime",
+                            "this `CString` is deallocated at the end of the expression, bind it to a variable to extend its lifetime",
                         );
+                        diag.note("pointers do not have a lifetime; when calling `as_ptr` the `CString` is deallocated because nothing is referencing it as far as the type system is concerned");
                         diag.emit();
                     });
                 }