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

Make size an associated constant #1168

Closed
wants to merge 2 commits into from

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Jun 21, 2015

(rendered)

Personally I'd prefer lower case names such as size and alignment for these associated constants, but I guess upper case is more in line with other Rust guidelines.

@alexcrichton alexcrichton added T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. labels Jun 22, 2015
@oli-obk
Copy link
Contributor

oli-obk commented Jun 23, 2015

I definitely want this, but the implementation is non-trivial, since you need to call into llvm to obtain this value. That is usually only done in trans, but for this to be a constant it needs to be available much earlier.

@bstrie
Copy link
Contributor

bstrie commented Jun 24, 2015

This is super wacky and I think I love it. It would also let us ditch the sizeof and alignof keywords that we have reserved.

@DanielKeep
Copy link

I love this; it's so obvious in hindsight... but I have one concern: having these on every type seems a little out of proportion to how often this information would presumably be needed. I'm still marginally in favour of const functions for that reason.

@diwic
Copy link

diwic commented Jun 24, 2015

Functions don't have to be freestanding - maybe the .size_of() and align_of() functions could be methods on the Sized trait?

@diwic
Copy link

diwic commented Jun 24, 2015

But then maybe associated constants would make more constant expressions possible...

@DanielKeep
Copy link

@diwic My concern is that putting anything on the Sized trait makes it visible everywhere. Shoving in a dark corner of std::mem means you only see it if you need it. Making them associated functions doesn't change that.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 24, 2015

You can always define a trait MemInfo + impl<T: Sized> MemInfo for T that is in a dark corner of std::mem and exposes those associated constants.

@glaebhoerl
Copy link
Contributor

@DanielKeep I see your point, but for perspective, sizeof and alignof in C and C++ are also visible everywhere (even more everywhere), being keywords - and we have also reserved them. I do agree that, all else equal, it would be preferable to scope these more narrowly, but the fact that we've already reserved sizeof and alignof as keywords suggests that a more-global scope would not be particularly hard to swallow, either. (And all else is probably not equal: for example, there is the matter that the Sized trait feels like the natural, logical, intuitive, and rightful place for the size constant to belong, so at least these two things have to be weighed against each other.)

@eternaleye
Copy link

I find myself rather in love with this idea. It's elegant, simple, and just makes sense.

@quantheory
Copy link
Contributor

Note #1062, which would have to be accepted first (and in fact, that RFC mentions this very idea in one of the examples). I am generally in favor of this, modulo backwards compatibility issues. I have always though that size/alignment in bytes made more sense as associated consts than const functions.

@quantheory
Copy link
Contributor

Correction, #1062 does not actually have to be accepted for this, but if it isn't, you won't be able to use T::SIZE in a constant expression for a generic type T. And we will need a little work to get it in non-constant expressions, i.e. just to be able to use it in the same way that we use size_of::<T>() right now.

Further edit: This is orthogonal to the issue of having a const function versus an associated const, since in both cases the issue is actually the ability for the value of a constant expression to depend on a type parameter at all, regardless of the mechanism.

@bstrie
Copy link
Contributor

bstrie commented Jun 25, 2015

I wouldn't worry about a BC hazard here. I'd expect exactly 0% of stable Rust code to have a problem since associated constants are still unstable, and so there's no reason for anyone to be using ALL_CAPS_CASE for an associated item (let alone the specific identifiers SIZE and ALIGNMENT). Not that we shouldn't still run Crater to give us assurance.

@arielb1
Copy link
Contributor

arielb1 commented Jun 25, 2015

@quantheory

Allowing associated consts as ExprPath shouldn't have the #1062 problems.

@quantheory
Copy link
Contributor

@arielb1 I guess that this was what I was trying to clarify in my second comment. I don't think that the syntax really matters for the #1062 issue. If you have SIZE be an associated const, but you don't let it actually be used in constant expressions, then it's not fundamentally all that different from a size_of call right now.

But if you allow the size of a type parameter to be used in a constant expression (e.g. an array size), you will definitely hit the #1062 issue, regardless of whether you accomplish this by using an associated const or by making size_of a constant function. So in that sense, the issue of allowing the size to be queried in a constant expression at all is orthogonal to the interface design issue in this RFC (though important in its own right).

@pcwalton
Copy link
Contributor

This seems very nice, if we figure out the details around implementation.

@alexcrichton
Copy link
Member

This would be adding T::SIZE for all types that currently exist today, so I think it'd be also get a preliminary run through crater to see if there are any backwards-compatibility hazards with this.

@huonw
Copy link
Member

huonw commented Jun 30, 2015

Minor point: there are non-Sized types that have static alignment info, specifically I believe [T] is aligned like T. However, I don't think we handle this "properly" today (align_of::<[u8]>() doesn't work) so it doesn't seem so bad.

@retep998
Copy link
Member

retep998 commented Jul 1, 2015

@alexcrichton Couldn't someone just grab all the crates.io packages and grep for every instance of [^A-Za-z0-9_]SIZE[^A-Za-z0-9_]?

@eddyb
Copy link
Member

eddyb commented Jul 1, 2015

I would prefer making size_of and align_of the stable interface, even if associated constants are being used under the hood, but either way, it's great to move forward on this.

I definitely want this, but the implementation is non-trivial, since you need to call into llvm to obtain this value.

Not entirely accurate. We already do a lot of size/alignment computations ourselves for aggregates; the algorithm for structs is straight-forward and enums get their LLVM type after variants have been laid out.
What we're missing right now is querying the size and alignment for primitives - those are encoded in the datalayout string.

A big chunk of trans::adt would have to be moved to librustc for this to work; IMO that's going to help with a back-end agnostic specification (though not necessarily a stable ABI).

@eternaleye
Copy link

@huonw, are there any types which have a size but no alignment? Off the top of my head I don't think so, and I wonder if an Aligned OIBIT that Sized derives from would be possible, i.e.

trait Aligned { const ALIGNMENT: usize; }
trait Sized: Aligned { const SIZE: usize; }

I don't think that would break any existing code, though I could be wrong.

EDIT: In the long run, that might be useful for informing the compiler of alignment constraints on a type, but I'm not suggesting that be supported at the moment.

@eddyb
Copy link
Member

eddyb commented Jul 1, 2015

@eternaleye it wouldn't need to be OIBIT, it could be implemented by the compiler for all sized types (and also [T] where T: Aligned).

I should also mention that lifting layout computation up from trans would allow us to operate with less known information, e.g. <*mut T>::SIZE is the same for all T: Sized.
This could be used in transmute checking to allow more cases than today and/or perform all checking before trans.

@nikomatsakis
Copy link
Contributor

We discussed in the language design subteam meeting and decided that while we like this idea, it is premature, given the current state of associated constants. Therefore, we are going to close the RFC for the time being, and revisit this approach once associated constants are in better shape. Thanks very much for the writeup!

@bstrie
Copy link
Contributor

bstrie commented Jun 23, 2017

Greetings from the future! Associated consts are on the verge of being stabilized ( rust-lang/rust#29646 ), likely landing in 1.20, so as per @nikomatsakis' comment I think it may be time to un-postpone this RFC.

@eddyb re: "I would prefer making size_of and align_of the stable interface, even if associated constants are being used under the hood", do you still hold this opinion? IMO, mem::size_of::<Foo>() just looks plain less nice than Foo::SIZE.

@cramertj
Copy link
Member

@bstrie As @alexcrichton mentioned above, I'm not sure we could do that backwards compatibly-- there are almost certainly types with methods called SIZE, and AFAIK associated consts and associated fns share the same namespace.

@bstrie
Copy link
Contributor

bstrie commented Jun 23, 2017

@cramertj I'll dispute the "almost certainly" claim, I think it's vanishingly unlikely that we'll find any types that defy convention (in defiance of the default-on style lint) to give themselves an all-uppercase method name like that. Of course, I still welcome a crater run to determine this (or are we on cargobomb by now?).

That said, if we do want to ever do this, it's true that we should take action to reserve the names before associated consts reach the stable channel.

@cramertj
Copy link
Member

@bstrie yeah, we can certainly check. I was surprised that someone had a struct named catch, but, well...

If crater is clean, we can probably just do a warning cycle. Still, I personally prefer the mem::size_of route, since it seems a little less magic to me.

@Stebalien
Copy link
Contributor

You can always write <Foo as Sized>::SIZE if you prefer (and, IMO, most unsafe code should write it this way so that inference changes don't lead to UB).

@eddyb
Copy link
Member

eddyb commented Jun 23, 2017

You can implement the associated constant version, in a library, using the intrinsic.
I prefer size_of because it's used everywhere already and some of that code can become CTFE-enabled.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 23, 2017

One issue with making it a constant is that changing the size of an object becomes a breaking change, because people might code things in constants like arr[5 - sizeof::<T>()] where arr: [U; 3], which would compile as long as T is in 3..6.

@eddyb
Copy link
Member

eddyb commented Jun 23, 2017

You can do that with transmute already though.

@bstrie
Copy link
Contributor

bstrie commented Jun 23, 2017

@cramertj size_of also seemed less magical to me at first, but now I've reconsidered. My new opinion is that, since size is a static property of a type (which is itself a notion that only exists statically), it makes more sense to present it as a constant property rather than something computed by a function (since my normal intuition is that a function is something that ought to have something to do dynamically, but size_of never can). I'd still prefer a size_of function to the sizeof keyword for the sake of keeping the language small, but I want to resist the notion that size_of should be preferred simply because "that's how we've always done it".

@bstrie
Copy link
Contributor

bstrie commented Jun 23, 2017

@eddyb re: "I prefer size_of because it's used everywhere already and some of that code can become CTFE-enabled.", but const fn is nowhere near stable, and associated consts are being stabilized as we speak. With this RFC, size would be available in const contexts immediately. Or are we intending to allow the stable channel to const-evaluate while disallowing stable users from defining const functions; if so, then what's the timeframe on that?

@droundy
Copy link

droundy commented Jun 24, 2017

Regarding @oli-obk 's good point about changing size becoming a breaking change, perhaps the size should not be exported for structs that have private fields?

I guess changing a size is sort-of already a breaking change, since other crates can see the size at run time. It just can't currently cause compilation to fail...

@Stebalien
Copy link
Contributor

Or just document it? At some point, every change is a breaking change...

@droundy
Copy link

droundy commented Jun 24, 2017

Yeah, the more I think about it, the more I think that you really don't want to prevent code from creating types based on the size of anything, and code that breaks when the size changes is broken code. After all, the point of using the const sizes is to properly handle the case when there size changes!

@est31
Copy link
Member

est31 commented Jun 30, 2017

I'm strongly against polluting the namespace of every single type with something that is mostly useful in a subset of contexts (FFI most likely). I personally never had to use std::mem::size_of.

With the change applied, in IDE's, SIZE (or size) would always show up if you expanded the methods that you can call on a type. It would always be part of rustdoc output. Unless of course, you did the choice to leave it out of rustdoc and out of IDE's, but IMO that would be a bad idea as well.

So while I'm not against the general idea of having an associated constant (I understand why it can be seen as advantageous over a const fn), can this be be opt in at usage site? As in, you have to use std::mem::MemInfo; or use std::mem::SizeOf; or something similar.

@jplatte
Copy link
Contributor

jplatte commented Jul 15, 2017

Or are we intending to allow the stable channel to const-evaluate while disallowing stable users from defining const functions; if so, then what's the timeframe on that?

Yes, that it what is probably going to happen: rust-lang/rust#43017. Not sure about the timeframe, there is still some discussion going on about whether all const fns currently available in std should have their constness stabilized at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.