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.

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

Request to add a warning for large type sizes #6600

Closed
mstewartgallus opened this issue May 19, 2013 · 8 comments
Closed

Request to add a warning for large type sizes #6600

mstewartgallus opened this issue May 19, 2013 · 8 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@mstewartgallus
Copy link
Contributor

Many times it easy to accidentally create types that are many bytes in size. These structures are of course expensive to copy, and move around. Because there is already an option to check the size of structures in rustc (-Z count-type-sizes) it'd be nice to have a warning if some the sizes of some data structures are suspiciously large. I'd greatly appreciate this "-W large-type-sizes" feature as it'd reduce the chances of accidentally creating slow programs with silly mistakes, and already has a lot of the work done towards it implemented. Furthermore, giant know everything objects are usually a sign of a badly factored design.

As well, it might be useful to have a warning for medium sized data types that have sizes that are powers of two as they can have weird, and pathological worst-case effects on CPU caches (I don't fully understand exactly how they effect the cache so I'd appreciate if someone with a more detailed knowledge of cache effects can comment on whether this is a good idea.)

Basically, I think that as long as the type size information is already being collected it should start being used.

@huonw
Copy link
Member

huonw commented May 19, 2013

An additional lint in this vein would be one that warns about implicit copies of large types. E.g. the following compiles without a hitch:

type V = [int, .. 10000];

fn main() {
    let x: V = [0, .. 10000];

    let y = x, z = x;

    println(fmt!("%? %? %?", x, y, z))
}

and generates a memcpy for each of y and z.

(This would be a lint separate to one that warns about defining large types.)

@graydon
Copy link
Contributor

graydon commented Jul 11, 2013

Agreed this would be nice. Tagging but not nominating, I don't think we should block on it.

@huonw
Copy link
Member

huonw commented Jul 11, 2013

Another additional lint would be warning about enums where some variants are much larger than others, since the size of the whole enum is the size of the largest variant and so variants with very different sizes waste space. (It could suggest it be broken out and placed behind an ~.) E.g.

enum Foo { // "warning: mismatched variant sizes"
   A,
   B(int, int, int, int, int, int, int, int) // "note: this variant is much larger than the others"
}

// could become

struct BInner(int, int, int, int, int, int, int, int);
enum Foo {
   A,
   B(~BInner)
}

(This one should probably be allow by default.)

@emberian
Copy link
Member

emberian commented Aug 1, 2013

I started looking into implementing the large-type lint and it is easy: it's just reporting that's hard. Because trans runs after lint, lints can't use trans info. Some reworking needs to be done to keep the lint Context around and be able to register a set of lints specifically for trans.

From there, it's just iterating over the tydescs hashmap in the CrateContext, checking if the size is greater than some epsilon, and looking through the ty::t's sty, pulling out the span for the given type, and emitting the warning.

Linting for enum variants or linting on implicit copies seems to be a lot more difficult/non-obvious.

@alexcrichton
Copy link
Member

@cmr the lint pass could return a HashMap<ast::node_id, level> where each type declaration in the crate is a key of the hash map and the value is the level of the relevant lint at that time. Over time if we needed to give more lint levels to trans then the value could be a struct.

Then you just have to pass the map to trans which can shove it into the CrateContext

@pnkfelix
Copy link
Member

visiting for (long overdue) triage. This is a good idea (though not a 1.0 blocker).

@emberian
Copy link
Member

There is now a variant-size-difference lint which does this just for enum variants. I am not sure this in particular belongs in rustc, especially now that we have loadable lints. The problem is, "how big is too big?".

I agree this is nice. But it belongs in a separate analysis tool, not rustc. Anyone object to closing?

@thestinger
Copy link
Contributor

@cmr: Closing sounds fine. I don't think this is suitable for an official lint, because there's nothing wrong with having huge types like the rand crate does. It does need to be something you keep in mind while using them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

7 participants