Skip to content

Commit 6c201db

Browse files
atwamatwam
committed
Add suspicious_open_options lint.
Checks for the suspicious use of OpenOptions::create() without an explicit OpenOptions::truncate(). create() alone will either create a new file or open an existing file. If the file already exists, it will be overwritten when written to, but the file will not be truncated by default. If less data is written to the file than it already contains, the remainder of the file will remain unchanged, and the end of the file will contain old data. In most cases, one should either use `create_new` to ensure the file is created from scratch, or ensure `truncate` is called so that the truncation behaviour is explicit. `truncate(true)` will ensure the file is entirely overwritten with new data, whereas `truncate(false)` will explicitely keep the default behavior. ```rust use std::fs::OpenOptions; OpenOptions::new().create(true).truncate(true); ```
1 parent 692f53f commit 6c201db

9 files changed

+131
-101
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5606,6 +5606,7 @@ Released 2018-09-13
56065606
[`suspicious_else_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_else_formatting
56075607
[`suspicious_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_map
56085608
[`suspicious_op_assign_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_op_assign_impl
5609+
[`suspicious_open_options`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_open_options
56095610
[`suspicious_operation_groupings`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_operation_groupings
56105611
[`suspicious_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_splitn
56115612
[`suspicious_to_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_to_owned

‎clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
439439
crate::methods::STR_SPLIT_AT_NEWLINE_INFO,
440440
crate::methods::SUSPICIOUS_COMMAND_ARG_SPACE_INFO,
441441
crate::methods::SUSPICIOUS_MAP_INFO,
442+
crate::methods::SUSPICIOUS_OPEN_OPTIONS_INFO,
442443
crate::methods::SUSPICIOUS_SPLITN_INFO,
443444
crate::methods::SUSPICIOUS_TO_OWNED_INFO,
444445
crate::methods::TYPE_ID_ON_BOX_INFO,

‎clippy_lints/src/methods/mod.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2827,6 +2827,37 @@ declare_clippy_lint! {
28272827
"nonsensical combination of options for opening a file"
28282828
}
28292829

2830+
declare_clippy_lint! {
2831+
/// ### What it does
2832+
/// Checks for the suspicious use of OpenOptions::create()
2833+
/// without an explicit OpenOptions::truncate().
2834+
///
2835+
/// ### Why is this bad?
2836+
/// create() alone will either create a new file or open an
2837+
/// existing file. If the file already exists, it will be
2838+
/// overwritten when written to, but the file will not be
2839+
/// truncated by default. If less data is written to the file
2840+
/// than it already contains, the remainder of the file will
2841+
/// remain unchanged, and the end of the file will contain old
2842+
/// data.
2843+
/// In most cases, one should either use `create_new` to ensure
2844+
/// the file is created from scratch, or ensure `truncate` is
2845+
/// called so that the truncation behaviour is explicit. `truncate(true)`
2846+
/// will ensure the file is entirely overwritten with new data, whereas
2847+
/// `truncate(false)` will explicitely keep the default behavior.
2848+
///
2849+
/// ### Example
2850+
/// ```rust
2851+
/// use std::fs::OpenOptions;
2852+
///
2853+
/// OpenOptions::new().create(true).truncate(true);
2854+
/// ```
2855+
#[clippy::version = "1.73.0"]
2856+
pub SUSPICIOUS_OPEN_OPTIONS,
2857+
suspicious,
2858+
"suspicious combination of options for opening a file"
2859+
}
2860+
28302861
declare_clippy_lint! {
28312862
/// ### What it does
28322863
///* Checks for [push](https://doc.rust-lang.org/std/path/struct.PathBuf.html#method.push)
@@ -4033,6 +4064,7 @@ impl_lint_pass!(Methods => [
40334064
MAP_ERR_IGNORE,
40344065
MUT_MUTEX_LOCK,
40354066
NONSENSICAL_OPEN_OPTIONS,
4067+
SUSPICIOUS_OPEN_OPTIONS,
40364068
PATH_BUF_PUSH_OVERWRITE,
40374069
RANGE_ZIP_WITH_LEN,
40384070
REPEAT_ONCE,

‎clippy_lints/src/methods/open_options.rs

Lines changed: 64 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
use rustc_data_structures::fx::FxHashMap;
2+
13
use clippy_utils::diagnostics::span_lint;
24
use clippy_utils::ty::is_type_diagnostic_item;
35
use rustc_ast::ast::LitKind;
@@ -6,7 +8,7 @@ use rustc_lint::LateContext;
68
use rustc_span::source_map::Spanned;
79
use rustc_span::{sym, Span};
810

9-
use super::NONSENSICAL_OPEN_OPTIONS;
11+
use super::{NONSENSICAL_OPEN_OPTIONS, SUSPICIOUS_OPEN_OPTIONS};
1012

1113
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, recv: &'tcx Expr<'_>) {
1214
if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
@@ -19,20 +21,32 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, recv: &'tcx
1921
}
2022
}
2123

22-
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
24+
#[derive(Eq, PartialEq, Clone)]
2325
enum Argument {
24-
True,
25-
False,
26+
Set(bool),
2627
Unknown,
2728
}
2829

29-
#[derive(Debug)]
30+
#[derive(Debug, Eq, PartialEq, Hash, Clone)]
3031
enum OpenOption {
31-
Write,
32+
Append,
33+
Create,
34+
CreateNew,
3235
Read,
3336
Truncate,
34-
Create,
35-
Append,
37+
Write,
38+
}
39+
impl std::fmt::Display for OpenOption {
40+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
41+
match self {
42+
OpenOption::Append => write!(f, "append"),
43+
OpenOption::Create => write!(f, "create"),
44+
OpenOption::CreateNew => write!(f, "create_new"),
45+
OpenOption::Read => write!(f, "read"),
46+
OpenOption::Truncate => write!(f, "truncate"),
47+
OpenOption::Write => write!(f, "write"),
48+
}
49+
}
3650
}
3751

3852
fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec<(OpenOption, Argument)>) {
@@ -48,7 +62,7 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
4862
..
4963
} = span
5064
{
51-
if *lit { Argument::True } else { Argument::False }
65+
Argument::Set(*lit)
5266
} else {
5367
// The function is called with a literal which is not a boolean literal.
5468
// This is theoretically possible, but not very likely.
@@ -62,6 +76,9 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
6276
"create" => {
6377
options.push((OpenOption::Create, argument_option));
6478
},
79+
"create_new" => {
80+
options.push((OpenOption::CreateNew, argument_option));
81+
},
6582
"append" => {
6683
options.push((OpenOption::Append, argument_option));
6784
},
@@ -82,97 +99,52 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
8299
}
83100
}
84101

85-
fn check_open_options(cx: &LateContext<'_>, options: &[(OpenOption, Argument)], span: Span) {
86-
let (mut create, mut append, mut truncate, mut read, mut write) = (false, false, false, false, false);
87-
let (mut create_arg, mut append_arg, mut truncate_arg, mut read_arg, mut write_arg) =
88-
(false, false, false, false, false);
89-
// This code is almost duplicated (oh, the irony), but I haven't found a way to
90-
// unify it.
102+
fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument)], span: Span) {
103+
// The args passed to these methods, if they have been called
104+
let mut options = FxHashMap::default();
105+
for (option, arg) in settings {
106+
if options.insert(option.clone(), arg.clone()).is_some() {
107+
span_lint(
108+
cx,
109+
NONSENSICAL_OPEN_OPTIONS,
110+
span,
111+
&format!("the method `{}` is called more than once", &option),
112+
);
113+
}
114+
}
91115

92-
for option in options {
93-
match *option {
94-
(OpenOption::Create, arg) => {
95-
if create {
96-
span_lint(
97-
cx,
98-
NONSENSICAL_OPEN_OPTIONS,
99-
span,
100-
"the method `create` is called more than once",
101-
);
102-
} else {
103-
create = true;
104-
}
105-
create_arg = create_arg || (arg == Argument::True);
106-
},
107-
(OpenOption::Append, arg) => {
108-
if append {
109-
span_lint(
110-
cx,
111-
NONSENSICAL_OPEN_OPTIONS,
112-
span,
113-
"the method `append` is called more than once",
114-
);
115-
} else {
116-
append = true;
117-
}
118-
append_arg = append_arg || (arg == Argument::True);
119-
},
120-
(OpenOption::Truncate, arg) => {
121-
if truncate {
122-
span_lint(
123-
cx,
124-
NONSENSICAL_OPEN_OPTIONS,
125-
span,
126-
"the method `truncate` is called more than once",
127-
);
128-
} else {
129-
truncate = true;
130-
}
131-
truncate_arg = truncate_arg || (arg == Argument::True);
132-
},
133-
(OpenOption::Read, arg) => {
134-
if read {
135-
span_lint(
136-
cx,
137-
NONSENSICAL_OPEN_OPTIONS,
138-
span,
139-
"the method `read` is called more than once",
140-
);
141-
} else {
142-
read = true;
143-
}
144-
read_arg = read_arg || (arg == Argument::True);
145-
},
146-
(OpenOption::Write, arg) => {
147-
if write {
148-
span_lint(
149-
cx,
150-
NONSENSICAL_OPEN_OPTIONS,
151-
span,
152-
"the method `write` is called more than once",
153-
);
154-
} else {
155-
write = true;
156-
}
157-
write_arg = write_arg || (arg == Argument::True);
158-
},
116+
if let (Some(Argument::Set(true)), Some(Argument::Set(true))) =
117+
(options.get(&OpenOption::Read), options.get(&OpenOption::Truncate))
118+
{
119+
if options.get(&OpenOption::Write).unwrap_or(&Argument::Set(false)) == &Argument::Set(false) {
120+
span_lint(
121+
cx,
122+
NONSENSICAL_OPEN_OPTIONS,
123+
span,
124+
"file opened with `truncate` and `read`",
125+
);
159126
}
160127
}
161128

162-
if read && truncate && read_arg && truncate_arg && !(write && write_arg) {
163-
span_lint(
164-
cx,
165-
NONSENSICAL_OPEN_OPTIONS,
166-
span,
167-
"file opened with `truncate` and `read`",
168-
);
129+
if let (Some(Argument::Set(true)), Some(Argument::Set(true))) =
130+
(options.get(&OpenOption::Append), options.get(&OpenOption::Truncate))
131+
{
132+
if options.get(&OpenOption::Write).unwrap_or(&Argument::Set(false)) == &Argument::Set(false) {
133+
span_lint(
134+
cx,
135+
NONSENSICAL_OPEN_OPTIONS,
136+
span,
137+
"file opened with `append` and `truncate`",
138+
);
139+
}
169140
}
170-
if append && truncate && append_arg && truncate_arg {
141+
142+
if let (Some(Argument::Set(true)), None) = (options.get(&OpenOption::Create), options.get(&OpenOption::Truncate)) {
171143
span_lint(
172144
cx,
173-
NONSENSICAL_OPEN_OPTIONS,
145+
SUSPICIOUS_OPEN_OPTIONS,
174146
span,
175-
"file opened with `append` and `truncate`",
147+
"file opened with `create`, but `truncate` behavior not defined",
176148
);
177149
}
178150
}

‎tests/ui/open_options.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ use std::fs::OpenOptions;
22

33
#[allow(unused_must_use)]
44
#[warn(clippy::nonsensical_open_options)]
5+
#[warn(clippy::suspicious_open_options)]
56
fn main() {
67
OpenOptions::new().read(true).truncate(true).open("foo.txt");
78
//~^ ERROR: file opened with `truncate` and `read`
@@ -19,4 +20,6 @@ fn main() {
1920
//~^ ERROR: the method `append` is called more than once
2021
OpenOptions::new().truncate(true).truncate(false).open("foo.txt");
2122
//~^ ERROR: the method `truncate` is called more than once
23+
OpenOptions::new().create(true).open("foo.txt");
24+
//~^ ERROR: file opened with `create`, but `truncate` behavior not defined
2225
}

‎tests/ui/open_options.stderr

Lines changed: 23 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:6:5
2+
--> $DIR/open_options.rs:7:5
33
|
44
LL | OpenOptions::new().read(true).truncate(true).open("foo.txt");
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -8,40 +8,55 @@ 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:9:5
11+
--> $DIR/open_options.rs:10: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:12:5
17+
--> $DIR/open_options.rs:13:5
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:14:5
23+
--> $DIR/open_options.rs:15:5
2424
|
2525
LL | OpenOptions::new().create(true).create(false).open("foo.txt");
2626
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2727

28+
error: file opened with `create`, but `truncate` behavior not defined
29+
--> $DIR/open_options.rs:15:5
30+
|
31+
LL | OpenOptions::new().create(true).create(false).open("foo.txt");
32+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
33+
|
34+
= note: `-D clippy::suspicious-open-options` implied by `-D warnings`
35+
= help: to override `-D warnings` add `#[allow(clippy::suspicious_open_options)]`
36+
2837
error: the method `write` is called more than once
29-
--> $DIR/open_options.rs:16:5
38+
--> $DIR/open_options.rs:17:5
3039
|
3140
LL | OpenOptions::new().write(true).write(false).open("foo.txt");
3241
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3342

3443
error: the method `append` is called more than once
35-
--> $DIR/open_options.rs:18:5
44+
--> $DIR/open_options.rs:19:5
3645
|
3746
LL | OpenOptions::new().append(true).append(false).open("foo.txt");
3847
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3948

4049
error: the method `truncate` is called more than once
41-
--> $DIR/open_options.rs:20:5
50+
--> $DIR/open_options.rs:21:5
4251
|
4352
LL | OpenOptions::new().truncate(true).truncate(false).open("foo.txt");
4453
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
4554

46-
error: aborting due to 7 previous errors
55+
error: file opened with `create`, but `truncate` behavior not defined
56+
--> $DIR/open_options.rs:23:5
57+
|
58+
LL | OpenOptions::new().create(true).open("foo.txt");
59+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
60+
61+
error: aborting due to 9 previous errors
4762

‎tests/ui/seek_to_start_instead_of_rewind.fixed

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ fn main() {
8080
.write(true)
8181
.read(true)
8282
.create(true)
83+
.truncate(true)
8384
.open("foo.txt")
8485
.unwrap();
8586

@@ -104,6 +105,7 @@ fn msrv_1_54() {
104105
.write(true)
105106
.read(true)
106107
.create(true)
108+
.truncate(true)
107109
.open("foo.txt")
108110
.unwrap();
109111

@@ -124,6 +126,7 @@ fn msrv_1_55() {
124126
.write(true)
125127
.read(true)
126128
.create(true)
129+
.truncate(true)
127130
.open("foo.txt")
128131
.unwrap();
129132

0 commit comments

Comments
 (0)