diff --git a/src/consts.rs b/src/consts.rs new file mode 100644 index 00000000000..bb74b3c71d1 --- /dev/null +++ b/src/consts.rs @@ -0,0 +1,393 @@ +use rustc::lint::Context; +use rustc::middle::const_eval::lookup_const_by_id; +use rustc::middle::def::PathResolution; +use rustc::middle::def::Def::*; +use syntax::ast::*; +use syntax::ptr::P; +use std::rc::Rc; +use std::ops::Deref; +use self::ConstantVariant::*; +use self::FloatWidth::*; + +#[derive(PartialEq, Eq, Debug, Copy, Clone)] +pub enum FloatWidth { + Fw32, + Fw64, + FwAny +} + +impl From for FloatWidth { + fn from(ty: FloatTy) -> FloatWidth { + match ty { + TyF32 => Fw32, + TyF64 => Fw64, + } + } +} + +#[derive(PartialEq, Eq, Debug, Clone)] +pub struct Constant { + pub constant: ConstantVariant, + pub needed_resolution: bool +} + +impl Constant { + pub fn new(variant: ConstantVariant) -> Constant { + Constant { constant: variant, needed_resolution: false } + } + + pub fn new_resolved(variant: ConstantVariant) -> Constant { + Constant { constant: variant, needed_resolution: true } + } + + // convert this constant to a f64, if possible + pub fn as_float(&self) -> Option { + match &self.constant { + &ConstantByte(b) => Some(b as f64), + &ConstantFloat(ref s, _) => s.parse().ok(), + &ConstantInt(i, ty) => Some(if is_negative(ty) { + -(i as f64) } else { i as f64 }), + _ => None + } + } +} + +/// a Lit_-like enum to fold constant `Expr`s into +#[derive(PartialEq, Eq, Debug, Clone)] +pub enum ConstantVariant { + /// a String "abc" + ConstantStr(String, StrStyle), + /// a Binary String b"abc" + ConstantBinary(Rc>), + /// a single byte b'a' + ConstantByte(u8), + /// a single char 'a' + ConstantChar(char), + /// an integer + ConstantInt(u64, LitIntType), + /// a float with given type + ConstantFloat(String, FloatWidth), + /// true or false + ConstantBool(bool), + /// an array of constants + ConstantVec(Vec), + /// also an array, but with only one constant, repeated N times + ConstantRepeat(Box, usize), + /// a tuple of constants + ConstantTuple(Vec), +} + +impl ConstantVariant { + /// convert to u64 if possible + /// + /// # panics + /// + /// if the constant could not be converted to u64 losslessly + fn as_u64(&self) -> u64 { + if let &ConstantInt(val, _) = self { + val // TODO we may want to check the sign if any + } else { + panic!("Could not convert a {:?} to u64"); + } + } +} + +/// simple constant folding: Insert an expression, get a constant or none. +pub fn constant(cx: &Context, e: &Expr) -> Option { + match &e.node { + &ExprParen(ref inner) => constant(cx, inner), + &ExprPath(_, _) => fetch_path(cx, e), + &ExprBlock(ref block) => constant_block(cx, block), + &ExprIf(ref cond, ref then, ref otherwise) => + constant_if(cx, &*cond, &*then, &*otherwise), + &ExprLit(ref lit) => Some(lit_to_constant(&lit.node)), + &ExprVec(ref vec) => constant_vec(cx, &vec[..]), + &ExprTup(ref tup) => constant_tup(cx, &tup[..]), + &ExprRepeat(ref value, ref number) => + constant_binop_apply(cx, value, number,|v, n| + Some(ConstantRepeat(Box::new(v), n.as_u64() as usize))), + &ExprUnary(op, ref operand) => constant(cx, operand).and_then( + |o| match op { + UnNot => + if let ConstantBool(b) = o.constant { + Some(Constant{ + needed_resolution: o.needed_resolution, + constant: ConstantBool(!b), + }) + } else { None }, + UnNeg => constant_negate(o), + UnUniq | UnDeref => Some(o), + }), + &ExprBinary(op, ref left, ref right) => + constant_binop(cx, op, left, right), + //TODO: add other expressions + _ => None, + } +} + +fn lit_to_constant(lit: &Lit_) -> Constant { + match lit { + &LitStr(ref is, style) => + Constant::new(ConstantStr(is.to_string(), style)), + &LitBinary(ref blob) => Constant::new(ConstantBinary(blob.clone())), + &LitByte(b) => Constant::new(ConstantByte(b)), + &LitChar(c) => Constant::new(ConstantChar(c)), + &LitInt(value, ty) => Constant::new(ConstantInt(value, ty)), + &LitFloat(ref is, ty) => { + Constant::new(ConstantFloat(is.to_string(), ty.into())) + }, + &LitFloatUnsuffixed(ref is) => { + Constant::new(ConstantFloat(is.to_string(), FwAny)) + }, + &LitBool(b) => Constant::new(ConstantBool(b)), + } +} + +/// create `Some(ConstantVec(..))` of all constants, unless there is any +/// non-constant part +fn constant_vec + Sized>(cx: &Context, vec: &[E]) -> Option { + let mut parts = Vec::new(); + let mut resolved = false; + for opt_part in vec { + match constant(cx, opt_part) { + Some(p) => { + resolved |= (&p).needed_resolution; + parts.push(p) + }, + None => { return None; }, + } + } + Some(Constant { + constant: ConstantVec(parts), + needed_resolution: resolved + }) +} + +fn constant_tup + Sized>(cx: &Context, tup: &[E]) -> Option { + let mut parts = Vec::new(); + let mut resolved = false; + for opt_part in tup { + match constant(cx, opt_part) { + Some(p) => { + resolved |= (&p).needed_resolution; + parts.push(p) + }, + None => { return None; }, + } + } + Some(Constant { + constant: ConstantTuple(parts), + needed_resolution: resolved + }) +} + +/// lookup a possibly constant expression from a ExprPath +fn fetch_path(cx: &Context, e: &Expr) -> Option { + if let Some(&PathResolution { base_def: DefConst(id), ..}) = + cx.tcx.def_map.borrow().get(&e.id) { + lookup_const_by_id(cx.tcx, id, None).and_then( + |l| constant(cx, l).map(|c| Constant::new_resolved(c.constant))) + } else { None } +} + +/// A block can only yield a constant if it only has one constant expression +fn constant_block(cx: &Context, block: &Block) -> Option { + if block.stmts.is_empty() { + block.expr.as_ref().and_then(|b| constant(cx, &*b)) + } else { None } +} + +fn constant_if(cx: &Context, cond: &Expr, then: &Block, otherwise: + &Option>) -> Option { + if let Some(Constant{ constant: ConstantBool(b), needed_resolution: res }) = + constant(cx, cond) { + if b { + constant_block(cx, then) + } else { + otherwise.as_ref().and_then(|expr| constant(cx, &*expr)) + }.map(|part| + Constant { + constant: part.constant, + needed_resolution: res || part.needed_resolution, + }) + } else { None } +} + +fn constant_negate(o: Constant) -> Option { + Some(Constant{ + needed_resolution: o.needed_resolution, + constant: match o.constant { + ConstantInt(value, ty) => + ConstantInt(value, match ty { + SignedIntLit(ity, sign) => + SignedIntLit(ity, neg_sign(sign)), + UnsuffixedIntLit(sign) => UnsuffixedIntLit(neg_sign(sign)), + _ => { return None; }, + }), + ConstantFloat(is, ty) => + ConstantFloat(neg_float_str(is), ty), + _ => { return None; }, + } + }) +} + +fn neg_sign(s: Sign) -> Sign { + match s { + Sign::Plus => Sign::Minus, + Sign::Minus => Sign::Plus, + } +} + +fn neg_float_str(s: String) -> String { + if s.starts_with('-') { + s[1..].to_owned() + } else { + format!("-{}", &*s) + } +} + +/// is the given LitIntType negative? +/// +/// Examples +/// +/// ``` +/// assert!(is_negative(UnsuffixedIntLit(Minus))); +/// ``` +pub fn is_negative(ty: LitIntType) -> bool { + match ty { + SignedIntLit(_, sign) | UnsuffixedIntLit(sign) => sign == Minus, + UnsignedIntLit(_) => false, + } +} + +fn unify_int_type(l: LitIntType, r: LitIntType, s: Sign) -> Option { + match (l, r) { + (SignedIntLit(lty, _), SignedIntLit(rty, _)) => if lty == rty { + Some(SignedIntLit(lty, s)) } else { None }, + (UnsignedIntLit(lty), UnsignedIntLit(rty)) => + if s == Plus && lty == rty { + Some(UnsignedIntLit(lty)) + } else { None }, + (UnsuffixedIntLit(_), UnsuffixedIntLit(_)) => Some(UnsuffixedIntLit(s)), + (SignedIntLit(lty, _), UnsuffixedIntLit(_)) => Some(SignedIntLit(lty, s)), + (UnsignedIntLit(lty), UnsuffixedIntLit(rs)) => if rs == Plus { + Some(UnsignedIntLit(lty)) } else { None }, + (UnsuffixedIntLit(_), SignedIntLit(rty, _)) => Some(SignedIntLit(rty, s)), + (UnsuffixedIntLit(ls), UnsignedIntLit(rty)) => if ls == Plus { + Some(UnsignedIntLit(rty)) } else { None }, + _ => None, + } +} + +fn constant_binop(cx: &Context, op: BinOp, left: &Expr, right: &Expr) + -> Option { + match op.node { + BiAdd => constant_binop_apply(cx, left, right, |l, r| + match (l, r) { + (ConstantByte(l8), ConstantByte(r8)) => + l8.checked_add(r8).map(ConstantByte), + (ConstantInt(l64, lty), ConstantInt(r64, rty)) => { + let (ln, rn) = (is_negative(lty), is_negative(rty)); + if ln == rn { + unify_int_type(lty, rty, if ln { Minus } else { Plus }) + .and_then(|ty| l64.checked_add(r64).map( + |v| ConstantInt(v, ty))) + } else { + if ln { + add_neg_int(r64, rty, l64, lty) + } else { + add_neg_int(l64, lty, r64, rty) + } + } + }, + // TODO: float + _ => None + }), + BiSub => constant_binop_apply(cx, left, right, |l, r| + match (l, r) { + (ConstantByte(l8), ConstantByte(r8)) => if r8 > l8 { + None } else { Some(ConstantByte(l8 - r8)) }, + (ConstantInt(l64, lty), ConstantInt(r64, rty)) => { + let (ln, rn) = (is_negative(lty), is_negative(rty)); + match (ln, rn) { + (false, false) => sub_int(l64, lty, r64, rty, r64 > l64), + (true, true) => sub_int(l64, lty, r64, rty, l64 > r64), + (true, false) => unify_int_type(lty, rty, Minus) + .and_then(|ty| l64.checked_add(r64).map( + |v| ConstantInt(v, ty))), + (false, true) => unify_int_type(lty, rty, Plus) + .and_then(|ty| l64.checked_add(r64).map( + |v| ConstantInt(v, ty))), + } + }, + _ => None, + }), + //BiMul, + //BiDiv, + //BiRem, + BiAnd => constant_short_circuit(cx, left, right, false), + BiOr => constant_short_circuit(cx, left, right, true), + //BiBitXor, + //BiBitAnd, + //BiBitOr, + //BiShl, + //BiShr, + //BiEq, + //BiLt, + //BiLe, + //BiNe, + //BiGe, + //BiGt, + _ => None, + } +} + +fn add_neg_int(pos: u64, pty: LitIntType, neg: u64, nty: LitIntType) -> + Option { + if neg > pos { + unify_int_type(nty, pty, Minus).map(|ty| ConstantInt(neg - pos, ty)) + } else { + unify_int_type(nty, pty, Plus).map(|ty| ConstantInt(pos - neg, ty)) + } +} + +fn sub_int(l: u64, lty: LitIntType, r: u64, rty: LitIntType, neg: bool) -> + Option { + unify_int_type(lty, rty, if neg { Minus } else { Plus }).and_then( + |ty| l.checked_sub(r).map(|v| ConstantInt(v, ty))) +} + +fn constant_binop_apply(cx: &Context, left: &Expr, right: &Expr, op: F) + -> Option +where F: Fn(ConstantVariant, ConstantVariant) -> Option { + if let (Some(Constant { constant: lc, needed_resolution: ln }), + Some(Constant { constant: rc, needed_resolution: rn })) = + (constant(cx, left), constant(cx, right)) { + op(lc, rc).map(|c| + Constant { + needed_resolution: ln || rn, + constant: c, + }) + } else { None } +} + +fn constant_short_circuit(cx: &Context, left: &Expr, right: &Expr, b: bool) -> + Option { + constant(cx, left).and_then(|left| + if let &ConstantBool(lbool) = &left.constant { + if lbool == b { + Some(left) + } else { + constant(cx, right).and_then(|right| + if let ConstantBool(_) = right.constant { + Some(Constant { + constant: right.constant, + needed_resolution: left.needed_resolution || + right.needed_resolution, + }) + } else { None } + ) + } + } else { None } + ) +} diff --git a/src/identity_op.rs b/src/identity_op.rs index 18a475bb737..a429d42c4cc 100644 --- a/src/identity_op.rs +++ b/src/identity_op.rs @@ -4,6 +4,8 @@ use rustc::middle::def::*; use syntax::ast::*; use syntax::codemap::Span; +use consts::{constant, Constant, is_negative}; +use consts::ConstantVariant::ConstantInt; use utils::{span_lint, snippet}; declare_lint! { pub IDENTITY_OP, Warn, @@ -44,35 +46,19 @@ impl LintPass for IdentityOp { fn check(cx: &Context, e: &Expr, m: i8, span: Span, arg: Span) { - if have_lit(cx, e, m) { - span_lint(cx, IDENTITY_OP, span, &format!( - "the operation is ineffective. Consider reducing it to `{}`", - snippet(cx, arg, ".."))); - } -} - -fn have_lit(cx: &Context, e : &Expr, m: i8) -> bool { - match &e.node { - &ExprUnary(UnNeg, ref litexp) => have_lit(cx, litexp, -m), - &ExprLit(ref lit) => { - match (&lit.node, m) { - (&LitInt(0, _), 0) => true, - (&LitInt(1, SignedIntLit(_, Plus)), 1) => true, - (&LitInt(1, UnsuffixedIntLit(Plus)), 1) => true, - (&LitInt(1, SignedIntLit(_, Minus)), -1) => true, - (&LitInt(1, UnsuffixedIntLit(Minus)), -1) => true, - _ => false - } - }, - &ExprParen(ref p) => have_lit(cx, p, m), - &ExprPath(_, _) => { - match cx.tcx.def_map.borrow().get(&e.id) { - Some(&PathResolution { base_def: DefConst(id), ..}) => - lookup_const_by_id(cx.tcx, id, Option::None) - .map_or(false, |l| have_lit(cx, l, m)), - _ => false - } - }, - _ => false + if let Some(c) = constant(cx, e) { + if c.needed_resolution { return; } // skip linting w/ lookup for now + if let ConstantInt(v, ty) = c.constant { + if match m { + 0 => v == 0, + -1 => is_negative(ty), + 1 => !is_negative(ty), + _ => unreachable!(), + } { + span_lint(cx, IDENTITY_OP, span, &format!( + "the operation is ineffective. Consider reducing it to `{}`", + snippet(cx, arg, ".."))); + } + } } } diff --git a/src/lib.rs b/src/lib.rs index 4c0617b1696..d7a10562df1 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,6 +16,7 @@ use rustc::lint::LintPassObject; #[macro_use] pub mod utils; +pub mod consts; pub mod types; pub mod misc; pub mod eq_op; diff --git a/tests/compile-fail/identity_op.rs b/tests/compile-fail/identity_op.rs index 987bada2ece..18e683e8a9e 100755 --- a/tests/compile-fail/identity_op.rs +++ b/tests/compile-fail/identity_op.rs @@ -11,14 +11,14 @@ fn main() { x + 0; //~ERROR the operation is ineffective 0 + x; //~ERROR the operation is ineffective - x - ZERO; //~ERROR the operation is ineffective + x - ZERO; //no error, as we skip lookups (for now) x | (0); //~ERROR the operation is ineffective - ((ZERO)) | x; //~ERROR the operation is ineffective + ((ZERO)) | x; //no error, as we skip lookups (for now) x * 1; //~ERROR the operation is ineffective 1 * x; //~ERROR the operation is ineffective - x / ONE; //~ERROR the operation is ineffective + x / ONE; //no error, as we skip lookups (for now) - x & NEG_ONE; //~ERROR the operation is ineffective + x & NEG_ONE; //no error, as we skip lookups (for now) -1 & x; //~ERROR the operation is ineffective } diff --git a/tests/compile-fail/precedence.rs b/tests/compile-fail/precedence.rs index 4d57f119479..ebb25f61b75 100755 --- a/tests/compile-fail/precedence.rs +++ b/tests/compile-fail/precedence.rs @@ -2,6 +2,7 @@ #![plugin(clippy)] #[deny(precedence)] +#[allow(identity_op)] #[allow(eq_op)] fn main() { format!("{} vs. {}", 1 << 2 + 3, (1 << 2) + 3); //~ERROR operator precedence can trip diff --git a/tests/consts.rs b/tests/consts.rs new file mode 100644 index 00000000000..db309952be4 --- /dev/null +++ b/tests/consts.rs @@ -0,0 +1,34 @@ +#![allow(plugin_as_library)] +#![feature(rustc_private)] + +extern crate clippy; +extern crate syntax; +extern crate rustc; + +use clippy::consts::constant; +use clippy::consts::ConstantVariant::*; +use syntax::ast::*; +use syntax::ptr::P; +use syntax::codemap::{Spanned, COMMAND_LINE_SP}; +use std::mem; +use rustc::lint::Context; + +fn ctx() -> &'static Context<'static, 'static> { + unsafe { + let x : *const Context<'static, 'static> = std::ptr::null(); + mem::transmute(x) + } +} + +#[test] +fn test_lit() { + assert_eq!(Some(ConstantBool(true)), constant(ctx(), + &Expr{ + id: 1, + node: ExprLit(P(Spanned{ + node: LitBool(true), + span: COMMAND_LINE_SP, + })), + span: COMMAND_LINE_SP, + }).map(|x| x.constant)); +}