Turn constants in modules core::[i/u][8/16/32/64/size] into associated constants #28656

petrochenkov · 2015-09-25T13:05:35Z

Module versions of the constants are left in place for both stability reasons and as an escape hatch for #28586

After this change (and after the resolution of #28586) primitive types [i/u][8/16/32/64/size] and bool can be put into the prelude and not treated specially in resolve (the same can't be done backward-compatibly with f[32/64], char and str, unfortunately).

rust-highfive · 2015-09-25T13:05:48Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

bluss · 2015-09-25T13:34:46Z

src/libcore/num/mod.rs

+        // FIXME(#11621): Should be deprecated once CTFE is implemented in favour of
+        // calling the `mem::size_of` function.
+        #[unstable(feature = "num_bits_bytes",
+                   reason = "may want to be an associated function",


reason needs update

Why? It still may want to be an associated function, just like std::usize::BYTES.

I believe this reason was written down long before we even had associated constants, but with those I don't think we'd want both a constant and a function, and in today's Rust these are definitely most appropriate as a constant.

bluss · 2015-09-25T13:35:13Z

Issue #28586 means this may cause ICEs (const is ok in array sizes but associated consts are not).

bluss · 2015-09-25T13:36:21Z

Needs [breaking-change] in commit or merge log.

petrochenkov · 2015-09-25T13:52:28Z

Issue #28586 means this may cause ICEs (const is ok in array sizes but associated consts are not).

That issue is interesting, because there is no associated constant BYTES in usize currently. But yeah, the ICE happens with existing associated constants too.

bluss · 2015-09-25T14:17:25Z

Maybe the regular const can stay (deprecated) until the issue is resolved? It may be an annoying limitation otherwise. I realize it's only unstable, but still.

petrochenkov · 2015-09-25T15:44:18Z

Yes, that would be reasonable. Updated.

bors · 2015-09-25T21:59:52Z

☔ The latest upstream changes (presumably #28610) made this pull request unmergeable. Please resolve the merge conflicts.

…d constants

petrochenkov · 2015-09-26T15:13:05Z

Rebased.

alexcrichton · 2015-09-28T17:03:23Z

src/librustdoc/html/render.rs

-        let &(ref fqp, short) = cache.paths.get(&did).unwrap();
-        if i > 0 {
-            try!(write!(&mut w, ","));
+        if let Some(&(ref fqp, short)) = cache.paths.get(&did) {


How come this was necessary? It seems like this may indicate a bug in rustdoc rather than something that should be ignored?

Yes, it's certainly a rustdoc bug, I spent may be a couple of hours trying to fix it, but haven't found out what's wrong.

alexcrichton · 2015-09-28T17:05:17Z

I'm also a little wary of deprecating an unstable feature in favor of something which may not work in all situations (e.g. those who migrate may end up hitting ICEs). I think it'd be fine to introduce these as associated constants, however in addition to leaving the previous const values there so we can get experience with both.

petrochenkov · 2015-09-28T17:35:08Z

Given that #28586 is a hard issue and is unlikely to be resolved soon, I'm ok with undeprecating.

petrochenkov · 2015-09-28T21:38:14Z

Updated.

alexcrichton · 2015-09-29T16:58:06Z

Thanks! Just a few more questions:

Could the rustdoc change be investigated and looked into? Rustdoc tends to always limp along but it's probably best if we at least don't knowingly do so!
What're the implications of having a stable associated constant? It's still unstable to use, right?
Could the unstable messages for the associated constants be updated? (as @bluss mentioned)

Needs [breaking-change] in commit or merge log.

@bluss could you elaborate on what the breaking change is? Was it the deprecation? On the surface this looks like a backwards-compatible change to me, but I haven't worked too closely with associated constants yet!

bluss · 2015-09-29T17:10:22Z

Don't think there is any breaking change anymore. It was the removal of the old unstable consts, that turned into deprecation and now no change.

bluss · 2015-09-29T17:11:39Z

That said I don't know how code will respond to std::usize; usize::BITS; where it could be both a const and an assoc. const.

petrochenkov · 2015-09-29T17:40:00Z

I don't know how code will respond to std::usize; usize::BITS; where it could be both a const and an assoc. const

It is resolved to std::usize::BITS in this case.

Could the rustdoc change be investigated and looked into?

If I could do it in reasonable time, I'd fix it, but I can't (as I said, I've already spent too much time on it). I can fill an issue and someone (maybe future me) will fix it later.

What're the implications of having a stable associated constant? It's still unstable to use, right?

I need to investigate.

petrochenkov · 2015-09-29T18:11:17Z

What're the implications of having a stable associated constant? It's still unstable to use, right?

Associated constants can be used on stable, but not defined.

alexcrichton · 2015-09-29T23:37:09Z

Hm ok, in that case I'd be more comfortable if these stayed as unstable for now to ensure we don't let usage accidentally leak in (I don't think we have many other stable associated constants).

Given that, though, can you remind me what the motivation for this change is? We'd definitely want to make the switch once associated constants are stable, but while they're in limbo we may want to be cautious about exposing and using them. You mention we can make some changes to resolve, but can you elaborate on that as well?

petrochenkov · 2015-09-30T12:20:14Z

@alexcrichton

You mention we can make some changes to resolve, but can you elaborate on that as well?

Primitive types are currently special-cased in resolve. In particular, this special casing implies weird shadowing rules. They are so weird they have to be guarded by a hack (#22093). This is all bad (rust-lang/rfcs#907 (comment)). Ideally, primitive types should live in the prelude and be resolved as all other types. If the new primitive types are added (f16, [u/i]128) they will have to be added this way. But it can't be done backward compatibly for existing types, because some people were careless enough to stabilize modules with names conflicting with primitive types and lots of various items in them. However, integer types can likely be recovered, because their respective modules contain only constants, and constants can ideally be replaced with associated constants and mod i32 {} can be replaced with type i32 = core::prelude::i32 (more details in #20427 (comment) and below). Unfortunately, associated constants can't be used in all places where free constants can be used, because they are resolved too late, and fixing it will probably require a major compiler overhaul.

Sooo... The only remaining motivation for this change is ability to use i32::MAX more conveniently without importing any modules.

alexcrichton · 2015-09-30T18:30:36Z

Hm ok, I must admit that I'm not very familiar with the resolve parts of this, but from what it sounds like the benefits of this may be pretty small by this point? If major overhauls are required before associated constants are usable like constants today it may be best to wait on this change?

petrochenkov · 2015-09-30T18:45:52Z

Ok, it's not urgent.

bluss · 2016-03-31T19:00:25Z

Can this be revived now?

petrochenkov · 2016-03-31T19:49:50Z

Can this be revived now?

Yes, I think. Associated constants are in much better state now and we have crater to check for breakage.

rust-highfive assigned alexcrichton Sep 25, 2015

bluss reviewed Sep 25, 2015
View reviewed changes

petrochenkov force-pushed the primod branch from 9be4f63 to 7792636 Compare September 25, 2015 15:43

petrochenkov added 2 commits September 26, 2015 17:03

Turn constants in modules core::[i/u][8,16,32,64,size] into associate…

3760d1d

…d constants

Workaround panic in rustdoc

2865d65

petrochenkov force-pushed the primod branch from 7792636 to e486076 Compare September 26, 2015 15:12

alexcrichton reviewed Sep 28, 2015
View reviewed changes

Restore unstable module level constants to avoid ICE rust-lang#28586

ad7a32a

petrochenkov force-pushed the primod branch from e486076 to ad7a32a Compare September 28, 2015 21:37

petrochenkov closed this Sep 30, 2015

petrochenkov deleted the primod branch November 22, 2015 11:43

petrochenkov mentioned this pull request Jan 11, 2018

Add lint to replace consts with const fns rust-lang/rust-clippy#2344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn constants in modules core::[i/u][8/16/32/64/size] into associated constants #28656

Turn constants in modules core::[i/u][8/16/32/64/size] into associated constants #28656

petrochenkov commented Sep 25, 2015

rust-highfive commented Sep 25, 2015

bluss Sep 25, 2015

petrochenkov Sep 29, 2015

alexcrichton Sep 29, 2015

bluss commented Sep 25, 2015

bluss commented Sep 25, 2015

petrochenkov commented Sep 25, 2015

bluss commented Sep 25, 2015

petrochenkov commented Sep 25, 2015

bors commented Sep 25, 2015

petrochenkov commented Sep 26, 2015

alexcrichton Sep 28, 2015

petrochenkov Sep 28, 2015

alexcrichton commented Sep 28, 2015

petrochenkov commented Sep 28, 2015

petrochenkov commented Sep 28, 2015

alexcrichton commented Sep 29, 2015

bluss commented Sep 29, 2015

bluss commented Sep 29, 2015

petrochenkov commented Sep 29, 2015

petrochenkov commented Sep 29, 2015

alexcrichton commented Sep 29, 2015

petrochenkov commented Sep 30, 2015

alexcrichton commented Sep 30, 2015

petrochenkov commented Sep 30, 2015

bluss commented Mar 31, 2016

petrochenkov commented Mar 31, 2016

Turn constants in modules core::[i/u][8/16/32/64/size] into associated constants #28656

Turn constants in modules core::[i/u][8/16/32/64/size] into associated constants #28656

Conversation

petrochenkov commented Sep 25, 2015

rust-highfive commented Sep 25, 2015

bluss Sep 25, 2015

Choose a reason for hiding this comment

petrochenkov Sep 29, 2015

Choose a reason for hiding this comment

alexcrichton Sep 29, 2015

Choose a reason for hiding this comment

bluss commented Sep 25, 2015

bluss commented Sep 25, 2015

petrochenkov commented Sep 25, 2015

bluss commented Sep 25, 2015

petrochenkov commented Sep 25, 2015

bors commented Sep 25, 2015

petrochenkov commented Sep 26, 2015

alexcrichton Sep 28, 2015

Choose a reason for hiding this comment

petrochenkov Sep 28, 2015

Choose a reason for hiding this comment

alexcrichton commented Sep 28, 2015

petrochenkov commented Sep 28, 2015

petrochenkov commented Sep 28, 2015

alexcrichton commented Sep 29, 2015

bluss commented Sep 29, 2015

bluss commented Sep 29, 2015

petrochenkov commented Sep 29, 2015

petrochenkov commented Sep 29, 2015

alexcrichton commented Sep 29, 2015

petrochenkov commented Sep 30, 2015

alexcrichton commented Sep 30, 2015

petrochenkov commented Sep 30, 2015

bluss commented Mar 31, 2016

petrochenkov commented Mar 31, 2016