Skip to content

Commit f09cd88

Browse files
y21atwam
y21
authored and
atwam
committed
fix false positive in suspicious_open_options, make paths work
1 parent 515fe65 commit f09cd88

File tree

6 files changed

+87
-27
lines changed

6 files changed

+87
-27
lines changed

‎clippy_lints/src/methods/mod.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2833,10 +2833,11 @@ declare_clippy_lint! {
28332833
/// without an explicit `OpenOptions::truncate()`.
28342834
///
28352835
/// ### Why is this bad?
2836-
/// create() alone will either create a new file or open an
2836+
/// `create()` alone will either create a new file or open an
28372837
/// existing file. If the file already exists, it will be
28382838
/// overwritten when written to, but the file will not be
2839-
/// truncated by default. If less data is written to the file
2839+
/// truncated by default.
2840+
/// If less data is written to the file
28402841
/// than it already contains, the remainder of the file will
28412842
/// remain unchanged, and the end of the file will contain old
28422843
/// data.
@@ -2847,10 +2848,14 @@ declare_clippy_lint! {
28472848
/// `truncate(false)` will explicitely keep the default behavior.
28482849
///
28492850
/// ### Example
2850-
/// ```rust
2851+
/// ```rust,no_run
28512852
/// use std::fs::OpenOptions;
28522853
///
28532854
/// OpenOptions::new().create(true);
2855+
/// ```
2856+
/// Use instead:
2857+
/// ```rust,no_run
2858+
/// use std::fs::OpenOptions;
28542859
///
28552860
/// OpenOptions::new().create(true).truncate(true);
28562861
/// ```

‎clippy_lints/src/methods/open_options.rs

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,12 @@ fn is_open_options(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
1919
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, recv: &'tcx Expr<'_>) {
2020
if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
2121
&& let Some(impl_id) = cx.tcx.impl_of_method(method_id)
22-
//&& is_open_options(cx, cx.tcx.type_of(impl_id).instantiate_identity())
22+
&& is_open_options(cx, cx.tcx.type_of(impl_id).instantiate_identity())
2323
{
2424
let mut options = Vec::new();
25-
get_open_options(cx, recv, &mut options);
26-
check_open_options(cx, &options, e.span);
25+
if get_open_options(cx, recv, &mut options) {
26+
check_open_options(cx, &options, e.span);
27+
}
2728
}
2829
}
2930

@@ -55,6 +56,9 @@ impl std::fmt::Display for OpenOption {
5556
}
5657
}
5758

59+
/// Collects information about a method call chain on `OpenOptions`.
60+
/// Returns false if an unexpected expression kind was found "on the way",
61+
/// and linting should then be avoided.
5862
fn get_open_options(
5963
cx: &LateContext<'_>,
6064
argument: &Expr<'_>,
@@ -102,7 +106,16 @@ fn get_open_options(
102106
"write" => {
103107
options.push((OpenOption::Write, argument_option, span));
104108
},
105-
_ => (),
109+
_ => {
110+
// Avoid linting altogether if this method is from a trait.
111+
// This might be a user defined extension trait with a method like `truncate_write`
112+
// which would be a false positive
113+
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(argument.hir_id)
114+
&& cx.tcx.trait_of_item(method_def_id).is_some()
115+
{
116+
return false;
117+
}
118+
},
106119
}
107120

108121
get_open_options(cx, receiver, options)
@@ -113,9 +126,17 @@ fn get_open_options(
113126
&& let ExprKind::Path(path) = callee.kind
114127
&& let Some(did) = cx.qpath_res(&path, callee.hir_id).opt_def_id()
115128
{
116-
match_any_def_paths(cx, did, &[&paths::TOKIO_IO_OPEN_OPTIONS]).is_some()
117-
//is_type_diagnostic_item(cx, ty, sym::FsOpenOptions) || match_type(cx, ty,
118-
// &paths::TOKIO_IO_OPEN_OPTIONS)
129+
match_any_def_paths(
130+
cx,
131+
did,
132+
&[
133+
&paths::TOKIO_IO_OPEN_OPTIONS_NEW,
134+
&paths::OPEN_OPTIONS_NEW,
135+
&paths::FILE_OPTIONS,
136+
&paths::TOKIO_FILE_OPTIONS,
137+
],
138+
)
139+
.is_some()
119140
} else {
120141
false
121142
}
@@ -168,9 +189,17 @@ fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument, S
168189
*create_span,
169190
"file opened with `create`, but `truncate` behavior not defined",
170191
|diag| {
171-
diag
172-
.span_suggestion(create_span.shrink_to_hi(), "add", ".truncate(true)".to_string(), rustc_errors::Applicability::MaybeIncorrect)
173-
.help("if you intend to overwrite an existing file entirely, call `.truncate(true)`. if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`. Alternatively, use `.append` to append to the file instead of overwriting it.");
192+
diag.span_suggestion(
193+
create_span.shrink_to_hi(),
194+
"add",
195+
".truncate(true)".to_string(),
196+
rustc_errors::Applicability::MaybeIncorrect,
197+
)
198+
.help("if you intend to overwrite an existing file entirely, call `.truncate(true)`")
199+
.help(
200+
"if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`",
201+
)
202+
.help("alternatively, use `.append(true)` to append to the file instead of overwriting it");
174203
},
175204
);
176205
}

‎clippy_utils/src/paths.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ pub const EARLY_CONTEXT: [&str; 2] = ["rustc_lint", "EarlyContext"];
2626
pub const EARLY_LINT_PASS: [&str; 3] = ["rustc_lint", "passes", "EarlyLintPass"];
2727
pub const F32_EPSILON: [&str; 4] = ["core", "f32", "<impl f32>", "EPSILON"];
2828
pub const F64_EPSILON: [&str; 4] = ["core", "f64", "<impl f64>", "EPSILON"];
29+
pub const FILE_OPTIONS: [&str; 4] = ["std", "fs", "File", "options"];
2930
#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates
3031
pub const FUTURES_IO_ASYNCREADEXT: [&str; 3] = ["futures_util", "io", "AsyncReadExt"];
3132
#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates
@@ -50,8 +51,7 @@ pub const LATE_CONTEXT: [&str; 2] = ["rustc_lint", "LateContext"];
5051
pub const LATE_LINT_PASS: [&str; 3] = ["rustc_lint", "passes", "LateLintPass"];
5152
pub const LINT: [&str; 2] = ["rustc_lint_defs", "Lint"];
5253
pub const MSRV: [&str; 3] = ["clippy_config", "msrvs", "Msrv"];
53-
#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates
54-
pub const TOKIO_IO_OPEN_OPTIONS: [&str; 3] = ["tokio", "fs", "OpenOptions"];
54+
pub const OPEN_OPTIONS_NEW: [&str; 4] = ["std", "fs", "OpenOptions", "new"];
5555
pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString", "as_os_str"];
5656
pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"];
5757
pub const PARKING_LOT_MUTEX_GUARD: [&str; 3] = ["lock_api", "mutex", "MutexGuard"];
@@ -91,9 +91,15 @@ pub const SYMBOL_TO_IDENT_STRING: [&str; 4] = ["rustc_span", "symbol", "Symbol",
9191
pub const SYM_MODULE: [&str; 3] = ["rustc_span", "symbol", "sym"];
9292
pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"];
9393
#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates
94+
pub const TOKIO_FILE_OPTIONS: [&str; 5] = ["tokio", "fs", "file", "File", "options"];
95+
#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates
9496
pub const TOKIO_IO_ASYNCREADEXT: [&str; 5] = ["tokio", "io", "util", "async_read_ext", "AsyncReadExt"];
9597
#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates
9698
pub const TOKIO_IO_ASYNCWRITEEXT: [&str; 5] = ["tokio", "io", "util", "async_write_ext", "AsyncWriteExt"];
99+
#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates
100+
pub const TOKIO_IO_OPEN_OPTIONS: [&str; 4] = ["tokio", "fs", "open_options", "OpenOptions"];
101+
#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates
102+
pub const TOKIO_IO_OPEN_OPTIONS_NEW: [&str; 5] = ["tokio", "fs", "open_options", "OpenOptions", "new"];
97103
pub const VEC_AS_MUT_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_mut_slice"];
98104
pub const VEC_AS_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_slice"];
99105
pub const VEC_DEQUE_ITER: [&str; 5] = ["alloc", "collections", "vec_deque", "VecDeque", "iter"];

‎tests/ui/open_options.rs

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,18 @@
1+
#![allow(unused_must_use)]
2+
#![warn(clippy::nonsensical_open_options)]
3+
14
use std::fs::OpenOptions;
2-
#[allow(unused_must_use)]
3-
#[warn(clippy::nonsensical_open_options)]
5+
6+
trait OpenOptionsExt {
7+
fn truncate_write(&mut self, opt: bool) -> &mut Self;
8+
}
9+
10+
impl OpenOptionsExt for OpenOptions {
11+
fn truncate_write(&mut self, opt: bool) -> &mut Self {
12+
self.truncate(opt).write(opt)
13+
}
14+
}
15+
416
fn main() {
517
OpenOptions::new().read(true).truncate(true).open("foo.txt");
618
//~^ ERROR: file opened with `truncate` and `read`
@@ -29,6 +41,12 @@ fn main() {
2941
let mut options = std::fs::OpenOptions::new();
3042
options.read(true);
3143
options.read(false);
32-
//#~^ ERROR: the method `read` is called more than once
44+
// Possible future improvement: recognize open options method call chains across statements.
3345
options.open("foo.txt");
46+
47+
let mut options = std::fs::OpenOptions::new();
48+
options.truncate(true);
49+
options.create(true).open("foo.txt");
50+
51+
OpenOptions::new().create(true).truncate_write(true).open("foo.txt");
3452
}

‎tests/ui/open_options.stderr

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: file opened with `truncate` and `read`
2-
--> $DIR/open_options.rs:5:5
2+
--> $DIR/open_options.rs:17:5
33
|
44
LL | OpenOptions::new().read(true).truncate(true).open("foo.txt");
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -8,43 +8,43 @@ LL | OpenOptions::new().read(true).truncate(true).open("foo.txt");
88
= help: to override `-D warnings` add `#[allow(clippy::nonsensical_open_options)]`
99

1010
error: file opened with `append` and `truncate`
11-
--> $DIR/open_options.rs:8:5
11+
--> $DIR/open_options.rs:20:5
1212
|
1313
LL | OpenOptions::new().append(true).truncate(true).open("foo.txt");
1414
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1515

1616
error: the method `read` is called more than once
17-
--> $DIR/open_options.rs:11:35
17+
--> $DIR/open_options.rs:23:35
1818
|
1919
LL | OpenOptions::new().read(true).read(false).open("foo.txt");
2020
| ^^^^^^^^^^^
2121

2222
error: the method `create` is called more than once
23-
--> $DIR/open_options.rs:16:10
23+
--> $DIR/open_options.rs:28:10
2424
|
2525
LL | .create(false)
2626
| ^^^^^^^^^^^^^
2727

2828
error: the method `write` is called more than once
29-
--> $DIR/open_options.rs:19:36
29+
--> $DIR/open_options.rs:31:36
3030
|
3131
LL | OpenOptions::new().write(true).write(false).open("foo.txt");
3232
| ^^^^^^^^^^^^
3333

3434
error: the method `append` is called more than once
35-
--> $DIR/open_options.rs:21:37
35+
--> $DIR/open_options.rs:33:37
3636
|
3737
LL | OpenOptions::new().append(true).append(false).open("foo.txt");
3838
| ^^^^^^^^^^^^^
3939

4040
error: the method `truncate` is called more than once
41-
--> $DIR/open_options.rs:23:39
41+
--> $DIR/open_options.rs:35:39
4242
|
4343
LL | OpenOptions::new().truncate(true).truncate(false).open("foo.txt");
4444
| ^^^^^^^^^^^^^^^
4545

4646
error: the method `read` is called more than once
47-
--> $DIR/open_options.rs:26:41
47+
--> $DIR/open_options.rs:38:41
4848
|
4949
LL | std::fs::File::options().read(true).read(false).open("foo.txt");
5050
| ^^^^^^^^^^^

‎tests/ui/open_options_fixable.stderr

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ error: file opened with `create`, but `truncate` behavior not defined
44
LL | OpenOptions::new().create(true).open("foo.txt");
55
| ^^^^^^^^^^^^- help: add: `.truncate(true)`
66
|
7-
= help: if you intend to overwrite an existing file entirely, call `.truncate(true)`. if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`. Alternatively, use `.append` to append to the file instead of overwriting it.
7+
= help: if you intend to overwrite an existing file entirely, call `.truncate(true)`
8+
= help: if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`
9+
= help: alternatively, use `.append(true)` to append to the file instead of overwriting it
810
= note: `-D clippy::suspicious-open-options` implied by `-D warnings`
911
= help: to override `-D warnings` add `#[allow(clippy::suspicious_open_options)]`
1012

0 commit comments

Comments
 (0)