Skip to content
This repository was archived by the owner on May 28, 2025. It is now read-only.

Commit 8b65df0

Browse files
nathanwhitnathanwhit
committed
Address review comments
1 parent a2f4afe commit 8b65df0

File tree

4 files changed

+48
-57
lines changed

4 files changed

+48
-57
lines changed

‎compiler/rustc_lint/src/methods.rs

Lines changed: 35 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
use crate::LateContext;
22
use crate::LateLintPass;
33
use crate::LintContext;
4-
use rustc_hir::{Expr, ExprKind};
4+
use rustc_hir::{Expr, ExprKind, PathSegment};
55
use rustc_middle::ty;
66
use rustc_span::{
7-
symbol::{sym, Symbol, SymbolStr},
7+
symbol::{sym, Symbol},
88
ExpnKind, Span,
99
};
1010

@@ -16,34 +16,6 @@ declare_lint! {
1616

1717
declare_lint_pass!(TemporaryCStringAsPtr => [TEMPORARY_CSTRING_AS_PTR]);
1818

19-
/// Returns the method names and argument list of nested method call expressions that make up
20-
/// `expr`. method/span lists are sorted with the most recent call first.
21-
pub fn method_calls<'tcx>(
22-
expr: &'tcx Expr<'tcx>,
23-
max_depth: usize,
24-
) -> (Vec<Symbol>, Vec<&'tcx [Expr<'tcx>]>, Vec<Span>) {
25-
let mut method_names = Vec::with_capacity(max_depth);
26-
let mut arg_lists = Vec::with_capacity(max_depth);
27-
let mut spans = Vec::with_capacity(max_depth);
28-
29-
let mut current = expr;
30-
for _ in 0..max_depth {
31-
if let ExprKind::MethodCall(path, span, args, _) = &current.kind {
32-
if args.iter().any(|e| e.span.from_expansion()) {
33-
break;
34-
}
35-
method_names.push(path.ident.name);
36-
arg_lists.push(&**args);
37-
spans.push(*span);
38-
current = &args[0];
39-
} else {
40-
break;
41-
}
42-
}
43-
44-
(method_names, arg_lists, spans)
45-
}
46-
4719
fn in_macro(span: Span) -> bool {
4820
if span.from_expansion() {
4921
!matches!(span.ctxt().outer_expn_data().kind, ExpnKind::Desugaring(..))
@@ -52,47 +24,61 @@ fn in_macro(span: Span) -> bool {
5224
}
5325
}
5426

27+
fn first_method_call<'tcx>(
28+
expr: &'tcx Expr<'tcx>,
29+
) -> Option<(&'tcx PathSegment<'tcx>, &'tcx [Expr<'tcx>])> {
30+
if let ExprKind::MethodCall(path, _, args, _) = &expr.kind {
31+
if args.iter().any(|e| e.span.from_expansion()) { None } else { Some((path, *args)) }
32+
} else {
33+
None
34+
}
35+
}
36+
5537
impl<'tcx> LateLintPass<'tcx> for TemporaryCStringAsPtr {
5638
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
5739
if in_macro(expr.span) {
5840
return;
5941
}
6042

61-
let (method_names, arg_lists, _) = method_calls(expr, 2);
62-
let method_names: Vec<SymbolStr> = method_names.iter().map(|s| s.as_str()).collect();
63-
let method_names: Vec<&str> = method_names.iter().map(|s| &**s).collect();
64-
65-
if let ["as_ptr", "unwrap" | "expect"] = method_names.as_slice() {
66-
lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]);
43+
match first_method_call(expr) {
44+
Some((path, args)) if path.ident.name == sym::as_ptr => {
45+
let unwrap_arg = &args[0];
46+
match first_method_call(unwrap_arg) {
47+
Some((path, args))
48+
if path.ident.name == sym::unwrap || path.ident.name == sym::expect =>
49+
{
50+
let source_arg = &args[0];
51+
lint_cstring_as_ptr(cx, source_arg, unwrap_arg);
52+
}
53+
_ => return,
54+
}
55+
}
56+
_ => return,
6757
}
6858
}
6959
}
7060

61+
const CSTRING_PATH: [Symbol; 4] = [sym::std, sym::ffi, sym::c_str, sym::CString];
62+
7163
fn lint_cstring_as_ptr(
7264
cx: &LateContext<'_>,
73-
expr: &rustc_hir::Expr<'_>,
7465
source: &rustc_hir::Expr<'_>,
7566
unwrap: &rustc_hir::Expr<'_>,
7667
) {
7768
let source_type = cx.typeck_results().expr_ty(source);
7869
if let ty::Adt(def, substs) = source_type.kind {
79-
if cx.tcx.is_diagnostic_item(Symbol::intern("result_type"), def.did) {
70+
if cx.tcx.is_diagnostic_item(sym::result_type, def.did) {
8071
if let ty::Adt(adt, _) = substs.type_at(0).kind {
81-
let path = [
82-
sym::std,
83-
Symbol::intern("ffi"),
84-
Symbol::intern("c_str"),
85-
Symbol::intern("CString"),
86-
];
87-
if cx.match_def_path(adt.did, &path) {
88-
cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, expr.span, |diag| {
72+
if cx.match_def_path(adt.did, &CSTRING_PATH) {
73+
cx.struct_span_lint(TEMPORARY_CSTRING_AS_PTR, source.span, |diag| {
8974
let mut diag = diag
90-
.build("you are getting the inner pointer of a temporary `CString`");
91-
diag.note("that pointer will be invalid outside this expression");
75+
.build("getting the inner pointer of a temporary `CString`");
76+
diag.span_label(source.span, "this pointer will be invalid");
9277
diag.span_help(
9378
unwrap.span,
94-
"assign the `CString` to a variable to extend its lifetime",
79+
"this `CString` is deallocated at the end of the expression, bind it to a variable to extend its lifetime",
9580
);
81+
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");
9682
diag.emit();
9783
});
9884
}

‎compiler/rustc_span/src/symbol.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ symbols! {
127127
ArgumentV1,
128128
Arguments,
129129
C,
130+
CString,
130131
Center,
131132
Clone,
132133
Copy,
@@ -261,6 +262,7 @@ symbols! {
261262
arm_target_feature,
262263
array,
263264
arrays,
265+
as_ptr,
264266
as_str,
265267
asm,
266268
assert,
@@ -310,6 +312,7 @@ symbols! {
310312
breakpoint,
311313
bridge,
312314
bswap,
315+
c_str,
313316
c_variadic,
314317
call,
315318
call_mut,
@@ -477,6 +480,7 @@ symbols! {
477480
existential_type,
478481
exp2f32,
479482
exp2f64,
483+
expect,
480484
expected,
481485
expf32,
482486
expf64,
@@ -500,6 +504,7 @@ symbols! {
500504
fadd_fast,
501505
fdiv_fast,
502506
feature,
507+
ffi,
503508
ffi_const,
504509
ffi_pure,
505510
ffi_returns_twice,
@@ -1167,6 +1172,7 @@ symbols! {
11671172
unused_qualifications,
11681173
unwind,
11691174
unwind_attributes,
1175+
unwrap,
11701176
unwrap_or,
11711177
use_extern_macros,
11721178
use_nested_groups,

‎src/test/ui/lint/lint-temporary-cstring-as-ptr.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
1-
// check-fail
21
// ignore-tidy-linelength
32

43
use std::ffi::CString;
54

65
fn main() {
7-
let s = CString::new("some text").unwrap().as_ptr(); //~ ERROR you are getting the inner pointer of a temporary `CString`
6+
let s = CString::new("some text").unwrap().as_ptr(); //~ ERROR getting the inner pointer of a temporary `CString`
87
}

‎src/test/ui/lint/lint-temporary-cstring-as-ptr.stderr

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
1-
error: you are getting the inner pointer of a temporary `CString`
2-
--> $DIR/lint-temporary-cstring-as-ptr.rs:7:13
1+
error: getting the inner pointer of a temporary `CString`
2+
--> $DIR/lint-temporary-cstring-as-ptr.rs:6:13
33
|
44
LL | let s = CString::new("some text").unwrap().as_ptr();
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^ this pointer will be invalid
66
|
77
= note: `#[deny(temporary_cstring_as_ptr)]` on by default
8-
= note: that pointer will be invalid outside this expression
9-
help: assign the `CString` to a variable to extend its lifetime
10-
--> $DIR/lint-temporary-cstring-as-ptr.rs:7:13
8+
help: this `CString` is deallocated at the end of the expression, bind it to a variable to extend its lifetime
9+
--> $DIR/lint-temporary-cstring-as-ptr.rs:6:13
1110
|
1211
LL | let s = CString::new("some text").unwrap().as_ptr();
1312
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
13+
= 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
1414

1515
error: aborting due to previous error
1616

0 commit comments

Comments
 (0)