Skip to content
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.

Sign up for GitHub

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

Rustup to rust-lang/rust#64874 #4628

Merged
merged 6 commits into from
Oct 8, 2019
Merged

Rustup to rust-lang/rust#64874 #4628

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b46f5b4
Rustup to rust-lang/rust#64874
flip1995 Oct 4, 2019
c420b07
Fix needless_pass_by_value
flip1995 Oct 4, 2019
b7d4735
Merge consume and consume_pat in escape analysis
flip1995 Oct 5, 2019
3b23092
Get rid of rvalue_promotable_map method call
flip1995 Oct 5, 2019
3aa531c
Run util/dev fmt
flip1995 Oct 5, 2019
3d39379
Move is_argument check into mutate
flip1995 Oct 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 6 additions & 10 deletions clippy_lints/src/escape.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BoxedLocal {
cx.param_env,
region_scope_tree,
cx.tables,
None,
)
.consume_body(body);

Expand Down Expand Up @@ -114,15 +113,14 @@ fn is_argument(map: &hir::map::Map<'_>, id: HirId) -> bool {
}

impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> {
fn consume(&mut self, _: HirId, _: Span, cmt: &cmt_<'tcx>, mode: ConsumeMode) {
fn consume(&mut self, cmt: &cmt_<'tcx>, mode: ConsumeMode) {
if let Categorization::Local(lid) = cmt.cat {
if let Move(DirectRefMove) | Move(CaptureMove) = mode {
if let ConsumeMode::Move = mode {
// moved out or in. clearly can't be localized
self.set.remove(&lid);
}
}
}
fn matched_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: MatchMode) {}
fn consume_pat(&mut self, consume_pat: &Pat, cmt: &cmt_<'tcx>, _: ConsumeMode) {
let map = &self.cx.tcx.hir();
if is_argument(map, consume_pat.hir_id) {
Expand All @@ -137,7 +135,7 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> {
}
return;
}
if let Categorization::Rvalue(..) = cmt.cat {
if let Categorization::Rvalue = cmt.cat {
if let Some(Node::Stmt(st)) = map.find(map.get_parent_node(cmt.hir_id)) {
if let StmtKind::Local(ref loc) = st.kind {
if let Some(ref ex) = loc.init {
Expand All @@ -163,12 +161,10 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> {
}
}
}

fn borrow(
&mut self,
_: HirId,
_: Span,
cmt: &cmt_<'tcx>,
_: ty::Region<'_>,
_: ty::BorrowKind,
loan_cause: LoanCause,
) {
Expand All @@ -192,8 +188,8 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> {
}
}
}
fn decl_without_init(&mut self, _: HirId, _: Span) {}
fn mutate(&mut self, _: HirId, _: Span, _: &cmt_<'tcx>, _: MutateMode) {}

fn mutate(&mut self, _: &cmt_<'tcx>) {}
}

impl<'a, 'tcx> EscapeDelegate<'a, 'tcx> {
Expand Down
21 changes: 7 additions & 14 deletions clippy_lints/src/loops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1547,37 +1547,31 @@ struct MutatePairDelegate {
}

impl<'tcx> Delegate<'tcx> for MutatePairDelegate {
fn consume(&mut self, _: HirId, _: Span, _: &cmt_<'tcx>, _: ConsumeMode) {}
fn consume(&mut self, _: &cmt_<'tcx>, _: ConsumeMode) {}

fn matched_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: MatchMode) {}

fn consume_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: ConsumeMode) {}

fn borrow(&mut self, _: HirId, sp: Span, cmt: &cmt_<'tcx>, _: ty::Region<'_>, bk: ty::BorrowKind, _: LoanCause) {
fn borrow(&mut self, cmt: &cmt_<'tcx>, bk: ty::BorrowKind) {
if let ty::BorrowKind::MutBorrow = bk {
if let Categorization::Local(id) = cmt.cat {
if Some(id) == self.hir_id_low {
self.span_low = Some(sp)
self.span_low = Some(cmt.span)
}
if Some(id) == self.hir_id_high {
self.span_high = Some(sp)
self.span_high = Some(cmt.span)
}
}
}
}

fn mutate(&mut self, _: HirId, sp: Span, cmt: &cmt_<'tcx>, _: MutateMode) {
fn mutate(&mut self, cmt: &cmt_<'tcx>) {
if let Categorization::Local(id) = cmt.cat {
if Some(id) == self.hir_id_low {
self.span_low = Some(sp)
self.span_low = Some(cmt.span)
}
if Some(id) == self.hir_id_high {
self.span_high = Some(sp)
self.span_high = Some(cmt.span)
}
}
}

fn decl_without_init(&mut self, _: HirId, _: Span) {}
}

impl<'tcx> MutatePairDelegate {
Expand Down Expand Up @@ -1655,7 +1649,6 @@ fn check_for_mutation(
cx.param_env,
region_scope_tree,
cx.tables,
None,
)
.walk_expr(body);
delegate.mutation_span()
Expand Down
97 changes: 10 additions & 87 deletions clippy_lints/src/needless_pass_by_value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
spans_need_deref,
..
} = {
let mut ctx = MovedVariablesCtxt::new(cx);
let mut ctx = MovedVariablesCtxt::default();
let region_scope_tree = &cx.tcx.region_scope_tree(fn_def_id);
euv::ExprUseVisitor::new(
&mut ctx,
Expand All @@ -143,7 +143,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
cx.param_env,
region_scope_tree,
cx.tables,
None,
)
.consume_body(body);
ctx
Expand Down Expand Up @@ -325,115 +324,39 @@ fn requires_exact_signature(attrs: &[Attribute]) -> bool {
})
}

struct MovedVariablesCtxt<'a, 'tcx> {
cx: &'a LateContext<'a, 'tcx>,
#[derive(Default)]
struct MovedVariablesCtxt {
moved_vars: FxHashSet<HirId>,
/// Spans which need to be prefixed with `*` for dereferencing the
/// suggested additional reference.
spans_need_deref: FxHashMap<HirId, FxHashSet<Span>>,
}

impl<'a, 'tcx> MovedVariablesCtxt<'a, 'tcx> {
fn new(cx: &'a LateContext<'a, 'tcx>) -> Self {
Self {
cx,
moved_vars: FxHashSet::default(),
spans_need_deref: FxHashMap::default(),
}
}

fn move_common(&mut self, _consume_id: HirId, _span: Span, cmt: &mc::cmt_<'tcx>) {
impl MovedVariablesCtxt {
fn move_common(&mut self, cmt: &mc::cmt_<'_>) {
let cmt = unwrap_downcast_or_interior(cmt);

if let mc::Categorization::Local(vid) = cmt.cat {
self.moved_vars.insert(vid);
}
}

fn non_moving_pat(&mut self, matched_pat: &Pat, cmt: &mc::cmt_<'tcx>) {
Copy link
Member Author

@flip1995 flip1995 Oct 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is removing this completely ok? It didn't break anything in the tests 🤔 cc @sinkuu since you added this function in 627d24c

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is inserted to spans_need_deref after this change, so the suggestions to add * should be broken, but idk.

Anyway, Rust has gained default binding mode and this suggestion became useless. I think it's OK to remove it.

let cmt = unwrap_downcast_or_interior(cmt);

if let mc::Categorization::Local(vid) = cmt.cat {
let mut id = matched_pat.hir_id;
loop {
let parent = self.cx.tcx.hir().get_parent_node(id);
if id == parent {
// no parent
return;
}
id = parent;

if let Some(node) = self.cx.tcx.hir().find(id) {
match node {
Node::Expr(e) => {
// `match` and `if let`
if let ExprKind::Match(ref c, ..) = e.kind {
self.spans_need_deref
.entry(vid)
.or_insert_with(FxHashSet::default)
.insert(c.span);
}
},

Node::Stmt(s) => {
// `let <pat> = x;`
if_chain! {
if let StmtKind::Local(ref local) = s.kind;
then {
self.spans_need_deref
.entry(vid)
.or_insert_with(FxHashSet::default)
.insert(local.init
.as_ref()
.map(|e| e.span)
.expect("`let` stmt without init aren't caught by match_pat"));
}
}
},

_ => {},
}
}
}
}
}
}

impl<'a, 'tcx> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
fn consume(&mut self, consume_id: HirId, consume_span: Span, cmt: &mc::cmt_<'tcx>, mode: euv::ConsumeMode) {
if let euv::ConsumeMode::Move(_) = mode {
self.move_common(consume_id, consume_span, cmt);
}
}

fn matched_pat(&mut self, matched_pat: &Pat, cmt: &mc::cmt_<'tcx>, mode: euv::MatchMode) {
if let euv::MatchMode::MovingMatch = mode {
self.move_common(matched_pat.hir_id, matched_pat.span, cmt);
} else {
self.non_moving_pat(matched_pat, cmt);
}
}

fn consume_pat(&mut self, consume_pat: &Pat, cmt: &mc::cmt_<'tcx>, mode: euv::ConsumeMode) {
if let euv::ConsumeMode::Move(_) = mode {
self.move_common(consume_pat.hir_id, consume_pat.span, cmt);
impl<'tcx> euv::Delegate<'tcx> for MovedVariablesCtxt {
fn consume(&mut self, cmt: &mc::cmt_<'tcx>, mode: euv::ConsumeMode) {
if let euv::ConsumeMode::Move = mode {
self.move_common(cmt);
}
}

fn borrow(
&mut self,
_: HirId,
_: Span,
_: &mc::cmt_<'tcx>,
_: ty::Region<'_>,
_: ty::BorrowKind,
_: euv::LoanCause,
) {
}

fn mutate(&mut self, _: HirId, _: Span, _: &mc::cmt_<'tcx>, _: euv::MutateMode) {}

fn decl_without_init(&mut self, _: HirId, _: Span) {}
fn mutate(&mut self, _: &mc::cmt_<'tcx>) {}
}

fn unwrap_downcast_or_interior<'a, 'tcx>(mut cmt: &'a mc::cmt_<'tcx>) -> mc::cmt_<'tcx> {
Expand Down
14 changes: 3 additions & 11 deletions clippy_lints/src/utils/usage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use rustc::middle::mem_categorization::cmt_;
use rustc::middle::mem_categorization::Categorization;
use rustc::ty;
use rustc_data_structures::fx::FxHashSet;
use syntax::source_map::Span;

/// Returns a set of mutated local variable IDs, or `None` if mutations could not be determined.
pub fn mutated_variables<'a, 'tcx>(expr: &'tcx Expr, cx: &'a LateContext<'a, 'tcx>) -> Option<FxHashSet<HirId>> {
Expand All @@ -23,7 +22,6 @@ pub fn mutated_variables<'a, 'tcx>(expr: &'tcx Expr, cx: &'a LateContext<'a, 'tc
cx.param_env,
region_scope_tree,
cx.tables,
None,
)
.walk_expr(expr);

Expand Down Expand Up @@ -66,21 +64,15 @@ impl<'tcx> MutVarsDelegate {
}

impl<'tcx> Delegate<'tcx> for MutVarsDelegate {
fn consume(&mut self, _: HirId, _: Span, _: &cmt_<'tcx>, _: ConsumeMode) {}
fn consume(&mut self, _: &cmt_<'tcx>, _: ConsumeMode) {}

fn matched_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: MatchMode) {}

fn consume_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: ConsumeMode) {}

fn borrow(&mut self, _: HirId, _: Span, cmt: &cmt_<'tcx>, _: ty::Region<'_>, bk: ty::BorrowKind, _: LoanCause) {
fn borrow(&mut self, cmt: &cmt_<'tcx>, bk: ty::BorrowKind) {
if let ty::BorrowKind::MutBorrow = bk {
self.update(&cmt.cat)
}
}

fn mutate(&mut self, _: HirId, _: Span, cmt: &cmt_<'tcx>, _: MutateMode) {
fn mutate(&mut self, cmt: &cmt_<'tcx>) {
self.update(&cmt.cat)
}

fn decl_without_init(&mut self, _: HirId, _: Span) {}
}
8 changes: 4 additions & 4 deletions tests/ui/mut_range_bound.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,27 @@ error: attempt to mutate range bound within loop; note that the range of the loo
--> $DIR/mut_range_bound.rs:16:9
|
LL | m = 5;
| ^^^^^
| ^
|
= note: `-D clippy::mut-range-bound` implied by `-D warnings`

error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
--> $DIR/mut_range_bound.rs:23:9
|
LL | m *= 2;
| ^^^^^^
| ^

error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
--> $DIR/mut_range_bound.rs:31:9
|
LL | m = 5;
| ^^^^^
| ^

error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
--> $DIR/mut_range_bound.rs:32:9
|
LL | n = 7;
| ^^^^^
| ^

error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
--> $DIR/mut_range_bound.rs:46:22
Expand Down
32 changes: 4 additions & 28 deletions tests/ui/needless_pass_by_value.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,7 @@ error: this argument is passed by value, but not consumed in the function body
--> $DIR/needless_pass_by_value.rs:49:18
|
LL | fn test_match(x: Option<Option<String>>, y: Option<Option<String>>) {
| ^^^^^^^^^^^^^^^^^^^^^^
help: consider taking a reference instead
|
LL | fn test_match(x: &Option<Option<String>>, y: Option<Option<String>>) {
LL | match *x {
|
| ^^^^^^^^^^^^^^^^^^^^^^ help: consider taking a reference instead: `&Option<Option<String>>`

error: this argument is passed by value, but not consumed in the function body
--> $DIR/needless_pass_by_value.rs:62:24
Expand All @@ -45,14 +40,7 @@ error: this argument is passed by value, but not consumed in the function body
--> $DIR/needless_pass_by_value.rs:62:36
|
LL | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) {
| ^^^^^^^
help: consider taking a reference instead
|
LL | fn test_destructure(x: Wrapper, y: &Wrapper, z: Wrapper) {
LL | let Wrapper(s) = z; // moved
LL | let Wrapper(ref t) = *y; // not moved
LL | let Wrapper(_) = *y; // still not moved
|
| ^^^^^^^ help: consider taking a reference instead: `&Wrapper`

error: this argument is passed by value, but not consumed in the function body
--> $DIR/needless_pass_by_value.rs:78:49
Expand Down Expand Up @@ -152,37 +140,25 @@ error: this argument is passed by value, but not consumed in the function body
--> $DIR/needless_pass_by_value.rs:131:45
|
LL | fn test_destructure_copy(x: CopyWrapper, y: CopyWrapper, z: CopyWrapper) {
| ^^^^^^^^^^^
| ^^^^^^^^^^^ help: consider taking a reference instead: `&CopyWrapper`
|
help: consider marking this type as Copy
--> $DIR/needless_pass_by_value.rs:123:1
|
LL | struct CopyWrapper(u32);
| ^^^^^^^^^^^^^^^^^^^^^^^^
help: consider taking a reference instead
|
LL | fn test_destructure_copy(x: CopyWrapper, y: &CopyWrapper, z: CopyWrapper) {
LL | let CopyWrapper(s) = z; // moved
LL | let CopyWrapper(ref t) = *y; // not moved
LL | let CopyWrapper(_) = *y; // still not moved
|

error: this argument is passed by value, but not consumed in the function body
--> $DIR/needless_pass_by_value.rs:131:61
|
LL | fn test_destructure_copy(x: CopyWrapper, y: CopyWrapper, z: CopyWrapper) {
| ^^^^^^^^^^^
| ^^^^^^^^^^^ help: consider taking a reference instead: `&CopyWrapper`
|
help: consider marking this type as Copy
--> $DIR/needless_pass_by_value.rs:123:1
|
LL | struct CopyWrapper(u32);
| ^^^^^^^^^^^^^^^^^^^^^^^^
help: consider taking a reference instead
|
LL | fn test_destructure_copy(x: CopyWrapper, y: CopyWrapper, z: &CopyWrapper) {
LL | let CopyWrapper(s) = *z; // moved
|

error: this argument is passed by value, but not consumed in the function body
--> $DIR/needless_pass_by_value.rs:143:40
Expand Down