-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add lint to suggest into() to construct reference-counted slice
#2807
Closed
rudyardrichter
wants to merge
6
commits into
rust-lang:master
from
rudyardrichter:use_shared_from_slice
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5c2fc4e
Add lint to suggest `into()`
b9e161a
Fix dogfood test
ff3228c
Fix check_expr and add check_local
8e290cc
Merge branch 'master' of github.com:rust-lang-nursery/rust-clippy int…
bb17e8b
Remove extra line
4e3263d
Use if let instead of match
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| use rustc::hir; | ||
| use rustc::hir::{Expr, Local}; | ||
| use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; | ||
| use rustc::ty; | ||
| use crate::utils::{match_qpath, match_type, match_type_parameter, snippet, span_lint_and_sugg, walk_ptrs_ty, get_type_parameter}; | ||
| use crate::utils::paths; | ||
|
|
||
| /// **What it does:** | ||
| /// Checks for usage of `Rc<String>` or `Rc<Vec<T>>`. | ||
| /// | ||
| /// **Why is this bad?** | ||
| /// Using `Rc<str>` or `Rc<[T]>` is more efficient and easy to construct with | ||
| /// `into()`. | ||
| /// | ||
| /// **Known problems:** | ||
| /// None. | ||
| /// | ||
| /// **Example:** | ||
| /// | ||
| /// ```rust | ||
| /// use std::rc::Rc; | ||
| /// | ||
| /// // Bad | ||
| /// let bad_ref: Rc<Vec<usize>> = Rc::new(vec!(1, 2, 3)); | ||
| /// | ||
| /// // Good | ||
| /// let good_ref: Rc<[usize]> = vec!(1, 2, 3).into(); | ||
| /// ``` | ||
| declare_clippy_lint! { | ||
| pub USE_SHARED_FROM_SLICE, | ||
| nursery, | ||
| "constructing reference-counted type from `Vec` or `String`" | ||
| } | ||
|
|
||
| #[derive(Copy, Clone)] | ||
| pub struct Pass; | ||
|
|
||
| impl LintPass for Pass { | ||
| fn get_lints(&self) -> LintArray { | ||
| lint_array!(USE_SHARED_FROM_SLICE) | ||
| } | ||
| } | ||
|
|
||
| /// If the given `expr` is constructing an `Rc` or `Arc` containing a `Vec` or | ||
| /// `String`, output a suggestion to fix accordingly. | ||
| fn check_rc_construction<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { | ||
| let expr_ty = walk_ptrs_ty(cx.tables.expr_ty(expr)); | ||
|
|
||
| // Check for expressions with the type `Rc<Vec<T>>`. | ||
| if_chain! { | ||
| // Check if this expression is constructing an `Rc` or `Arc`. | ||
| let is_rc = match_type(cx, expr_ty, &paths::RC); | ||
| let is_arc = match_type(cx, expr_ty, &paths::ARC); | ||
| if is_rc || is_arc; | ||
|
|
||
| // Check if the `Rc` or `Arc` is constructed with `Vec` or `String`. | ||
| if let ty::TyAdt(_, subst) = expr_ty.sty; | ||
| let arg_type = subst.type_at(0); | ||
| let arg_is_vec = match_type(cx, arg_type, &paths::VEC); | ||
| let arg_is_string = match_type(cx, arg_type, &paths::STRING); | ||
| if arg_is_vec || arg_is_string; | ||
|
|
||
| // Get the argument, to use for writing out the lint message. | ||
| if let hir::ExprCall(_, ref args) = expr.node; | ||
| if let Some(arg) = args.get(0); | ||
|
|
||
| then { | ||
| if arg_is_vec { | ||
| let msg = "avoid constructing reference-counted type from Vec; convert from slice instead"; | ||
| let help = "use"; | ||
| let argument = snippet(cx, arg.span.source_callsite(), ".."); | ||
| let sugg = format!("{}.into()", argument); | ||
| span_lint_and_sugg(cx, USE_SHARED_FROM_SLICE, expr.span, &msg, help, sugg); | ||
| } else if arg_is_string { | ||
| let msg = "avoid constructing reference-counted type from String; convert from slice instead"; | ||
| let help = "use"; | ||
| let argument = snippet(cx, arg.span.source_callsite(), ".."); | ||
| let sugg = format!("{}.as_str().into()", argument); | ||
| span_lint_and_sugg(cx, USE_SHARED_FROM_SLICE, expr.span, &msg, help, sugg); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Check a type declaration to lint, such as in | ||
| /// | ||
| /// let x: Rc<String> = Rc::new(some_string) | ||
| /// | ||
| /// If `ty`, the declared type, is an `Rc` or `Arc` containing a `Vec` or | ||
| /// `String` then output a suggestion to change it. | ||
| fn check_rc_type<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: &hir::Ty) { | ||
| if let hir::TyPath(ref qpath) = ty.node { | ||
| let matches_rc = match_qpath(qpath, &paths::RC); | ||
| let matches_arc = match_qpath(qpath, &paths::ARC); | ||
| if matches_rc || matches_arc { | ||
| let has_vec = match_type_parameter(cx, qpath, &paths::VEC); | ||
| let has_string = match_type_parameter(cx, qpath, &paths::STRING); | ||
| // Keep the type for making suggestions later. | ||
| let constructor = if matches_arc { "Arc" } else { "Rc" }; | ||
| if_chain! { | ||
| if has_vec; | ||
| // In the case we have something like `Rc<Vec<usize>>`, get the inner parameter | ||
| // type out from the parameter type of the `Rc`; so in this example, get the | ||
| // type `usize`. Use this to suggest using the type `Rc<[usize]>` instead. | ||
| let mut vec_ty = get_type_parameter(qpath).expect(""); | ||
| if let hir::TyPath(ref vec_qpath) = vec_ty.node; | ||
| if let Some(param_ty) = get_type_parameter(&vec_qpath); | ||
| then { | ||
| let msg = "use slice instead of `Vec` in reference-counted type"; | ||
| let help = "use"; | ||
| let sugg = format!("{}<[{}]>", constructor, snippet(cx, param_ty.span.source_callsite(), "..")); | ||
| span_lint_and_sugg(cx, USE_SHARED_FROM_SLICE, ty.span, msg, help, sugg); | ||
| } | ||
| } | ||
| if has_string { | ||
| let msg = "use slice instead of `String` in reference-counted type"; | ||
| let help = "use"; | ||
| let sugg = format!("{}<str>", constructor); | ||
| span_lint_and_sugg(cx, USE_SHARED_FROM_SLICE, ty.span, msg, help, sugg); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl <'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { | ||
| fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { | ||
| check_rc_construction(cx, expr); | ||
| } | ||
|
|
||
| fn check_local(&mut self, cx: &LateContext<'a, 'tcx>, local: &Local) { | ||
| if let Some(ref ty) = local.ty { | ||
| check_rc_type(cx, ty); | ||
| } | ||
| } | ||
| } | ||
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 |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| use std::rc::Rc; | ||
| use std::rc; | ||
| use std::sync::Arc; | ||
|
|
||
| #[warn(clippy, use_shared_from_slice)] | ||
| #[allow(unused_variables)] | ||
| fn main() { | ||
| // Test constructing `Rc` directly from `vec!` macro. | ||
| let bad_rc_vec_0: Rc<Vec<usize>> = Rc::new(vec!(1, 2, 3)); | ||
| let bad_rc_vec_00: rc::Rc<Vec<usize>> = rc::Rc::new(vec!(1, 2, 3)); | ||
| // Test constructing `Rc` from `Vec` variable. | ||
| let example_vec: Vec<usize> = vec!(4, 5, 6); | ||
| let bad_rc_vec_1: Rc<Vec<usize>> = Rc::new(example_vec); | ||
| // Test constructing `Rc` with a `String`. | ||
| let bad_rc_string_0: Rc<String> = Rc::new("test".to_string()); | ||
| // Test constructing `Rc` with a `String` variable. | ||
| let example_string: String = "test".to_string(); | ||
| let bad_rc_string_1: Rc<String> = Rc::new(example_string); | ||
|
|
||
| // Test constructing `Arc` from `vec!` macro. | ||
| let bad_arc_vec_0: Arc<Vec<usize>> = Arc::new(vec!(1, 2, 3)); | ||
| // Test constructing `Arc` from `Vec` variable. | ||
| let example_vec: Vec<usize> = vec!(4, 5, 6); | ||
| let bad_arc_vec_1: Arc<Vec<usize>> = Arc::new(example_vec); | ||
| // Test constructing `Arc` with a `String`. | ||
| let bad_arc_string_0: Arc<String> = Arc::new("test".to_string()); | ||
| // Test constructing `Arc` with a `String` variable. | ||
| let example_string: String = "test".to_string(); | ||
| let bad_arc_string_0: Arc<String> = Arc::new(example_string); | ||
|
|
||
| // Test that using `.into()` doesn't cause suggestions. | ||
| let good_rc_0: Rc<[usize]> = vec!(1, 2, 3).into(); | ||
| let example_vec: Vec<usize> = vec!(4, 5, 6); | ||
| let good_rc_1: Rc<[usize]> = example_vec.into(); | ||
| } |
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 |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| error: use slice instead of `Vec` in reference-counted type | ||
| --> $DIR/use_shared_from_slice.rs:9:23 | ||
| | | ||
| 9 | let bad_rc_vec_0: Rc<Vec<usize>> = Rc::new(vec!(1, 2, 3)); | ||
| | ^^^^^^^^^^^^^^ help: use: `Rc<[usize]>` | ||
| | | ||
| = note: `-D use-shared-from-slice` implied by `-D warnings` | ||
|
|
||
| error: avoid constructing reference-counted type from Vec; convert from slice instead | ||
| --> $DIR/use_shared_from_slice.rs:9:40 | ||
| | | ||
| 9 | let bad_rc_vec_0: Rc<Vec<usize>> = Rc::new(vec!(1, 2, 3)); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ help: use: `vec!(1, 2, 3).into()` | ||
|
|
||
| error: use slice instead of `Vec` in reference-counted type | ||
| --> $DIR/use_shared_from_slice.rs:10:24 | ||
| | | ||
| 10 | let bad_rc_vec_00: rc::Rc<Vec<usize>> = rc::Rc::new(vec!(1, 2, 3)); | ||
| | ^^^^^^^^^^^^^^^^^^ help: use: `Rc<[usize]>` | ||
|
|
||
| error: avoid constructing reference-counted type from Vec; convert from slice instead | ||
| --> $DIR/use_shared_from_slice.rs:10:45 | ||
| | | ||
| 10 | let bad_rc_vec_00: rc::Rc<Vec<usize>> = rc::Rc::new(vec!(1, 2, 3)); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `vec!(1, 2, 3).into()` | ||
|
|
||
| error: use slice instead of `Vec` in reference-counted type | ||
| --> $DIR/use_shared_from_slice.rs:13:23 | ||
| | | ||
| 13 | let bad_rc_vec_1: Rc<Vec<usize>> = Rc::new(example_vec); | ||
| | ^^^^^^^^^^^^^^ help: use: `Rc<[usize]>` | ||
|
|
||
| error: avoid constructing reference-counted type from Vec; convert from slice instead | ||
| --> $DIR/use_shared_from_slice.rs:13:40 | ||
| | | ||
| 13 | let bad_rc_vec_1: Rc<Vec<usize>> = Rc::new(example_vec); | ||
| | ^^^^^^^^^^^^^^^^^^^^ help: use: `example_vec.into()` | ||
|
|
||
| error: use slice instead of `String` in reference-counted type | ||
| --> $DIR/use_shared_from_slice.rs:15:26 | ||
| | | ||
| 15 | let bad_rc_string_0: Rc<String> = Rc::new("test".to_string()); | ||
| | ^^^^^^^^^^ help: use: `Rc<str>` | ||
|
|
||
| error: avoid constructing reference-counted type from String; convert from slice instead | ||
| --> $DIR/use_shared_from_slice.rs:15:39 | ||
| | | ||
| 15 | let bad_rc_string_0: Rc<String> = Rc::new("test".to_string()); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `"test".to_string().as_str().into()` | ||
|
|
||
| error: use slice instead of `String` in reference-counted type | ||
| --> $DIR/use_shared_from_slice.rs:18:26 | ||
| | | ||
| 18 | let bad_rc_string_1: Rc<String> = Rc::new(example_string); | ||
| | ^^^^^^^^^^ help: use: `Rc<str>` | ||
|
|
||
| error: avoid constructing reference-counted type from String; convert from slice instead | ||
| --> $DIR/use_shared_from_slice.rs:18:39 | ||
| | | ||
| 18 | let bad_rc_string_1: Rc<String> = Rc::new(example_string); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^ help: use: `example_string.as_str().into()` | ||
|
|
||
| error: use slice instead of `Vec` in reference-counted type | ||
| --> $DIR/use_shared_from_slice.rs:21:24 | ||
| | | ||
| 21 | let bad_arc_vec_0: Arc<Vec<usize>> = Arc::new(vec!(1, 2, 3)); | ||
| | ^^^^^^^^^^^^^^^ help: use: `Arc<[usize]>` | ||
|
|
||
| error: avoid constructing reference-counted type from Vec; convert from slice instead | ||
| --> $DIR/use_shared_from_slice.rs:21:42 | ||
| | | ||
| 21 | let bad_arc_vec_0: Arc<Vec<usize>> = Arc::new(vec!(1, 2, 3)); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^ help: use: `vec!(1, 2, 3).into()` | ||
|
|
||
| error: use slice instead of `Vec` in reference-counted type | ||
| --> $DIR/use_shared_from_slice.rs:24:24 | ||
| | | ||
| 24 | let bad_arc_vec_1: Arc<Vec<usize>> = Arc::new(example_vec); | ||
| | ^^^^^^^^^^^^^^^ help: use: `Arc<[usize]>` | ||
|
|
||
| error: avoid constructing reference-counted type from Vec; convert from slice instead | ||
| --> $DIR/use_shared_from_slice.rs:24:42 | ||
| | | ||
| 24 | let bad_arc_vec_1: Arc<Vec<usize>> = Arc::new(example_vec); | ||
| | ^^^^^^^^^^^^^^^^^^^^^ help: use: `example_vec.into()` | ||
|
|
||
| error: use slice instead of `String` in reference-counted type | ||
| --> $DIR/use_shared_from_slice.rs:26:27 | ||
| | | ||
| 26 | let bad_arc_string_0: Arc<String> = Arc::new("test".to_string()); | ||
| | ^^^^^^^^^^^ help: use: `Arc<str>` | ||
|
|
||
| error: avoid constructing reference-counted type from String; convert from slice instead | ||
| --> $DIR/use_shared_from_slice.rs:26:41 | ||
| | | ||
| 26 | let bad_arc_string_0: Arc<String> = Arc::new("test".to_string()); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `"test".to_string().as_str().into()` | ||
|
|
||
| error: use slice instead of `String` in reference-counted type | ||
| --> $DIR/use_shared_from_slice.rs:29:27 | ||
| | | ||
| 29 | let bad_arc_string_0: Arc<String> = Arc::new(example_string); | ||
| | ^^^^^^^^^^^ help: use: `Arc<str>` | ||
|
|
||
| error: avoid constructing reference-counted type from String; convert from slice instead | ||
| --> $DIR/use_shared_from_slice.rs:29:41 | ||
| | | ||
| 29 | let bad_arc_string_0: Arc<String> = Arc::new(example_string); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `example_string.as_str().into()` | ||
|
|
||
| error: aborting due to 18 previous errors | ||
|
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a little dangerous, because the message might appear even if the user has no way to change the type. For
Rc<Vec<[T]>>this is is kinda fine, because the suggestion will still work, but forRc<String>I'm not sure if.as_str().into()is equivalent.