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

Redefining built-in types does not work #20427

Closed
Marckvdv opened this Issue Jan 2, 2015 · 24 comments

Comments

Projects
None yet
7 participants
@Marckvdv
Copy link

Marckvdv commented Jan 2, 2015

The following code does not do what I expect it to do:

type int = i8;
println!("{}", std::mem::size_of::<int>()); // => prints 8, should be 1? Or shouldn't it compile at all?
@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 7, 2015

Nominating. Seems like this should be well-defined, though this behavior may be so useless there's no backwards compat risk.

@brson brson added the I-nominated label Jan 7, 2015

@brson brson added this to the 1.0 milestone Jan 15, 2015

@brson brson added P-backcompat-lang and removed I-nominated labels Jan 15, 2015

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 15, 2015

Could possibly do post-1.0 bugfix.

@brson brson added the E-easy label Jan 15, 2015

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 7, 2015

@brson
I'm trying to fix this.
It takes place because in resolve_type for paths built-in names are searched first and the user-defined names are searched last. If I switch the order, then a lot of code breaks, because in cases like

use std::usize;
fn f() -> usize {}

the name lookup rightfully resolves usize as the module std::usize, because types and modules share the namespace.
I'm continuing to study rustc_resolve for now to come up with a better solution, but would be glad to recieve any advice.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 7, 2015

So, the current algorithm is:

  • if built-in name is a full path (u8) then it is resolved as built-in type
  • if built-in name is a part of full path (segment1::u8::segment2) then it is processed as all other user-defined names

Modules, unlike most of other items, are not exposed to the discussed problem just because they are never full paths.

It is possible to prohibit the built-in names only for items that can be used as full paths and are searched in type namespace, but IMO the most drastic solution would be the most appropriate here - prohibit built-in names for everything except for modules. Reusing the names of built-in types is never a good idea and modules are just an unfortunate exception.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 7, 2015

/cc @aturon

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Feb 7, 2015

@petrochenkov

prohibit built-in names for everything except for modules

To be clear: you're saying that you would not be able to define types (or values?) with names matching any primitive type? But modules are OK?

That sounds potentially fine to me (and can always be relaxed later).

cc @nikomatsakis

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 8, 2015

To be clear: you're saying that you would not be able to define types (or values?) with names matching any primitive type? But modules are OK?

Yes, only module items will be able to have names matching built-in types.

petrochenkov added a commit to petrochenkov/rust that referenced this issue Feb 8, 2015

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 8, 2015

A more fine-grained prohibition turned out to be easier to implement.
See PR #22093

bors added a commit that referenced this issue Feb 12, 2015

Auto merge of #22093 - petrochenkov:builtin, r=pnkfelix
Names of structs, enums, traits, type aliases and type parameters (i.e. all identifiers that can be used as full paths in type position) are not allowed to match the names of primitive types.
See #20427 for more information.

This is a minor [breaking-change]

bors added a commit that referenced this issue Feb 12, 2015

Auto merge of #22093 - petrochenkov:builtin, r=pnkfelix
Names of structs, enums, traits, type aliases and type parameters (i.e. all identifiers that can be used as full paths in type position) are not allowed to match the names of primitive types.
See #20427 for more information.

This is a minor [breaking-change]

bors added a commit that referenced this issue Feb 12, 2015

Auto merge of #22093 - petrochenkov:builtin, r=pnkfelix
Names of structs, enums, traits, type aliases and type parameters (i.e. all identifiers that can be used as full paths in type position) are not allowed to match the names of primitive types.
See #20427 for more information.

This is a minor [breaking-change]

bors added a commit that referenced this issue Feb 13, 2015

Auto merge of #22093 - petrochenkov:builtin, r=pnkfelix
Names of structs, enums, traits, type aliases and type parameters (i.e. all identifiers that can be used as full paths in type position) are not allowed to match the names of primitive types.
See #20427 for more information.

This is a minor [breaking-change]

mdinger added a commit to mdinger/rust that referenced this issue Feb 14, 2015

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Feb 16, 2015

@petrochenkov Has this been resolved?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 16, 2015

@aturon
Yes.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Feb 16, 2015

Thanks @petrochenkov!

@aturon aturon closed this Feb 16, 2015

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Feb 16, 2015

The implemented solution is only a band-aid and doesn't play well with post-UFCS rules and needs.
I'll keep it as part of #22172, even though that means more work to support it.
I guess the error added by #22093 prevents backwards compatibility issues so the proper solution shouldn't be a breaking change.

@eddyb eddyb reopened this Feb 16, 2015

@eddyb eddyb removed the E-easy label Feb 16, 2015

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 16, 2015

@eddyb
I'm interested, how would the proper solution look like?

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Feb 16, 2015

I've tried to summarize some implications of UFCS, including this one, in a comment on the UFCS issue.
The proposed "rooting" of primitives in a library would allow them to be treated like any other type and they would be made available through the prelude.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Apr 2, 2015

Note to self: what is the status now?

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Apr 2, 2015

assigning to @nrc to understand better what work remains here and how this should be proritized.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Apr 8, 2015

To summarise the status: there is a fix (the issue was fixed not by allowing redefinition but preventing using the names of primitive types as the name of anything user-defined except for modules), however, there were then doubts that this fix may not play well with some of the consequences of UFCS, specifically how primitive types can be used as modules. There does now seem to be consensus that primitive types and their impls and modules are fixed by #23104 and so I don't believe there is anything else to do here.

@nrc nrc closed this Apr 8, 2015

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 16, 2015

How so? #23104 does not make use of associated constants (#23606 is still not merged yet) so there is a chance things like u32::MIN will slightly change in behavior in 1.0.
Will we have std::u32 or w/e stuck around forever, as an alias to the type? (allowing beta code to keep working without removing the potentially needless import)

The names of primitives cannot be considered a fixed set, so I still prefer a solution that explicitly puts them in the prelude - maybe the fix should be modified so it behaves like the primitives are always imported where the prelude would be, even if primitives aren't being rooted in libcore?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Sep 14, 2015

@eddyb
I think removing special treatment of primitive types from resolve is still (almost) doable.
I've tried to prototype the implementation and the main problem is not moving primitive names to the "prelude-like" state itself, but backward compatibility :(
bool and [i/u][8/16/32/64/size] can be migrated easily (inherent impls can have associated constants), but f[32/64], char and str have problems.
Modules f32::consts and f64::consts (http://doc.rust-lang.org/std/f64/#modules) can be emulated by inherent associated types f32::consts and f64::consts redirecting to actual private types, which in their turn have lots of associated constants. Unfortunately, inherent associated types are not even implemented yet.
Structs in modules std::char and std::str (http://doc.rust-lang.org/std/char/#structs, http://doc.rust-lang.org/std/str/#structs) can be emulated by inherent associated types too, but, again, inherent associated types are not implemented.
std::str is a final boss, it has the trait FromStr in it, it will require an associated trait to emulate and I've never even heard about associated traits being discussed.

How do you think, would it make sense to migrate only part of primitive types to the new name lookup rules and keep the old rules for str, char etc.?

cc #8995

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Sep 14, 2015

@petrochenkov Sadly, it's backwards incompatible to change modules with the same name as types to anything else.
That is due to use statements pointing directly at associated items (and FromStr, I guess).
You can have both and, if the module is not imported, the associated item would be used instead.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Sep 14, 2015

Ah, right, use std::str::FromStr breaks if FromStr is an associated item, but it's probably not a problem for integer types - no one will write something like use i32::SIZE; (the crater run will be needed anyway), so I'm interested in moving at least integer types to the prelude (or any other strategy improving the current situation backward compatibly or almost backward compatibly).

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Sep 14, 2015

@petrochenkov You can move all the named built-in types to the prelude while keeping the existing modules. It's not pretty, but it should work.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Sep 14, 2015

@eddyb

// Assume `str` is in the prelude
use std::str; // Module `str` shadows `str` from the prelude
fn f(arg: &str) {} // Stops compiling, huge breakage

Similar example with existing prelude type:
http://is.gd/jHruih

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Sep 14, 2015

@petrochenkov What if std::str is both the primitive type and the existing module?
EDIT: Looks like they conflict right now: http://is.gd/TSxgpu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.