1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
use rustc::hir::def_id::DefId;
use rustc::hir::intravisit::{Visitor, walk_expr, NestedVisitorMap};
use rustc::hir::*;
use rustc::ty;
use rustc::lint::*;
use utils::{get_parent_expr, span_note_and_lint, span_lint};

/// **What it does:** Checks for a read and a write to the same variable where
/// whether the read occurs before or after the write depends on the evaluation
/// order of sub-expressions.
///
/// **Why is this bad?** It is often confusing to read. In addition, the
/// sub-expression evaluation order for Rust is not well documented.
///
/// **Known problems:** Code which intentionally depends on the evaluation
/// order, or which is correct for any evaluation order.
///
/// **Example:**
/// ```rust
/// let mut x = 0;
/// let a = {x = 1; 1} + x;
/// // Unclear whether a is 1 or 2.
/// ```
declare_lint! {
    pub EVAL_ORDER_DEPENDENCE,
    Warn,
    "whether a variable read occurs before a write depends on sub-expression evaluation order"
}

/// **What it does:** Checks for diverging calls that are not match arms or statements.
///
/// **Why is this bad?** It is often confusing to read. In addition, the
/// sub-expression evaluation order for Rust is not well documented.
///
/// **Known problems:** Someone might want to use `some_bool || panic!()` as a shorthand.
///
/// **Example:**
/// ```rust
/// let a = b() || panic!() || c();
/// // `c()` is dead, `panic!()` is only called if `b()` returns `false`
/// let x = (a, b, c, panic!());
/// // can simply be replaced by `panic!()`
/// ```
declare_lint! {
    pub DIVERGING_SUB_EXPRESSION,
    Warn,
    "whether an expression contains a diverging sub expression"
}

#[derive(Copy,Clone)]
pub struct EvalOrderDependence;

impl LintPass for EvalOrderDependence {
    fn get_lints(&self) -> LintArray {
        lint_array!(EVAL_ORDER_DEPENDENCE, DIVERGING_SUB_EXPRESSION)
    }
}

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for EvalOrderDependence {
    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
        // Find a write to a local variable.
        match expr.node {
            ExprAssign(ref lhs, _) |
            ExprAssignOp(_, ref lhs, _) => {
                if let ExprPath(ref qpath) = lhs.node {
                    if let QPath::Resolved(_, ref path) = *qpath {
                        if path.segments.len() == 1 {
                            let var = cx.tcx.tables().qpath_def(qpath, lhs.id).def_id();
                            let mut visitor = ReadVisitor {
                                cx: cx,
                                var: var,
                                write_expr: expr,
                                last_expr: expr,
                            };
                            check_for_unsequenced_reads(&mut visitor);
                        }
                    }
                }
            },
            _ => {},
        }
    }
    fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx Stmt) {
        match stmt.node {
            StmtExpr(ref e, _) |
            StmtSemi(ref e, _) => DivergenceVisitor { cx: cx }.maybe_walk_expr(e),
            StmtDecl(ref d, _) => {
                if let DeclLocal(ref local) = d.node {
                    if let Local { init: Some(ref e), .. } = **local {
                        DivergenceVisitor { cx: cx }.visit_expr(e);
                    }
                }
            },
        }
    }
}

struct DivergenceVisitor<'a, 'tcx: 'a> {
    cx: &'a LateContext<'a, 'tcx>,
}

impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> {
    fn maybe_walk_expr(&mut self, e: &'tcx Expr) {
        match e.node {
            ExprClosure(..) => {},
            ExprMatch(ref e, ref arms, _) => {
                self.visit_expr(e);
                for arm in arms {
                    if let Some(ref guard) = arm.guard {
                        self.visit_expr(guard);
                    }
                    // make sure top level arm expressions aren't linted
                    self.maybe_walk_expr(&*arm.body);
                }
            },
            _ => walk_expr(self, e),
        }
    }
    fn report_diverging_sub_expr(&mut self, e: &Expr) {
        span_lint(self.cx, DIVERGING_SUB_EXPRESSION, e.span, "sub-expression diverges");
    }
}

impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> {
    fn visit_expr(&mut self, e: &'tcx Expr) {
        match e.node {
            ExprAgain(_) | ExprBreak(_, _) | ExprRet(_) => self.report_diverging_sub_expr(e),
            ExprCall(ref func, _) => {
                match self.cx.tcx.tables().expr_ty(func).sty {
                    ty::TyFnDef(_, _, fn_ty) |
                    ty::TyFnPtr(fn_ty) => {
                        if let ty::TyNever = self.cx.tcx.erase_late_bound_regions(&fn_ty.sig).output().sty {
                            self.report_diverging_sub_expr(e);
                        }
                    },
                    _ => {},
                }
            },
            ExprMethodCall(..) => {
                let method_call = ty::MethodCall::expr(e.id);
                let borrowed_table = self.cx.tcx.tables.borrow();
                let method_type = borrowed_table.method_map.get(&method_call).expect("This should never happen.");
                let result_ty = method_type.ty.fn_ret();
                if let ty::TyNever = self.cx.tcx.erase_late_bound_regions(&result_ty).sty {
                    self.report_diverging_sub_expr(e);
                }
            },
            _ => {
                // do not lint expressions referencing objects of type `!`, as that required a diverging expression
                // to begin with
            },
        }
        self.maybe_walk_expr(e);
    }
    fn visit_block(&mut self, _: &'tcx Block) {
        // don't continue over blocks, LateLintPass already does that
    }
    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
        NestedVisitorMap::All(&self.cx.tcx.map)
    }
}

/// Walks up the AST from the the given write expression (`vis.write_expr`)
/// looking for reads to the same variable that are unsequenced relative to the
/// write.
///
/// This means reads for which there is a common ancestor between the read and
/// the write such that
///
/// * evaluating the ancestor necessarily evaluates both the read and the write
///   (for example, `&x` and `|| x = 1` don't necessarily evaluate `x`), and
///
/// * which one is evaluated first depends on the order of sub-expression
///   evaluation. Blocks, `if`s, loops, `match`es, and the short-circuiting
///   logical operators are considered to have a defined evaluation order.
///
/// When such a read is found, the lint is triggered.
fn check_for_unsequenced_reads(vis: &mut ReadVisitor) {
    let map = &vis.cx.tcx.map;
    let mut cur_id = vis.write_expr.id;
    loop {
        let parent_id = map.get_parent_node(cur_id);
        if parent_id == cur_id {
            break;
        }
        let parent_node = match map.find(parent_id) {
            Some(parent) => parent,
            None => break,
        };

        let stop_early = match parent_node {
            map::Node::NodeExpr(expr) => check_expr(vis, expr),
            map::Node::NodeStmt(stmt) => check_stmt(vis, stmt),
            map::Node::NodeItem(_) => {
                // We reached the top of the function, stop.
                break;
            },
            _ => StopEarly::KeepGoing,
        };
        match stop_early {
            StopEarly::Stop => break,
            StopEarly::KeepGoing => {},
        }

        cur_id = parent_id;
    }
}

/// Whether to stop early for the loop in `check_for_unsequenced_reads`. (If
/// `check_expr` weren't an independent function, this would be unnecessary and
/// we could just use `break`).
enum StopEarly {
    KeepGoing,
    Stop,
}

fn check_expr<'a, 'tcx>(vis: &mut ReadVisitor<'a, 'tcx>, expr: &'tcx Expr) -> StopEarly {
    if expr.id == vis.last_expr.id {
        return StopEarly::KeepGoing;
    }

    match expr.node {
        ExprArray(_) |
        ExprTup(_) |
        ExprMethodCall(_, _, _) |
        ExprCall(_, _) |
        ExprAssign(_, _) |
        ExprIndex(_, _) |
        ExprRepeat(_, _) |
        ExprStruct(_, _, _) => {
            walk_expr(vis, expr);
        },
        ExprBinary(op, _, _) |
        ExprAssignOp(op, _, _) => {
            if op.node == BiAnd || op.node == BiOr {
                // x && y and x || y always evaluate x first, so these are
                // strictly sequenced.
            } else {
                walk_expr(vis, expr);
            }
        },
        ExprClosure(_, _, _, _) => {
            // Either
            //
            // * `var` is defined in the closure body, in which case we've
            //   reached the top of the enclosing function and can stop, or
            //
            // * `var` is captured by the closure, in which case, because
            //   evaluating a closure does not evaluate its body, we don't
            //   necessarily have a write, so we need to stop to avoid
            //   generating false positives.
            //
            // This is also the only place we need to stop early (grrr).
            return StopEarly::Stop;
        },
        // All other expressions either have only one child or strictly
        // sequence the evaluation order of their sub-expressions.
        _ => {},
    }

    vis.last_expr = expr;

    StopEarly::KeepGoing
}

fn check_stmt<'a, 'tcx>(vis: &mut ReadVisitor<'a, 'tcx>, stmt: &'tcx Stmt) -> StopEarly {
    match stmt.node {
        StmtExpr(ref expr, _) |
        StmtSemi(ref expr, _) => check_expr(vis, expr),
        StmtDecl(ref decl, _) => {
            // If the declaration is of a local variable, check its initializer
            // expression if it has one. Otherwise, keep going.
            let local = match decl.node {
                DeclLocal(ref local) => Some(local),
                _ => None,
            };
            local.and_then(|local| local.init.as_ref())
                .map_or(StopEarly::KeepGoing, |expr| check_expr(vis, expr))
        },
    }
}

/// A visitor that looks for reads from a variable.
struct ReadVisitor<'a, 'tcx: 'a> {
    cx: &'a LateContext<'a, 'tcx>,
    /// The id of the variable we're looking for.
    var: DefId,
    /// The expressions where the write to the variable occurred (for reporting
    /// in the lint).
    write_expr: &'tcx Expr,
    /// The last (highest in the AST) expression we've checked, so we know not
    /// to recheck it.
    last_expr: &'tcx Expr,
}

impl<'a, 'tcx> Visitor<'tcx> for ReadVisitor<'a, 'tcx> {
    fn visit_expr(&mut self, expr: &'tcx Expr) {
        if expr.id == self.last_expr.id {
            return;
        }

        match expr.node {
            ExprPath(ref qpath) => {
                if let QPath::Resolved(None, ref path) = *qpath {
                    if path.segments.len() == 1 && self.cx.tcx.tables().qpath_def(qpath, expr.id).def_id() == self.var {
                        if is_in_assignment_position(self.cx, expr) {
                            // This is a write, not a read.
                        } else {
                            span_note_and_lint(
                                self.cx,
                                EVAL_ORDER_DEPENDENCE,
                                expr.span,
                                "unsequenced read of a variable",
                                self.write_expr.span,
                                "whether read occurs before this write depends on evaluation order"
                            );
                        }
                    }
                }
            }
            // We're about to descend a closure. Since we don't know when (or
            // if) the closure will be evaluated, any reads in it might not
            // occur here (or ever). Like above, bail to avoid false positives.
            ExprClosure(_, _, _, _) |

            // We want to avoid a false positive when a variable name occurs
            // only to have its address taken, so we stop here. Technically,
            // this misses some weird cases, eg.
            //
            // ```rust
            // let mut x = 0;
            // let a = foo(&{x = 1; x}, x);
            // ```
            //
            // TODO: fix this
            ExprAddrOf(_, _) => {
                return;
            }
            _ => {}
        }

        walk_expr(self, expr);
    }
    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
        NestedVisitorMap::All(&self.cx.tcx.map)
    }
}

/// Returns true if `expr` is the LHS of an assignment, like `expr = ...`.
fn is_in_assignment_position(cx: &LateContext, expr: &Expr) -> bool {
    if let Some(parent) = get_parent_expr(cx, expr) {
        if let ExprAssign(ref lhs, _) = parent.node {
            return lhs.id == expr.id;
        }
    }
    false
}