Skip to content

Commit 515fe65

Browse files
committed
Fix conflicts
- New ineffective_open_options had to be fixed. - Now not raising an issue on missing `truncate` when `append(true)` makes the intent clear. - Try implementing more advanced tests for non-chained operations. Fail
1 parent 84588a8 commit 515fe65

File tree

5 files changed

+44
-12
lines changed

5 files changed

+44
-12
lines changed

clippy_lints/src/methods/open_options.rs

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
use rustc_data_structures::fx::FxHashMap;
22

33
use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
4-
use clippy_utils::paths;
54
use clippy_utils::ty::{is_type_diagnostic_item, match_type};
5+
use clippy_utils::{match_any_def_paths, paths};
66
use rustc_ast::ast::LitKind;
77
use rustc_hir::{Expr, ExprKind};
88
use rustc_lint::LateContext;
@@ -19,15 +19,15 @@ 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();
2525
get_open_options(cx, recv, &mut options);
2626
check_open_options(cx, &options, e.span);
2727
}
2828
}
2929

30-
#[derive(Eq, PartialEq, Clone)]
30+
#[derive(Eq, PartialEq, Clone, Debug)]
3131
enum Argument {
3232
Set(bool),
3333
Unknown,
@@ -55,7 +55,11 @@ impl std::fmt::Display for OpenOption {
5555
}
5656
}
5757

58-
fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec<(OpenOption, Argument, Span)>) {
58+
fn get_open_options(
59+
cx: &LateContext<'_>,
60+
argument: &Expr<'_>,
61+
options: &mut Vec<(OpenOption, Argument, Span)>,
62+
) -> bool {
5963
if let ExprKind::MethodCall(path, receiver, arguments, span) = argument.kind {
6064
let obj_ty = cx.typeck_results().expr_ty(receiver).peel_refs();
6165

@@ -72,7 +76,8 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
7276
} else {
7377
// The function is called with a literal which is not a boolean literal.
7478
// This is theoretically possible, but not very likely.
75-
return;
79+
// We'll ignore it for now
80+
return get_open_options(cx, receiver, options);
7681
}
7782
},
7883
_ => Argument::Unknown,
@@ -100,8 +105,19 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
100105
_ => (),
101106
}
102107

103-
get_open_options(cx, receiver, options);
108+
get_open_options(cx, receiver, options)
109+
} else {
110+
false
104111
}
112+
} else if let ExprKind::Call(callee, _) = argument.kind
113+
&& let ExprKind::Path(path) = callee.kind
114+
&& let Some(did) = cx.qpath_res(&path, callee.hir_id).opt_def_id()
115+
{
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)
119+
} else {
120+
false
105121
}
106122
}
107123

@@ -144,6 +160,7 @@ fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument, S
144160

145161
if let Some((Argument::Set(true), create_span)) = options.get(&OpenOption::Create)
146162
&& let None = options.get(&OpenOption::Truncate)
163+
&& let None | Some((Argument::Set(false), _)) = options.get(&OpenOption::Append)
147164
{
148165
span_lint_and_then(
149166
cx,
@@ -153,7 +170,7 @@ fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument, S
153170
|diag| {
154171
diag
155172
.span_suggestion(create_span.shrink_to_hi(), "add", ".truncate(true)".to_string(), rustc_errors::Applicability::MaybeIncorrect)
156-
.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)`");
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.");
157174
},
158175
);
159176
}

tests/ui/ineffective_open_options.fixed

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,16 +26,23 @@ fn main() {
2626
.unwrap();
2727
let file = OpenOptions::new()
2828
.create(true)
29+
.truncate(true)
2930
.write(true)
3031
.append(false)
3132
.open("dump.json")
3233
.unwrap();
3334
let file = OpenOptions::new()
3435
.create(true)
36+
.truncate(true)
3537
.write(false)
3638
.append(false)
3739
.open("dump.json")
3840
.unwrap();
3941
let file = OpenOptions::new().create(true).append(true).open("dump.json").unwrap();
40-
let file = OpenOptions::new().create(true).write(true).open("dump.json").unwrap();
42+
let file = OpenOptions::new()
43+
.create(true)
44+
.truncate(true)
45+
.write(true)
46+
.open("dump.json")
47+
.unwrap();
4148
}

tests/ui/ineffective_open_options.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,16 +26,23 @@ fn main() {
2626
.unwrap();
2727
let file = OpenOptions::new()
2828
.create(true)
29+
.truncate(true)
2930
.write(true)
3031
.append(false)
3132
.open("dump.json")
3233
.unwrap();
3334
let file = OpenOptions::new()
3435
.create(true)
36+
.truncate(true)
3537
.write(false)
3638
.append(false)
3739
.open("dump.json")
3840
.unwrap();
3941
let file = OpenOptions::new().create(true).append(true).open("dump.json").unwrap();
40-
let file = OpenOptions::new().create(true).write(true).open("dump.json").unwrap();
42+
let file = OpenOptions::new()
43+
.create(true)
44+
.truncate(true)
45+
.write(true)
46+
.open("dump.json")
47+
.unwrap();
4148
}

tests/ui/open_options.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,5 +29,6 @@ fn main() {
2929
let mut options = std::fs::OpenOptions::new();
3030
options.read(true);
3131
options.read(false);
32-
//~^ ERROR: the method `read` is called more than once
32+
//#~^ ERROR: the method `read` is called more than once
33+
options.open("foo.txt");
3334
}

tests/ui/open_options_fixable.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +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)`
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.
88
= note: `-D clippy::suspicious-open-options` implied by `-D warnings`
99
= help: to override `-D warnings` add `#[allow(clippy::suspicious_open_options)]`
1010

11-
error: aborting due to previous error
11+
error: aborting due to 1 previous error
1212

0 commit comments

Comments
 (0)