new lint: bufreader_stdin#17029
Conversation
|
r? @llogiq rustbot has assigned @llogiq. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Note that the suggestion cannot probably more than MaybeIncorrect, as the surrounding code might expect a BufReader, and not any type implementing BufRead.
The category should probably be upgraded to at least pedantic because of the risk of false positives (due to the type change).
| && let TyKind::Path(ref qpath) = ty.kind | ||
| && let Some(did) = cx.qpath_res(qpath, ty.hir_id).opt_def_id() | ||
| && cx.tcx.def_path_str(did).ends_with("std::io::BufReader") |
There was a problem hiding this comment.
This is not the proper way to check for a given type. Look at the clippy_utils::paths module for that.
| "using `BufReader::new` with `Stdin`", | ||
| "instead of wrapping `Stdin` in `BufReader`, use the `lock` method directly", | ||
| format!("{}.lock()", snippet(cx, arg.span, "..")), | ||
| Applicability::MachineApplicable, |
There was a problem hiding this comment.
What if the expression doesn't come from a macro but part of it do? For example, BufReader::new(mac!(123)) where mac!(123) expands to std::io::stdin()? Please include examples involving such macros.
| format!("{}.lock()", snippet(cx, arg.span, "..")), | ||
| Applicability::MachineApplicable, | ||
| ); | ||
| } else if cx.tcx.def_path_str(arg_did).ends_with("std::io::StdinLock") { |
There was a problem hiding this comment.
Ditto, use clippy_utils::paths if there is no diagnostic item. We'll add one later.
| Box::new(|_| Box::new(unused_peekable::UnusedPeekable)), | ||
| Box::new(|_| Box::new(bool_to_int_with_if::BoolToIntWithIf)), | ||
| Box::new(|_| Box::new(box_default::BoxDefault)), | ||
| Box::new(move |_| Box::new(bufreader_stdin::BufreaderStdin::new())), |
There was a problem hiding this comment.
The syntax for registering lints has recently changed. Please rebase your PR and add your lint at the end of the lint list.
| expr.span, | ||
| "using `BufReader::new` with `Stdin`", | ||
| "instead of wrapping `Stdin` in `BufReader`, use the `lock` method directly", | ||
| format!("{}.lock()", snippet(cx, arg.span, "..")), |
There was a problem hiding this comment.
You want to check the applicability here, in case the source cannot be retrieved.
| cx, | ||
| BUFREADER_STDIN, | ||
| expr.span, | ||
| "using `BufReader::new` with `StdinLock`", |
There was a problem hiding this comment.
The error message could be clearer:
| "using `BufReader::new` with `StdinLock`", | |
| "wrapping already buffered `StdinLock` into a `BufReader`", |
|
Reminder, once the PR becomes ready for a review, use |
|
@JaafarTanoukhi Hi! Are you planning to continue work on this? I'd love to pick this up. :) |
|
@DanBondarenko I was planning to get back to it sometime this week, but I don't mind if you'd like to take over. |
|
@JaafarTanoukhi No worries, I'll leave it with you. Happy to help if anything comes up. |
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fe83bad to
346b56a
Compare
|
Lintcheck changes for 0a80617
This comment will be updated if you push new changes |
|
@rustbot ready |
| .ty_adt_def() | ||
| .map(rustc_middle::ty::AdtDef::did) | ||
| { | ||
| let snip = snippet(cx, arg.span, ".."); |
There was a problem hiding this comment.
Use snippet_opt() instead, that would be cleaner than checking the output.
| let (msg, help, sugg) = match arg_did_name { | ||
| Some(sym::Stdin) => ( | ||
| "wrapping already buffered `Stdin` into a `BufReader`", | ||
| "instead of wrapping `Stdin` in `BufReader`, use the `lock` method directly", | ||
| format!("{snip}.lock()"), | ||
| ), | ||
| Some(sym::StdinLock) => ( | ||
| "wrapping already buffered `StdinLock` into a `BufReader`", | ||
| "instead of wrapping `StdinLock` in `BufReader`, use it directly", | ||
| snip.to_string(), | ||
| ), | ||
| _ => return, | ||
| }; |
There was a problem hiding this comment.
It would be better to make sure we will lint before using snippet() (or snippet_opt()) which require accessing the source. Here, we might have used snippet() while we are not yet sure we will lint. Also, the suggestion would be best built inside a span_lint_and_then() to ensure that the new string will be allocated only if the lint is actually emitted.
| .typeck_results() | ||
| .expr_ty(arg) | ||
| .ty_adt_def() | ||
| .map(rustc_middle::ty::AdtDef::did) |
There was a problem hiding this comment.
Note: rustc_middle::ty is usually imported, not qualified.
| && let TyKind::Path(ref qpath) = ty.kind | ||
| && let Some(did) = cx.qpath_res(qpath, ty.hir_id).opt_def_id() | ||
| && cx.tcx.is_diagnostic_item(sym::IoBufReader, did) | ||
| && let Some(arg_did) = cx | ||
| .typeck_results() | ||
| .expr_ty(arg) | ||
| .ty_adt_def() | ||
| .map(rustc_middle::ty::AdtDef::did) |
There was a problem hiding this comment.
This can be made shorter using clippy_utils::res traits.
| && let TyKind::Path(ref qpath) = ty.kind | |
| && let Some(did) = cx.qpath_res(qpath, ty.hir_id).opt_def_id() | |
| && cx.tcx.is_diagnostic_item(sym::IoBufReader, did) | |
| && let Some(arg_did) = cx | |
| .typeck_results() | |
| .expr_ty(arg) | |
| .ty_adt_def() | |
| .map(rustc_middle::ty::AdtDef::did) | |
| && ty.basic_res().is_diag_item(cx, sym::IoBufReader) | |
| && let Some(arg_did) = cx.typeck_results().expr_ty(arg).opt_def_id() |
| if let ExprKind::Call(func, [arg]) = expr.kind | ||
| && let ExprKind::Path(QPath::TypeRelative(ty, segment)) = func.kind | ||
| && segment.ident.name == sym::new | ||
| && let TyKind::Path(ref qpath) = ty.kind |
There was a problem hiding this comment.
Note: we usually write this as ty::Path, not TyKind::Path.
View all comments
changelog: [
bufreader_stdin]: new lintfixes #6755