From c327080ee04e641a34f30ae71b713a91106680b1 Mon Sep 17 00:00:00 2001 From: Corey Richardson Date: Mon, 19 May 2014 14:57:24 -0700 Subject: [PATCH] rustc: add a lint for large enum variants It can be easy to accidentally bloat the size of an enum by making one variant larger than the others. When this happens, it usually goes unnoticed. This commit adds a lint that can warn when the largest variant in an enum is more than 3 times larger than the second-largest variant. This requires a little bit of rejiggering, because size information is only available in trans, but lint levels are only available in the lint context. It is allow by default because it's pretty noisy, and isn't really *that* undesirable. Closes #10362 --- src/librustc/middle/lint.rs | 118 ++++++++++++++++-------- src/librustc/middle/trans/base.rs | 58 +++++++++++- src/librustc/middle/ty.rs | 4 + src/test/run-pass/enum-size-variance.rs | 42 +++++++++ 4 files changed, 181 insertions(+), 41 deletions(-) create mode 100644 src/test/run-pass/enum-size-variance.rs diff --git a/src/librustc/middle/lint.rs b/src/librustc/middle/lint.rs index 88ded969ad5d..7dc3db0a5d1a 100644 --- a/src/librustc/middle/lint.rs +++ b/src/librustc/middle/lint.rs @@ -72,7 +72,7 @@ use syntax::parse::token; use syntax::visit::Visitor; use syntax::{ast, ast_util, visit}; -#[deriving(Clone, Eq, Ord, TotalEq, TotalOrd)] +#[deriving(Clone, Show, Eq, Ord, TotalEq, TotalOrd)] pub enum Lint { CTypes, UnusedImports, @@ -93,6 +93,7 @@ pub enum Lint { UnknownFeatures, UnknownCrateType, UnsignedNegate, + VariantSizeDifference, ManagedHeapMemory, OwnedHeapMemory, @@ -146,8 +147,9 @@ pub struct LintSpec { pub type LintDict = HashMap<&'static str, LintSpec>; +// this is public for the lints that run in trans #[deriving(Eq)] -enum LintSource { +pub enum LintSource { Node(Span), Default, CommandLine @@ -399,6 +401,13 @@ static lint_table: &'static [(&'static str, LintSpec)] = &[ default: Warn }), + ("variant_size_difference", + LintSpec { + lint: VariantSizeDifference, + desc: "detects enums with widely varying variant sizes", + default: Allow, + }), + ("unused_must_use", LintSpec { lint: UnusedMustUse, @@ -461,6 +470,54 @@ struct Context<'a> { /// Ids of structs/enums which have been checked for raw_pointer_deriving checked_raw_pointers: NodeSet, + + /// Level of EnumSizeVariance lint for each enum, stored here because the + /// body of the lint needs to run in trans. + enum_levels: HashMap, +} + +pub fn emit_lint(level: Level, src: LintSource, msg: &str, span: Span, + lint_str: &str, tcx: &ty::ctxt) { + if level == Allow { return } + + let mut note = None; + let msg = match src { + Default => { + format!("{}, \\#[{}({})] on by default", msg, + level_to_str(level), lint_str) + }, + CommandLine => { + format!("{} [-{} {}]", msg, + match level { + Warn => 'W', Deny => 'D', Forbid => 'F', + Allow => fail!() + }, lint_str.replace("_", "-")) + }, + Node(src) => { + note = Some(src); + msg.to_str() + } + }; + + match level { + Warn => { tcx.sess.span_warn(span, msg.as_slice()); } + Deny | Forbid => { tcx.sess.span_err(span, msg.as_slice()); } + Allow => fail!(), + } + + for &span in note.iter() { + tcx.sess.span_note(span, "lint level defined here"); + } +} + +pub fn lint_to_str(lint: Lint) -> &'static str { + for &(name, lspec) in lint_table.iter() { + if lspec.lint == lint { + return name; + } + } + + fail!("unrecognized lint: {}", lint); } impl<'a> Context<'a> { @@ -492,7 +549,7 @@ impl<'a> Context<'a> { return *k; } } - fail!("unregistered lint {:?}", lint); + fail!("unregistered lint {}", lint); } fn span_lint(&self, lint: Lint, span: Span, msg: &str) { @@ -501,37 +558,8 @@ impl<'a> Context<'a> { Some(&(Warn, src)) => (self.get_level(Warnings), src), Some(&pair) => pair, }; - if level == Allow { return } - let mut note = None; - let msg = match src { - Default => { - format_strbuf!("{}, \\#[{}({})] on by default", - msg, - level_to_str(level), - self.lint_to_str(lint)) - }, - CommandLine => { - format!("{} [-{} {}]", msg, - match level { - Warn => 'W', Deny => 'D', Forbid => 'F', - Allow => fail!() - }, self.lint_to_str(lint).replace("_", "-")) - }, - Node(src) => { - note = Some(src); - msg.to_str() - } - }; - match level { - Warn => self.tcx.sess.span_warn(span, msg.as_slice()), - Deny | Forbid => self.tcx.sess.span_err(span, msg.as_slice()), - Allow => fail!(), - } - - for &span in note.iter() { - self.tcx.sess.span_note(span, "lint level defined here"); - } + emit_lint(level, src, msg, span, self.lint_to_str(lint), self.tcx); } /** @@ -1685,9 +1713,24 @@ fn check_stability(cx: &Context, e: &ast::Expr) { cx.span_lint(lint, e.span, msg.as_slice()); } +fn check_enum_variant_sizes(cx: &mut Context, it: &ast::Item) { + match it.node { + ast::ItemEnum(..) => { + match cx.cur.find(&(VariantSizeDifference as uint)) { + Some(&(lvl, src)) if lvl != Allow => { + cx.node_levels.insert((it.id, VariantSizeDifference), (lvl, src)); + }, + _ => { } + } + }, + _ => { } + } +} + impl<'a> Visitor<()> for Context<'a> { fn visit_item(&mut self, it: &ast::Item, _: ()) { self.with_lint_attrs(it.attrs.as_slice(), |cx| { + check_enum_variant_sizes(cx, it); check_item_ctypes(cx, it); check_item_non_camel_case_types(cx, it); check_item_non_uppercase_statics(cx, it); @@ -1878,6 +1921,7 @@ pub fn check_crate(tcx: &ty::ctxt, lint_stack: Vec::new(), negated_expr_id: -1, checked_raw_pointers: NodeSet::new(), + enum_levels: HashMap::new(), }; // Install default lint levels, followed by the command line levels, and @@ -1913,13 +1957,11 @@ pub fn check_crate(tcx: &ty::ctxt, // in the iteration code. for (id, v) in tcx.sess.lints.borrow().iter() { for &(lint, span, ref msg) in v.iter() { - tcx.sess.span_bug(span, - format!("unprocessed lint {:?} at {}: {}", - lint, - tcx.map.node_to_str(*id), - *msg).as_slice()) + tcx.sess.span_bug(span, format!("unprocessed lint {} at {}: {}", + lint, tcx.map.node_to_str(*id), *msg).as_slice()) } } tcx.sess.abort_if_errors(); + *tcx.enum_lint_levels.borrow_mut() = cx.enum_levels; } diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs index e208097e99b3..5f708b2b8bfc 100644 --- a/src/librustc/middle/trans/base.rs +++ b/src/librustc/middle/trans/base.rs @@ -36,6 +36,7 @@ use lib::llvm::{ModuleRef, ValueRef, BasicBlockRef}; use lib::llvm::{llvm, Vector}; use lib; use metadata::{csearch, encoder}; +use middle::lint; use middle::astencode; use middle::lang_items::{LangItem, ExchangeMallocFnLangItem, StartFnLangItem}; use middle::weak_lang_items; @@ -57,7 +58,7 @@ use middle::trans::foreign; use middle::trans::glue; use middle::trans::inline; use middle::trans::machine; -use middle::trans::machine::{llalign_of_min, llsize_of}; +use middle::trans::machine::{llalign_of_min, llsize_of, llsize_of_real}; use middle::trans::meth; use middle::trans::monomorphize; use middle::trans::tvec; @@ -1539,7 +1540,7 @@ fn trans_enum_variant_or_tuple_like_struct(ccx: &CrateContext, } fn trans_enum_def(ccx: &CrateContext, enum_definition: &ast::EnumDef, - id: ast::NodeId, vi: &[Rc], + sp: Span, id: ast::NodeId, vi: &[Rc], i: &mut uint) { for &variant in enum_definition.variants.iter() { let disr_val = vi[*i].disr_val; @@ -1559,6 +1560,57 @@ fn trans_enum_def(ccx: &CrateContext, enum_definition: &ast::EnumDef, } } } + + enum_variant_size_lint(ccx, enum_definition, sp, id); +} + +fn enum_variant_size_lint(ccx: &CrateContext, enum_def: &ast::EnumDef, sp: Span, id: ast::NodeId) { + let mut sizes = Vec::new(); // does no allocation if no pushes, thankfully + + let (lvl, src) = ccx.tcx.node_lint_levels.borrow() + .find(&(id, lint::VariantSizeDifference)) + .map_or((lint::Allow, lint::Default), |&(lvl,src)| (lvl, src)); + + if lvl != lint::Allow { + let avar = adt::represent_type(ccx, ty::node_id_to_type(ccx.tcx(), id)); + match *avar { + adt::General(_, ref variants) => { + for var in variants.iter() { + let mut size = 0; + for field in var.fields.iter().skip(1) { + // skip the dicriminant + size += llsize_of_real(ccx, sizing_type_of(ccx, *field)); + } + sizes.push(size); + } + }, + _ => { /* its size is either constant or unimportant */ } + } + + let (largest, slargest, largest_index) = sizes.iter().enumerate().fold((0, 0, 0), + |(l, s, li), (idx, &size)| + if size > l { + (size, l, idx) + } else if size > s { + (l, size, li) + } else { + (l, s, li) + } + ); + + // we only warn if the largest variant is at least thrice as large as + // the second-largest. + if largest > slargest * 3 && slargest > 0 { + lint::emit_lint(lvl, src, + format!("enum variant is more than three times larger \ + ({} bytes) than the next largest (ignoring padding)", + largest).as_slice(), + sp, lint::lint_to_str(lint::VariantSizeDifference), ccx.tcx()); + + ccx.sess().span_note(enum_def.variants.get(largest_index).span, + "this variant is the largest"); + } + } } pub struct TransItemVisitor<'a> { @@ -1605,7 +1657,7 @@ pub fn trans_item(ccx: &CrateContext, item: &ast::Item) { if !generics.is_type_parameterized() { let vi = ty::enum_variants(ccx.tcx(), local_def(item.id)); let mut i = 0; - trans_enum_def(ccx, enum_definition, item.id, vi.as_slice(), &mut i); + trans_enum_def(ccx, enum_definition, item.span, item.id, vi.as_slice(), &mut i); } } ast::ItemStatic(_, m, expr) => { diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index 62679fa222bf..1ce1eb0a82a6 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -14,6 +14,7 @@ use back::svh::Svh; use driver::session::Session; use metadata::csearch; use mc = middle::mem_categorization; +use middle::lint; use middle::const_eval; use middle::dependency_format; use middle::lang_items::{ExchangeHeapLangItem, OpaqueStructLangItem}; @@ -352,6 +353,8 @@ pub struct ctxt { pub vtable_map: typeck::vtable_map, pub dependency_formats: RefCell, + + pub enum_lint_levels: RefCell>, } pub enum tbox_flag { @@ -1134,6 +1137,7 @@ pub fn mk_ctxt(s: Session, method_map: RefCell::new(FnvHashMap::new()), vtable_map: RefCell::new(FnvHashMap::new()), dependency_formats: RefCell::new(HashMap::new()), + enum_lint_levels: RefCell::new(HashMap::new()), } } diff --git a/src/test/run-pass/enum-size-variance.rs b/src/test/run-pass/enum-size-variance.rs new file mode 100644 index 000000000000..39ab8316958a --- /dev/null +++ b/src/test/run-pass/enum-size-variance.rs @@ -0,0 +1,42 @@ +// Copyright 2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. +// +// ignore-pretty + +#![deny(enum_size_variance)] +#![allow(dead_code)] + +enum Enum1 { } + +enum Enum2 { A, B, C } + +enum Enum3 { D(int), E, F } + +enum Enum4 { H(int), I(int), J } + +enum Enum5 { //~ ERROR three times larger + L(int, int, int, int), //~ NOTE this variant is the largest + M(int), + N +} + +enum Enum6 { + O(T), + P(U), + Q(int) +} + +#[allow(enum_size_variance)] +enum Enum7 { + R(int, int, int, int), + S(int), + T +} +pub fn main() { }