-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #11608 - atwam:suspicious-open-options, r=y21
Add suspicious_open_options lint. changelog: [`suspicious_open_options`]: Checks for the suspicious use of std::fs::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); ``` - [x] Followed [lint naming conventions][lint_naming] - [x] Added passing UI tests (including committed `.stderr` file) - [x] `cargo test` passes locally - [x] Executed `cargo dev update_lints` - [x] Added lint documentation - [x] Run `cargo dev fmt`
- Loading branch information
Showing
15 changed files
with
283 additions
and
122 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,178 +1,206 @@ | ||
use clippy_utils::diagnostics::span_lint; | ||
use clippy_utils::ty::is_type_diagnostic_item; | ||
use rustc_data_structures::fx::FxHashMap; | ||
|
||
use clippy_utils::diagnostics::{span_lint, span_lint_and_then}; | ||
use clippy_utils::ty::{is_type_diagnostic_item, match_type}; | ||
use clippy_utils::{match_any_def_paths, paths}; | ||
use rustc_ast::ast::LitKind; | ||
use rustc_hir::{Expr, ExprKind}; | ||
use rustc_lint::LateContext; | ||
use rustc_middle::ty::Ty; | ||
use rustc_span::source_map::Spanned; | ||
use rustc_span::{sym, Span}; | ||
|
||
use super::NONSENSICAL_OPEN_OPTIONS; | ||
use super::{NONSENSICAL_OPEN_OPTIONS, SUSPICIOUS_OPEN_OPTIONS}; | ||
|
||
fn is_open_options(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { | ||
is_type_diagnostic_item(cx, ty, sym::FsOpenOptions) || match_type(cx, ty, &paths::TOKIO_IO_OPEN_OPTIONS) | ||
} | ||
|
||
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, recv: &'tcx Expr<'_>) { | ||
if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id) | ||
&& let Some(impl_id) = cx.tcx.impl_of_method(method_id) | ||
&& is_type_diagnostic_item(cx, cx.tcx.type_of(impl_id).instantiate_identity(), sym::FsOpenOptions) | ||
&& is_open_options(cx, cx.tcx.type_of(impl_id).instantiate_identity()) | ||
{ | ||
let mut options = Vec::new(); | ||
get_open_options(cx, recv, &mut options); | ||
check_open_options(cx, &options, e.span); | ||
if get_open_options(cx, recv, &mut options) { | ||
check_open_options(cx, &options, e.span); | ||
} | ||
} | ||
} | ||
|
||
#[derive(Debug, PartialEq, Eq, Clone, Copy)] | ||
#[derive(Eq, PartialEq, Clone, Debug)] | ||
enum Argument { | ||
True, | ||
False, | ||
Set(bool), | ||
Unknown, | ||
} | ||
|
||
#[derive(Debug)] | ||
#[derive(Debug, Eq, PartialEq, Hash, Clone)] | ||
enum OpenOption { | ||
Write, | ||
Append, | ||
Create, | ||
CreateNew, | ||
Read, | ||
Truncate, | ||
Create, | ||
Append, | ||
Write, | ||
} | ||
impl std::fmt::Display for OpenOption { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
match self { | ||
OpenOption::Append => write!(f, "append"), | ||
OpenOption::Create => write!(f, "create"), | ||
OpenOption::CreateNew => write!(f, "create_new"), | ||
OpenOption::Read => write!(f, "read"), | ||
OpenOption::Truncate => write!(f, "truncate"), | ||
OpenOption::Write => write!(f, "write"), | ||
} | ||
} | ||
} | ||
|
||
fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec<(OpenOption, Argument)>) { | ||
if let ExprKind::MethodCall(path, receiver, arguments, _) = argument.kind { | ||
/// Collects information about a method call chain on `OpenOptions`. | ||
/// Returns false if an unexpected expression kind was found "on the way", | ||
/// and linting should then be avoided. | ||
fn get_open_options( | ||
cx: &LateContext<'_>, | ||
argument: &Expr<'_>, | ||
options: &mut Vec<(OpenOption, Argument, Span)>, | ||
) -> bool { | ||
if let ExprKind::MethodCall(path, receiver, arguments, span) = argument.kind { | ||
let obj_ty = cx.typeck_results().expr_ty(receiver).peel_refs(); | ||
|
||
// Only proceed if this is a call on some object of type std::fs::OpenOptions | ||
if is_type_diagnostic_item(cx, obj_ty, sym::FsOpenOptions) && !arguments.is_empty() { | ||
if !arguments.is_empty() && is_open_options(cx, obj_ty) { | ||
let argument_option = match arguments[0].kind { | ||
ExprKind::Lit(span) => { | ||
if let Spanned { | ||
node: LitKind::Bool(lit), | ||
.. | ||
} = span | ||
{ | ||
if *lit { Argument::True } else { Argument::False } | ||
Argument::Set(*lit) | ||
} else { | ||
// The function is called with a literal which is not a boolean literal. | ||
// This is theoretically possible, but not very likely. | ||
return; | ||
// We'll ignore it for now | ||
return get_open_options(cx, receiver, options); | ||
} | ||
}, | ||
_ => Argument::Unknown, | ||
}; | ||
|
||
match path.ident.as_str() { | ||
"create" => { | ||
options.push((OpenOption::Create, argument_option)); | ||
options.push((OpenOption::Create, argument_option, span)); | ||
}, | ||
"create_new" => { | ||
options.push((OpenOption::CreateNew, argument_option, span)); | ||
}, | ||
"append" => { | ||
options.push((OpenOption::Append, argument_option)); | ||
options.push((OpenOption::Append, argument_option, span)); | ||
}, | ||
"truncate" => { | ||
options.push((OpenOption::Truncate, argument_option)); | ||
options.push((OpenOption::Truncate, argument_option, span)); | ||
}, | ||
"read" => { | ||
options.push((OpenOption::Read, argument_option)); | ||
options.push((OpenOption::Read, argument_option, span)); | ||
}, | ||
"write" => { | ||
options.push((OpenOption::Write, argument_option)); | ||
options.push((OpenOption::Write, argument_option, span)); | ||
}, | ||
_ => { | ||
// Avoid linting altogether if this method is from a trait. | ||
// This might be a user defined extension trait with a method like `truncate_write` | ||
// which would be a false positive | ||
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(argument.hir_id) | ||
&& cx.tcx.trait_of_item(method_def_id).is_some() | ||
{ | ||
return false; | ||
} | ||
}, | ||
_ => (), | ||
} | ||
|
||
get_open_options(cx, receiver, options); | ||
get_open_options(cx, receiver, options) | ||
} else { | ||
false | ||
} | ||
} else if let ExprKind::Call(callee, _) = argument.kind | ||
&& let ExprKind::Path(path) = callee.kind | ||
&& let Some(did) = cx.qpath_res(&path, callee.hir_id).opt_def_id() | ||
{ | ||
match_any_def_paths( | ||
cx, | ||
did, | ||
&[ | ||
&paths::TOKIO_IO_OPEN_OPTIONS_NEW, | ||
&paths::OPEN_OPTIONS_NEW, | ||
&paths::FILE_OPTIONS, | ||
&paths::TOKIO_FILE_OPTIONS, | ||
], | ||
) | ||
.is_some() | ||
} else { | ||
false | ||
} | ||
} | ||
|
||
fn check_open_options(cx: &LateContext<'_>, options: &[(OpenOption, Argument)], span: Span) { | ||
let (mut create, mut append, mut truncate, mut read, mut write) = (false, false, false, false, false); | ||
let (mut create_arg, mut append_arg, mut truncate_arg, mut read_arg, mut write_arg) = | ||
(false, false, false, false, false); | ||
// This code is almost duplicated (oh, the irony), but I haven't found a way to | ||
// unify it. | ||
|
||
for option in options { | ||
match *option { | ||
(OpenOption::Create, arg) => { | ||
if create { | ||
span_lint( | ||
cx, | ||
NONSENSICAL_OPEN_OPTIONS, | ||
span, | ||
"the method `create` is called more than once", | ||
); | ||
} else { | ||
create = true; | ||
} | ||
create_arg = create_arg || (arg == Argument::True); | ||
}, | ||
(OpenOption::Append, arg) => { | ||
if append { | ||
span_lint( | ||
cx, | ||
NONSENSICAL_OPEN_OPTIONS, | ||
span, | ||
"the method `append` is called more than once", | ||
); | ||
} else { | ||
append = true; | ||
} | ||
append_arg = append_arg || (arg == Argument::True); | ||
}, | ||
(OpenOption::Truncate, arg) => { | ||
if truncate { | ||
span_lint( | ||
cx, | ||
NONSENSICAL_OPEN_OPTIONS, | ||
span, | ||
"the method `truncate` is called more than once", | ||
); | ||
} else { | ||
truncate = true; | ||
} | ||
truncate_arg = truncate_arg || (arg == Argument::True); | ||
}, | ||
(OpenOption::Read, arg) => { | ||
if read { | ||
span_lint( | ||
cx, | ||
NONSENSICAL_OPEN_OPTIONS, | ||
span, | ||
"the method `read` is called more than once", | ||
); | ||
} else { | ||
read = true; | ||
} | ||
read_arg = read_arg || (arg == Argument::True); | ||
}, | ||
(OpenOption::Write, arg) => { | ||
if write { | ||
span_lint( | ||
cx, | ||
NONSENSICAL_OPEN_OPTIONS, | ||
span, | ||
"the method `write` is called more than once", | ||
); | ||
} else { | ||
write = true; | ||
} | ||
write_arg = write_arg || (arg == Argument::True); | ||
}, | ||
fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument, Span)], span: Span) { | ||
// The args passed to these methods, if they have been called | ||
let mut options = FxHashMap::default(); | ||
for (option, arg, sp) in settings { | ||
if let Some((_, prev_span)) = options.insert(option.clone(), (arg.clone(), *sp)) { | ||
span_lint( | ||
cx, | ||
NONSENSICAL_OPEN_OPTIONS, | ||
prev_span, | ||
&format!("the method `{}` is called more than once", &option), | ||
); | ||
} | ||
} | ||
|
||
if read && truncate && read_arg && truncate_arg && !(write && write_arg) { | ||
if let Some((Argument::Set(true), _)) = options.get(&OpenOption::Read) | ||
&& let Some((Argument::Set(true), _)) = options.get(&OpenOption::Truncate) | ||
&& let None | Some((Argument::Set(false), _)) = options.get(&OpenOption::Write) | ||
{ | ||
span_lint( | ||
cx, | ||
NONSENSICAL_OPEN_OPTIONS, | ||
span, | ||
"file opened with `truncate` and `read`", | ||
); | ||
} | ||
if append && truncate && append_arg && truncate_arg { | ||
|
||
if let Some((Argument::Set(true), _)) = options.get(&OpenOption::Append) | ||
&& let Some((Argument::Set(true), _)) = options.get(&OpenOption::Truncate) | ||
{ | ||
span_lint( | ||
cx, | ||
NONSENSICAL_OPEN_OPTIONS, | ||
span, | ||
"file opened with `append` and `truncate`", | ||
); | ||
} | ||
|
||
if let Some((Argument::Set(true), create_span)) = options.get(&OpenOption::Create) | ||
&& let None = options.get(&OpenOption::Truncate) | ||
&& let None | Some((Argument::Set(false), _)) = options.get(&OpenOption::Append) | ||
{ | ||
span_lint_and_then( | ||
cx, | ||
SUSPICIOUS_OPEN_OPTIONS, | ||
*create_span, | ||
"file opened with `create`, but `truncate` behavior not defined", | ||
|diag| { | ||
diag.span_suggestion( | ||
create_span.shrink_to_hi(), | ||
"add", | ||
".truncate(true)".to_string(), | ||
rustc_errors::Applicability::MaybeIncorrect, | ||
) | ||
.help("if you intend to overwrite an existing file entirely, call `.truncate(true)`") | ||
.help( | ||
"if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`", | ||
) | ||
.help("alternatively, use `.append(true)` to append to the file instead of overwriting it"); | ||
}, | ||
); | ||
} | ||
} |
Oops, something went wrong.