Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Assist/Codelens to show size of type #4091

Closed
TimoFreiberg opened this issue Apr 22, 2020 · 28 comments
Closed

Assist/Codelens to show size of type #4091

TimoFreiberg opened this issue Apr 22, 2020 · 28 comments
Labels
A-ide general IDE features C-feature Category: feature request

Comments

@TimoFreiberg
Copy link
Contributor

Example:
()<|> -> shows 0
char<|> -> shows 4
struct Foo<|> { ... } shows whatever std::mem::size_of::<Foo>() returns

@bjorn3
Copy link
Member

bjorn3 commented Apr 22, 2020

This is going to be very hard to implement, as this basically requires a reimplementation of the layout code in rustc: https://github.com/rust-lang/rust/blob/8ce3f840ae9b735a66531996c32330f24b877cb0/src/librustc_middle/ty/layout.rs is 2776 lines long. It also depends on the exact version of rustc you are using, so it would have to be changed constantly.

@bjorn3 bjorn3 added the E-hard label Apr 22, 2020
@Diggsey
Copy link

Diggsey commented Apr 22, 2020

It also depends on the compilation target...

@TimoFreiberg
Copy link
Contributor Author

Maybe when the librarification progresses, rust-analyzer and rustc could use the same code?

@matklad
Copy link
Member

matklad commented Apr 23, 2020

Yeah, I feel like this is obviously something we should have. More deneraly, we should have an intention to see the whole layout. But it's also true that even MVP version of this would required a lot of progress on modularisation front

@lnicola lnicola added the S-unactionable Issue requires feedback, design decisions or is blocked on other work label Dec 23, 2020
@wrenger
Copy link

wrenger commented Jul 27, 2022

I do miss this a lot. Especially since clangd can show you the size of a struct and the size and offset of its fields.
Also cargo +nightly rustc -- -Zprint-type-sizes or const asserts with size_of are somewhat tedious...

@lnicola
Copy link
Member

lnicola commented Aug 8, 2022

#12972 is an interesting feature request: it asks for the type of each variant in an enum.

@oxalica
Copy link
Contributor

oxalica commented Aug 8, 2022

@bjorn3

This is going to be very hard to implement, as this basically requires a reimplementation of the layout code in rustc: [...] is 2776 lines long

If we only need the size, maintaining (align, size, niche) for each type should be enough. No need for the exact layout and arrangement. It would be much easier.

@bjorn3
Copy link
Member

bjorn3 commented Aug 8, 2022

If we only need the size, maintaining (align, size, niche) for each type should be enough. No need for the exact layout and arrangement. It would be much easier.

I don't think it would be much easier. Maybe a bit.

@kyrias
Copy link

kyrias commented Oct 13, 2022

This would be particularly useful for the implicit futures created by async functions, as per rust-lang/rust-clippy#5263.

bors added a commit that referenced this issue Dec 7, 2022
Compute data layout of types

cc #4091

Things that aren't working:
* Closures
* Generators (so no support for `Future` I think)
* Opaque types
* Type alias and associated types which may need normalization

Things that show wrong result:
* ~Enums with explicit discriminant~
* SIMD types
* ~`NonZero*` and similar standard library items which control layout with special attributes~

At the user level, I didn't put much work, since I wasn't confident about what is the best way to present this information. Currently it shows size and align for ADTs, and size, align, offset for struct fields, in the hover, similar to clangd. I used it some days and I feel I liked it, but we may consider it too noisy and move it to an assist or command.
@lnicola
Copy link
Member

lnicola commented Dec 12, 2022

Initial support added in #13490.

@jplatte
Copy link
Contributor

jplatte commented Dec 12, 2022

I find the comments inside the hovers a bit distracting, maybe they should be opt-in? It's information I need very rarely.

@flodiebold
Copy link
Member

We should make it configurable, but opt-out as usual.

@StripedMonkey
Copy link

Should this not be considered undefined for non-repr structs? I look at it as though this is leaking information that cannot/should not be depended on. If this information is shown, it should only be shown in contexts where the layout is actually defined.

This may be a pedantic or poor take, but I do have concerns about the idea of this information being used in a way that ignores the potential UB otherwise. It's happened before. Any instance in which you care about sizing/align should specify a layout anyways.

@lnicola
Copy link
Member

lnicola commented Dec 13, 2022

Someone messing with a sockaddr_in is not going to wait for the IDE to tell them how to do it. I think most people want this to have a rough idea of how large types are, to decide on moving vs. passing a mutable reference. Clippy even has a warning for it, and you could easily find dozens of cases of people boxing enum fields to reduce their size.

@NobodyXu
Copy link

Should this not be considered undefined for non-repr structs? I look at it as though this is leaking information that cannot/should not be depended on. If this information is shown, it should only be shown in contexts where the layout is actually defined.

This may be a pedantic or poor take, but I do have concerns about the idea of this information being used in a way that ignores the potential UB otherwise. It's happened before. Any instance in which you care about sizing/align should specify a layout anyways.

Yes, depending on the size/alignment is undefined behavior unless they are repr(C), but that could still be very useful.

For example, cargo_toml::Manifest on x86_64 is > 4000B and it's really hard to find this out just by reading the docs (because they don't document this).

I uses this information to optimize my code to extract information from Manifest and drop it before the next .await point.

I do not hard code the size/alignment information given by rustc/rust-analyzer in my code anyway and just use that to guide optimizations.

I think most people want this to have a rough idea of how large types are, to decide on moving vs. passing a mutable reference. Clippy even has a warning for it, and you could easily find dozens of cases of people boxing enum fields to reduce their size

That's exactly my use case, but clippy won't help a lot since cargo_toml::Manifest is an imported type.

Also, I'd like to know the size of the future returned by async fn/async {} and knowing the size will be super helpful.
I could box large variables that have to live past .await point, or boxing a specific Future if it gets way too large.

For examplereqwest::Pending was 2752B large if you enable brotli seanmonstar/reqwest#1681 and I only find this out after running RUSTFLAGS=-Zprint-type-size cargo b --release.

Having to run that every time to find out which types grow out of control is inefficient and I'd really like it to be shown in rust-analyzer by default, for variables and async fn/async {}.

@lnicola
Copy link
Member

lnicola commented Dec 13, 2022

That's exactly my use case, but clippy won't help a lot since cargo_toml::Manifest is an imported type.

RA won't help either, for the same reason :(.

@NobodyXu
Copy link

That's exactly my use case, but clippy won't help a lot since cargo_toml::Manifest is an imported type.

RA won't help either, for the same reason :(.

Well, can it print out size of the future generated by async fn/block?

@Veykril
Copy link
Member

Veykril commented Dec 13, 2022

No, as r-a doesn't support generator and closure captures yet

@NobodyXu
Copy link

No, as r-a doesn't support generator and closure captures yet

I would really love to see that.

@bjorn3
Copy link
Member

bjorn3 commented Dec 13, 2022

For example, cargo_toml::Manifest on x86_64 is > 4000B and it's really hard to find this out just by reading the docs (because they don't document this).

Nightly rustdoc has --show-type-layout. For an example of how this looks see https://doc.rust-lang.org/nightly/nightly-rustc/rustc_span/struct.Span.html#layout

@NobodyXu
Copy link

For example, cargo_toml::Manifest on x86_64 is > 4000B and it's really hard to find this out just by reading the docs (because they don't document this).

Nightly rustdoc has --show-type-layout. For an example of how this looks see https://doc.rust-lang.org/nightly/nightly-rustc/rustc_span/struct.Span.html#layout

Thanks, that will very helpful and I hope it gets turned on by default so that the size info will be accessible on docs.rs instead of requiring building doc on local.

Though I guess it won't show Future generated by async fn/async {}.

@StripedMonkey
Copy link

Someone messing with a sockaddr_in is not going to wait for the IDE to tell them how to do it.

Maybe. I'm not convinced someone throwing themselves in from a C background would say it's obvious non-repr structs don't have a defined memory layout. Missized allocations and naive assumptions about layout aren't exactly uncommon in the C (or cpp) world.

@lnicola
Copy link
Member

lnicola commented Dec 13, 2022

Missized allocations and naive assumptions about layout aren't exactly uncommon in the C (or cpp) world.

Exactly. And that's without the IDE showing them the type layout, so having this information available won't matter if you really insist on doing it.

But Rust developers need to use unsafe and generally know they'd need #[repr(C)] in this case. At least some of the cases you've pointed to were prompted by standard library API limitations.

@HKalbasi
Copy link
Member

Maybe we can add a (unstable) to message if the type is not recursively #[repr(C)]

@StripedMonkey
Copy link

Exactly. And that's without the IDE showing them the type layout, so having this information available won't matter if you really insist on doing it.

Rust is a language that tends to push you in safe directions, and the point that I had originally made was that very smart people made a very poor choice to rely on UB in memory layouts without anything suggesting that it was ok to do. Api limitations or not adding alignment and size information to something which is undefined makes things like my original post was referencing easier to stumble through or think is ok. Not every person using rust, even unsafe rust, knows or understands everything. Tools should point you in the right direction. Not just provide foot guns for people who don't know better.

Adding some annotation to indicate that the layout is unstable is a good suggestion, and would resolve my objections to putting it on non-repr structs.

@Veykril Veykril added C-feature Category: feature request A-ide general IDE features and removed E-hard S-unactionable Issue requires feedback, design decisions or is blocked on other work labels Jan 19, 2023
@HKalbasi
Copy link
Member

#12972 is an interesting feature request: it asks for the type of each variant in an enum.

Is this a well defined concept? In my understanding, layout of a variant has the same size as the layout of its enum, it just has more paddings.

@Veykril
Copy link
Member

Veykril commented May 18, 2023

Cargo has a lint that triggers when the biggest variant is a lot bigger than the second biggest. So the request there is really (I think), what is the variant's size disregarding it being an enum (so no tag, no padding from other variants, effectively handling it as a struct).

@HKalbasi
Copy link
Member

Layout of enum variant is implemented in #14845. I'm going to close this issue, feel free to open more specific ones if needed.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-ide general IDE features C-feature Category: feature request
Projects
None yet
Development

No branches or pull requests