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: Minimize hacks in name resolution of primitive types #32131

Merged
merged 3 commits into from
Mar 26, 2016

Conversation

petrochenkov
Copy link
Contributor

When resolving the first unqualified segment in a path with n segments and n - 1 associated item segments, e.g. (a or a::assoc or a::assoc::assoc etc) try to resolve a without considering primitive types first. If the "normal" lookup fails or results in a module, then try to resolve a as a primitive type as a fallback.

This way backward compatibility is respected, but the restriction from E0317 can be lifted, i.e. primitive names mostly can be shadowed like any other names.

Furthermore, if names of primitive types are put into prelude (now it's possible to do), then most of names will be resolved in conventional way and amount of code relying on this fallback will be greatly reduced. Although, it's not entirely convenient to put them into prelude right now due to temporary conflicts like use prelude::v1::*; use usize; in libcore/libstd, I'd better wait for proper glob shadowing before doing it.
I wish the no_prelude attribute were unstable as intended :(

cc @jseyfried @arielb1
r? @eddyb

@eddyb
Copy link
Member

eddyb commented Mar 8, 2016

LGTM, but I'd like to see a decision from @rust-lang/lang.

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

Looks good to me, +1 for this change.

I'm having trouble seeing why this isn't backwards compatible -- @petrochenkov can you come up with an example that breaks?

// use std::u8; // bring module u8 in scope
// fn f() -> u8 { // OK, resolves to primitive u8, not to std::u8
// u8::MAX // OK, resolves to associated constant <u8>::MAX,
// // not to non-existent std::u8::MAX
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this resolves to ::std::u8::MAX; MAX is not an associated const of <u8>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix the example

@petrochenkov
Copy link
Contributor Author

I'm having trouble seeing why this isn't backwards compatible

For some reason I thought extern crates have their own Def, but it turns out they are Def::Mod too and E0317 prevented any other breakage. So, yes, it looks like this change is completely backward compatible.

(The changes to prelude will require a crater run though, but I've not included them in this PR)

@jseyfried
Copy link
Contributor

this change is completely backward compatible

Excellent. I can't think of any other reason to continue disallowing types that would shadow primitive types.

The changes to prelude will require a crater run though, but I've not included them in this PR

The prelude is shadowable by everything else, so I think it would be backwards compatible to add to the prelude for all code that doesn't manually import the prelude.

@jseyfried
Copy link
Contributor

@petrochenkov What would you think about moving to disallow modules that shadow primitive types?

Then, once associated consts stabilize, we could make MAX and MIN associated constants of the type u8 (for example), make std::u8 the type instead of the module, and then stop special-casing primitive types relative to the rest of the prelude.

Without modules that shadow primitive types, I believe this change would be backwards compatible (except for use std::u8::MAX, unless we made associated consts importable).

@petrochenkov
Copy link
Contributor Author

backwards compatible ... for all code that doesn't manually import the prelude

#![no_implicit_prelude] not being feature gated is exactly the reason why it may be a breaking change for code on stable.

we could make MAX and MIN associated constants of the type u8 (for example), make std::u8 the type instead of the module, and then stop special-casing primitive types.

"Numeric" modules are more or less fixable if associated constants are improved, but we also have http://doc.rust-lang.org/nightly/std/str/...

@jseyfried
Copy link
Contributor

Ok, but #![no_implicit_prelude] is rarely used in stable Rust and on its way to deprecation.

str certainly doesn't look fixable before 2.0. I still think it might be a good idea to disallow modules that shadow the numeric primitive types.

@aturon aturon added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Mar 9, 2016
@aturon
Copy link
Member

aturon commented Mar 9, 2016

Nominated for discussion at next lang team meeting.

@nikomatsakis
Copy link
Contributor

On Wed, Mar 09, 2016 at 12:51:41AM -0800, Jeffrey Seyfried wrote:

Then, once associated consts stabilize, we could make MAX and MIN associated constants of the type u8 (for example) and make std::u8 the type instead of the module. Without modules that shadow primitive types, I believe this change would be backwards compatible (except for use std::u8::MAX, unless we made associated consts importable).

I do hope we will do this eventually; I think making imports of associated items work would be a pre-req to doing such a change.

@nikomatsakis
Copy link
Contributor

Is another way to view "no implicit prelude" as actually giving you a smaller prelude, that just contains the builtin numeric types?

Also, I think we could get away with retroactively feature-gating no-implicit-prelude, which seems like something we did not mean to stabilize.

@petrochenkov
Copy link
Contributor Author

@jseyfried

What would you think about moving to disallow modules that shadow primitive types?

Sorry, I haven't answered to this.
What is the purpose of this restriction? Reducing reliance of people on the fallback?
An experiment with crater wouldn't hurt, but I doubt this is a viable idea due to compatibility, I've definitely seen such modules used somewhere outside of the standard library.
I have a more targeted solution - a new unstable attribute #[this_module_permits_fallback_to_primitive_types]. This attribute could be applied (temporarily if possible) to several modules in the standard library (std::u8/std::str/etc), all other modules will shadow primitive types properly. It has the fewer compatibility problems and the same prerequisites:

  1. Primitive types should be available in qualified form, i.e. std::primitive::u8. This way they can be used when shadowed by a module (or anything else) + they can be used in use directives as a bonus.
mod S {}
type A = S;

should give an error message "S is a module and not a type" and not "type name S is undefined or not in scope" like now.

@petrochenkov
Copy link
Contributor Author

Also, I think we could get away with retroactively feature-gating no-implicit-prelude, which seems like something we did not mean to stabilize.

+1

Is another way to view "no implicit prelude" as actually giving you a smaller prelude, that just contains the builtin numeric types?

With prelude changes from #32167 the fallback implemented in this PR starts looking like kind of a small lower priority prelude hardcoded in the resolve (except for "legacy" module shadowing) :)
Edit: It can be "un-hardcoded", but I don't see strong reasons to do this so far.

@jseyfried
Copy link
Contributor

@nikomatsakis yeah, I think that is a good way to view #[no_implicit_prelude].

@petrochenkov

I have a more targeted solution - a new unstable attribute #[...fallback_to_primitive_types]

I really like this idea! I think we could take this further by including the primitive type modules in the prelude, only allowing primitive types to be used via a #[primitive_type] module, and cleaning up the fallback semantics. Then,

  • We could think of the #[primitive_type] modules as special type/module hybrids, most of which could be backwards-compatibly replaced with ordinary types once associated consts are stabilized and importable. In particular,
    • A #[primitive_type] module used as the last segment (or sole segment) in a path would resolve to the type (except when being imported).
    • Otherwise, it would resolve to the type unless the following segment resolved in the module.
  • We wouldn't need to make the primitive types specially available; std::u8, etc. would work.
  • #[no_implicit_prelude] would really mean no prelude, not just a smaller prelude.
  • Users could recover #[no_implicit_prelude]'s current behavior by importing just the primitive type modules or creating a custom prelude with just these modules.

@jseyfried
Copy link
Contributor

Finally, we could avoid any immediate breakage with a warning cycle, something like:

mod u8 {}
fn f(_: u8) {}
//~^ WARN the type `u8` is shadowed by the user-defined module `::u8`, use `::std::u8` instead

@petrochenkov
Copy link
Contributor Author

#[no_implicit_prelude] would really mean no prelude, not just a smaller prelude.

I'm not sure removing primitive types together with the main prelude is practical. I need to experiment with libcore as the main consumer of no prelude.

We could think of the #[primitive_type] modules as special type/module hybrids, ...

I suspect this will turn out to be much larger hack, then the current fallback.
Edit: Or maybe not! Now I think I've got the idea, I'll try to implement it this evening. (We still need to make sure that development of libcore doesn't turn into hell without primitive types available by default.)

@jseyfried
Copy link
Contributor

I suspect this will turn out to be much larger hack, then the current fallback.

I think of it as less of a hack if the semantics are cleaner (which I believe they would be).

Also, the current implementation can be so simple only because resolve_possibly_assoc_item is a hack -- once we rewrite that, I think my semantics would be no more complicated to implement.

@petrochenkov
Copy link
Contributor Author

@jseyfried
This is how libcore looks with primitive types disabled by default: https://github.com/petrochenkov/rust/tree/noprimcore
I haven't finished the work completely, but basically you have to manually import primitive types into almost every module, you can't do much without them. It's very inconvenient.
I think the solution is simple - make prelude injection crate-level (#32167 (comment)) and manually inject libcore's own prelude into its root once (I hope this won't break import resolution?).
After that #[primitive_type] can be implemented and primitive types can be put into the main prelude, because uses of no_prelude outside of libcore are exceedingly rare according to the crates.io grep.

@jseyfried
Copy link
Contributor

make prelude injection crate-level and manually inject libcore's own prelude into its root once

Completely agree.

In the mean time, perhaps we could leverage the existing prelude injection by adding a private module std in libcore:

mod std {
    pub use prelude; // i.e. `libcore`'s prelude
}

and enabling ordinary prelude injection in libcore.

@bors
Copy link
Contributor

bors commented Mar 13, 2016

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

@petrochenkov
Copy link
Contributor Author

I've implemented #[primitive_type] (mostly, std::u8 still doesn't resolve to the type u8 in type positions, but it doesn't matter much) and I don't know if I like the result or not
https://github.com/petrochenkov/rust/tree/prim3
Please share your opinion.

Primitive types are bound to items in libcore (and also libcollections, librustc_unicode, libstd, all necessary for compatibility), mostly to modules, but bool doesn't have compatibility issues and can be bound to anything - enum bool {} or even type bool = bool;.
Most of the time were spent on fixing libcore, libstd and all those no_core and no_implivit_prelude tests.

cc @eddyb, you wanted to do this looong time ago

@jseyfried
Copy link
Contributor

Nice! I definitely like the result.

@eddyb
Copy link
Member

eddyb commented Mar 15, 2016

How about enum bool { false = 0, true = 1 }?

@petrochenkov
Copy link
Contributor Author

@eddyb

How about enum bool { false = 0, true = 1 }?

For documentation purpose only (the item body is ignored) or rust-lang/rfcs#348?
(Either way requires demoting true and false from keywords.)

@jseyfried

Nice! I definitely like the result.

Ok, I'll fix the std::u8 bug, submit prim3 as a separate PR and let the lang team decide.

@Manishearth Manishearth reopened this Mar 26, 2016
@bors
Copy link
Contributor

bors commented Mar 26, 2016

⌛ Testing commit b418cd2 with merge ab13e5a...

@bors
Copy link
Contributor

bors commented Mar 26, 2016

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Contributor

bors commented Mar 26, 2016

⌛ Testing commit b418cd2 with merge 1ab7b62...

@bors
Copy link
Contributor

bors commented Mar 26, 2016

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Contributor

bors commented Mar 26, 2016

⌛ Testing commit b418cd2 with merge 5b39678...

@bors
Copy link
Contributor

bors commented Mar 26, 2016

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Contributor

bors commented Mar 26, 2016

⌛ Testing commit b418cd2 with merge c10b1e9...

@bors
Copy link
Contributor

bors commented Mar 26, 2016

⛄ The build was interrupted to prioritize another pull request.

bors added a commit that referenced this pull request Mar 26, 2016
Rollup of 11 pull requests

- Successful merges: #32131, #32199, #32257, #32325, #32435, #32447, #32448, #32456, #32469, #32476, #32482
- Failed merges: #32240
@bors bors merged commit b418cd2 into rust-lang:master Mar 26, 2016
@petrochenkov petrochenkov deleted the prim branch September 21, 2016 19:54
bors added a commit that referenced this pull request Apr 26, 2018
Module experiments: Add one more prelude layer for extern crate names passed with `--extern`

Implements one item from https://internals.rust-lang.org/t/the-great-module-adventure-continues/6678/183

When some name is looked up in lexical scope (`name`, i.e. not module-relative scope `some_mod::name` or `::name`), it's searched roughly in the next order:
- local variables
- items in unnamed blocks
- items in the current module
- ✨ NEW! ✨ crate names passed with `--extern` ("extern prelude")
- standard library prelude (`Vec`, `drop`)
- language prelude (built-in types like `u8`, `str`, etc)

The last two layers contain a limited set of names controlled by us and not arbitrary user-defined names like upper layers. We want to be able to add new names into these two layers without breaking user code, so "extern prelude" names have higher priority than std prelude and built-in types.
This is a one-time breaking change, that's why it would be nice to run this through crater.
Practical impact is expected to be minimal though due to stylistic reasons (there are not many `Uppercase` crates) and due to the way how primitive types are resolved (#32131).
bors added a commit that referenced this pull request May 1, 2018
Module experiments: Add one more prelude layer for extern crate names passed with `--extern`

Implements one item from https://internals.rust-lang.org/t/the-great-module-adventure-continues/6678/183

When some name is looked up in lexical scope (`name`, i.e. not module-relative scope `some_mod::name` or `::name`), it's searched roughly in the next order:
- local variables
- items in unnamed blocks
- items in the current module
- ✨ NEW! ✨ crate names passed with `--extern` ("extern prelude")
- standard library prelude (`Vec`, `drop`)
- language prelude (built-in types like `u8`, `str`, etc)

The last two layers contain a limited set of names controlled by us and not arbitrary user-defined names like upper layers. We want to be able to add new names into these two layers without breaking user code, so "extern prelude" names have higher priority than std prelude and built-in types.
This is a one-time breaking change, that's why it would be nice to run this through crater.
Practical impact is expected to be minimal though due to stylistic reasons (there are not many `Uppercase` crates) and due to the way how primitive types are resolved (#32131).
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 PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants