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

resolve: Bind primitive types to items in libcore #32274

Closed
wants to merge 5 commits into from

Conversation

petrochenkov
Copy link
Contributor

This is a more radical alternative to #32131
A new attribute #[primitive_type] is introduced. If path used in type position resolves to a #[primitive_type] item, the definition of a primitive type with the same name is used instead of this item.
A path cannot resolve to a primitive type in any other way. Several different #[primitive_type] items can be resolved to one primitive type.
Most of primitive types are bound to modules (core::u8, core::str etc.) for backward compatibility.
All the primitive type items are added to the prelude.

Pros:
This makes primitive types appear to be less special and more similar to lang items - they are defined in libcore (and also libcollections, librustc_unicode and libstd), shadowed by other items (including modules) and need to be imported before use (but prelude does it for you).

Cons:
It doesn't makes name resolution simpler internally, primitive types are still a hardcoded limited set of Def::PrimTys known to the compiler.
It puts the burden of defining/importing primitive types on #[no_core] and #[no_implicit_prelude] code. The alternative PR presents them as the second small prelude which is hardcoded and has lower priority, so the primitive types are always available.

[breaking-change]
Needs crater before proceeding.

cc #32131 (comment)
cc @jseyfried
r? @eddyb

@eddyb
Copy link
Member

eddyb commented Mar 15, 2016

Why not #[lang = "..."]?

@eddyb eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Mar 15, 2016
@petrochenkov
Copy link
Contributor Author

Lang items have to be unique, no?
str, for example, has to be bound to both core::str and collections::str for backward compatibility. The same is true for char, f32, f64.

@eddyb
Copy link
Member

eddyb commented Mar 15, 2016

(and also libcollections, librustc_unicode and libstd)

I would still allow the impls to be in different crates (if I'm understanding this right) but move the types in libcore.

@petrochenkov
Copy link
Contributor Author

The problem is not in impls, but in shadowing.
Neither use core::str; nor use std::str; (they are two different modules) should make primitive str unavailable.
If we mark only core::str with #[primitive_type], then use std::str; will break our code using str.
If we mark only std::str with #[primitive_type], then we won't be able to use str in libcore at all.

@eddyb
Copy link
Member

eddyb commented Mar 15, 2016

@petrochenkov What's in collections::str that can't be in libcore? I guess this is the other thing aside from impls - modules that should've been replaced the moment UFCS was implemented.
Sadly we couldn't do it for constants at the time so we ended up leaving functions in modules too.

@petrochenkov
Copy link
Contributor Author

What's in collections::str that can't be in libcore?

Hm, only EncodeUtf16 stabilized in not yet released 1.8.0. There's still chance to move it.
std::f32 and std::f64 contain only reexports from the libcore modules.
rustc_unicode::char contains two stable structs though.

@eddyb
Copy link
Member

eddyb commented Mar 15, 2016

@petrochenkov Even if stabilized, couldn't move everything but char's two structures? And char is only an issue because it depends on machinery in librustc_unicode, right?

@petrochenkov
Copy link
Contributor Author

EncodeUtf16 can't be moved to libcore too if stabilized, it depends on librustc_unicode as well.
(If I correctly understood your question)

@eddyb
Copy link
Member

eddyb commented Mar 15, 2016

@petrochenkov Why do all of these things depend on an unicode library that we don't want to have in libcore for bloat reasons? 😭

// Check if the resolution is a #[primitive_type] item in type namespace,
// replace that item with a primitive type with the same name.
if let Some(PathResolution{base_def, ..}) = resolution {
if let Some(def_id) = base_def.opt_def_id() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to check for Def::Err explicitly instead of introducing the method opt_def_id.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Def::Err and Def::SelfTy, but anyway, def kind doesn't matter here, only the presence of def_id.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

@jseyfried
Copy link
Contributor

Reviewed, looks good to me.

@petrochenkov
Copy link
Contributor Author

(The failure caught by Travis happens only during docs generation for libc, it doesn't prevent running crater, I'll fix it later.)

@DemiMarie
Copy link
Contributor

Could libstd et al pub use the libcore export of the primitive type?

@eddyb
Copy link
Member

eddyb commented Mar 16, 2016

@drbo I believe the issue is that libcore doesn't have everything about all of those types.

@petrochenkov
Copy link
Contributor Author

I know there are plans to make a snapshot in the near future, it would be ideal to land this PR before that snapshot.

@alexcrichton
Copy link
Member

eh we're still in "make snapshots whenever" mode so it's not like we can't have one for another 6 months :)

I went ahead and kicked one off as @eddyb requested it, but if this lands we can just make a new one.

@bors
Copy link
Contributor

bors commented Mar 21, 2016

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

@petrochenkov
Copy link
Contributor Author

Rebased.

@bors
Copy link
Contributor

bors commented Mar 21, 2016

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

@petrochenkov
Copy link
Contributor Author

Rebased.

@petrochenkov
Copy link
Contributor Author

Closing as per #32131 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-crater Status: Waiting on a crater run to be completed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants