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

Changes to name resolution #1560

Merged
merged 4 commits into from Jul 29, 2016

Conversation

Projects
None yet
@nrc
Copy link
Member

nrc commented Mar 29, 2016

Some internal and language-level changes to name resolution.

Internally, name resolution will be split into two parts - import resolution and
name lookup. Import resolution is moved forward in time to happen in the same
phase as parsing and macro expansion. Name lookup remains where name resolution
currently takes place (that may change in the future, but is outside the scope
of this RFC). However, name lookup can be done earlier if required (importantly
it can be done during macro expansion to allow using the module system for
macros, also outside the scope of this RFC). Import resolution will use a new
algorithm.

The observable effects of this RFC (i.e., language changes) are some increased
flexibility in the name resolution rules, especially around globs and shadowing.

rendered

Changes to name resolution
Some internal and language-level changes to name resolution.

Internally, name resolution will be split into two parts - import resolution and
name lookup. Import resolution is moved forward in time to happen in the same
phase as parsing and macro expansion. Name lookup remains where name resolution
currently takes place (that may change in the future, but is outside the scope
of this RFC). However, name lookup can be done earlier if required (importantly
it can be done during macro expansion to allow using the module system for
macros, also outside the scope of this RFC). Import resolution will use a new
algorithm.

The observable effects of this RFC (i.e., language changes) are some increased
flexibility in the name resolution rules, especially around globs and shadowing.
@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Mar 29, 2016

I am especially excited about the implications for macro resolution, since these are, as of now, really annoying. Having them fitting better into the module system would be an awesome change.

@nrc

This comment has been minimized.

Copy link
Member Author

nrc commented Mar 29, 2016

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Mar 29, 2016

This RFC, specifically the ability to import the same item multiple times without conflict, solves one of the bigger unresolved pains in Diesel. The ability to have a local name shadow an import feels like a loss to me, though. When you take macros into account, it's possible for defining a struct to silently break something, with a fairly non-local error. I'd much prefer something akin to hiding from Haskell.

mod foo {
    pub struct Bar;
}

use foo::* hiding Bar;

struct Bar;
@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Mar 29, 2016

Would this be legal as a result of this RFC?

mod foo {
pub mod apple {
    use super::*;
    pub type Carrot = u32;
}
pub mod banana {
    use super::*;
    pub type Carrot = u32;
}
pub use self::apple::*;
pub use self::banana::*;
}
fn main() {
    let x: foo::Carrot = 5;
}
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Mar 29, 2016

@retep998
No, types are still processed after name resolution, so apple::Carrot and banana::Carrot are considered different items.
Instead you can use primitive types like u32 in imports through one more level of indirection:

mod primitive {
    pub type u32_ = u32;
}
pub mod apple {
    pub use primitive::u32_ as Carrot;
}
pub mod banana {
    pub use primitive::u32_ as Carrot;
}

(Or even nicer, on >=1.9 with two levels of indirection: gist)


### Privacy

In order to resolve imports (and in the future for macro privacy), we must be

This comment has been minimized.

@petrochenkov

petrochenkov Mar 29, 2016

Contributor

To clarify, this RFC doesn't propose the ability to leak private items through macros, right? I.e.

struct S;
pub macro s {
    // Still an error, privacy is checked in the context
    // of macro expansion and not macro definition,
    // private `S` cannot be leaked.
    () => (S)
}

This comment has been minimized.

@nrc

nrc Mar 29, 2016

Author Member

No, there are no user-visible changes to privacy or macros in this RFC. I do plan to propose this later though.

@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Mar 29, 2016

@petrochenkov The reason I ask is due to winapi, where sometimes two (or more!) headers will both define the same typedef, and I'd rather not have to pick a header to be the canonical one (nevermind that if I want to be able to cfg out some modules, I'd have really complex cfg conditions to re-enable the non-canonical typedefs). If I define a typedef via type or use in a crate, is there a difference between the two for downstream consumers (assuming it is a primitive type or a non-tuple struct)?

@nrc nrc added the T-lang label Mar 29, 2016

@nrc nrc self-assigned this Mar 29, 2016

@nrc

This comment has been minimized.

Copy link
Member Author

nrc commented Mar 30, 2016

@retep998 There is a difference between type and use - the former are processed later in the compiler and are not part of the module system. At name resolution time two different aliases to the same type are considered different items. Two different use imports of the same item are considered the same for the purposes of 'overlapping' imports.

One could imagine erasing aliases earlier in the compilation process and thus making them transparent to name resolution (and the shadowing rules). I think that should be a separate RFC and I'm not sure if it is a good idea or not.

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Mar 30, 2016

I suspect that would be a bad idea if extended to generic type aliases.

the lowering from AST to HIR, so the HIR is a name-resolved data structure. Or,
name lookup could be done lazily (probably with some caching) so no tables
binding names to definitions are kept. I prefer the first option, but this is
not really in scope for this RFC.

This comment has been minimized.

@eddyb

eddyb Apr 2, 2016

Member

When in doubt, combine your options: lazy lowering (similar to HIR+Ty -> HAIR for MIR building), triggered by type collection (top-level items) or type inference (expressions inside functions) that uses binding tables to determine what nodes to produce.

@eddyb eddyb referenced this pull request Apr 3, 2016

Merged

Procedural macros #1566

where name resolution currently takes place.

To resolve imports, the algorithm proceeds as follows: we start by parsing as
much of the program as we can; like today we don't parse macros. When we find

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 5, 2016

Contributor

What exactly does this mean? Don't parse macro definitions? invocations? the result of macro expansion?

This comment has been minimized.

@eddyb

eddyb Apr 6, 2016

Member

We don't attempt to parse the invocations of a macro in any way (other than storing them as TTs).

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 6, 2016

Contributor

Ok, that of the 3 makes the most sense but just wanted to check :)

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 5, 2016

In order to keep macro expansion comprehensible to programmers, we must enforce that all macro uses resolve to the same binding at the end of resolution as they do when they were resolved.

I think this is required for basic soundness, not just peace of mind. Say two macro use sites, a and b, expand to a macro definition used at a third use site, c. Expanding a then c vs b then c leads to a different result, yet at each expansion of c the binding for c's macro was hitherto unambiguous. The only red flag at the end is the overlapping macro bindings once a and b are expanded.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 6, 2016

Worse yet, if a and b expand to glob imports providing binding the macro expanded at c, there isn't actually an error at the end of the [a, c, b] or [b, a, c] expansions (in the final states themselves), because overlapping glob imports are not a problem.


The macro changes are not backwards compatible, which means having a macro
system 2.0. If users are reluctant to use that, we will have two macro systems
forever.

This comment has been minimized.

@xeno-by

xeno-by Apr 8, 2016

How are you planning to mitigate this risk?

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 10, 2016

Would this be a good time to fix rust-lang/rust#26775 ?

@jseyfried

This comment has been minimized.

Copy link

jseyfried commented Apr 10, 2016

@Ericson2314 #26775 was fixed by #31362.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 10, 2016

@jseyfried pub extern crate seem to still not work for me, though?! It doesn't look that that was addressed by your fix yet rust-lang/rust#21757 was closed as dupe of rust-lang/rust#26775. edit nevermind, sorry for confusion.

Also does https://github.com/nrc/rfcs/blob/aac02932e3c3b5749d1c15b83813ca1c3ef00281/text/0000-name-resolution.md#non-public-imports apply to (non-pub) extern crate?

@jseyfried

This comment has been minimized.

Copy link

jseyfried commented Apr 10, 2016

@Ericson2314 Perhaps you are using stable? The fix is on beta but hasn't landed on stable yet. If it doesn't work for you on beta or nightly, could you provide an example?

extern crates are currently treated the way that section proposes we treat use imports, so it doesn't really apply.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 10, 2016

@jseyfried edited my comment but my mistake it does indeed work; for the record I am on nightly.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 10, 2016

Ah and yes I somehow also missed the current semantics of extern crate are indeed that, so this + your fix unifies everything as is desired. Yay!

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 12, 2016

I'm finding it hard to reason about the precise model proposed here, I admit. I wonder if there is a way to make the write up a bit more declarative.

In any case, I think the only change here that I found problematic when working on my name resolution prototype was this one:

Note that shadowing is namespace specific. I believe this is consistent with our general approach to name spaces.

I think the problem had to do with the possibility that, in some round, we would bring in a name from a glob that would later have to be shadowed, but we didn't know at the time whether it ought to be shadowed or not. I'll try to see if I can come up with a precise example.

@jseyfried

This comment has been minimized.

Copy link

jseyfried commented Apr 12, 2016

I think the problem had to do with the possibility that, in some round, we would bring in a name from a glob that would later have to be shadowed, but we didn't know at the time whether it ought to be shadowed or not.

This is primarily why we currently distinguish between Indeterminate and Failed imports, which adds quite a bit of complexity (cf. this code and this code). For example,

mod bar {
    pub fn foo() {}
    pub use baz::*;
}
mod baz {
    pub use bar::*;
}   

pub mod foo {}
mod quux {
    pub use super::*; // This defines `quux::foo` in the type namespace ...
    pub use bar::foo;
    //^ ... but we can't know that until we know that `bar::foo` fails in the type namespace,
    //| which requires us to detect that `bar -> baz -> bar -> ...` is a glob cycle.
}

If we changed the semantics so that a single import (and perhaps also an ordinary item for consistency) always shadowed glob imports in both namespaces (even if it only defined one of them), then we wouldn't need to distinguish between failing and indeterminate resolutions.

I think this change would cause too much breakage to be practical, but if you'd like I could implement it and we could do a crater run to see if doing a warning cycle would be feasible.

@jseyfried

This comment has been minimized.

Copy link

jseyfried commented Apr 12, 2016

Also, while the complexity is unfortunate, it does generalize well in the presence of unexpanded macros -- we would just have to report that an otherwise failing name in a module with unexpanded macros is indeterminate.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jul 29, 2016

Tracking issue: rust-lang/rust#35120

If you'd like to keep following the development of this feature, please subscribe to that issue, thanks! :)

### Multiple imports of the same binding

A name may be imported multiple times and used if both names bind to the same
item. E.g.,

This comment has been minimized.

@petrochenkov

petrochenkov Jul 29, 2016

Contributor

I forgot to mention a drawback - names (and paths) don't have a definitive "point of introduction" anymore.
I.e. you can't say "the name was defined/imported here", there is always can be >1 of such definition/import points.
Unless all of these points are tracked, this complicates, for example, stability checking for reexports or error reporting if errors refer to these points.

@jseyfried jseyfried referenced this pull request Aug 24, 2016

Closed

use some::Trait as _; #1311

bors added a commit to rust-lang/rust that referenced this pull request Aug 29, 2016

Auto merge of #35894 - jseyfried:new_import_semantics, r=nrc
Implement RFC 1560 behind `#![feature(item_like_imports)]`

This implements rust-lang/rfcs#1560 (cc #35120) behind the `item_like_imports` feature gate.

The [RFC text](https://github.com/rust-lang/rfcs/blob/master/text/1560-name-resolution.md#changes-to-name-resolution-rules) describes the changes to name resolution enabled by `#![feature(item_like_imports)` in detail. To summarize,
 - Items and named imports shadow glob imports.
 - Multiple globs can import the same name if the name is unused or the imports are shadowed.
 - Multiple globs can import the same name if the imports are of the same item (following re-exports).
  - The visibility of such a name is the maximum visibility of the imports.
  - Equivalently, adding a glob import will never reduce the visibility of a name, nor will removing one increase it.
 - Non-prelude private imports can be used wherever we currently allow private items to be used.
  - Prelude-imported names are unaffected, i.e. they continue to be usable only in lexical scopes.
 - Globs import all visible names, not just public names.
  - Equivalently, glob importing from an ancestor module imports all of the ancestor's names, and glob importing from other modules is unchanged.

r? @nrc

bors added a commit to rust-lang/rust that referenced this pull request Aug 29, 2016

Auto merge of #35894 - jseyfried:new_import_semantics, r=nrc
Implement RFC 1560 behind `#![feature(item_like_imports)]`

This implements rust-lang/rfcs#1560 (cc #35120) behind the `item_like_imports` feature gate.

The [RFC text](https://github.com/rust-lang/rfcs/blob/master/text/1560-name-resolution.md#changes-to-name-resolution-rules) describes the changes to name resolution enabled by `#![feature(item_like_imports)` in detail. To summarize,
 - Items and named imports shadow glob imports.
 - Multiple globs can import the same name if the name is unused or the imports are shadowed.
 - Multiple globs can import the same name if the imports are of the same item (following re-exports).
  - The visibility of such a name is the maximum visibility of the imports.
  - Equivalently, adding a glob import will never reduce the visibility of a name, nor will removing one increase it.
 - Non-prelude private imports can be used wherever we currently allow private items to be used.
  - Prelude-imported names are unaffected, i.e. they continue to be usable only in lexical scopes.
 - Globs import all visible names, not just public names.
  - Equivalently, glob importing from an ancestor module imports all of the ancestor's names, and glob importing from other modules is unchanged.

r? @nrc

bors added a commit to rust-lang/rust that referenced this pull request Sep 2, 2016

Auto merge of #35894 - jseyfried:new_import_semantics, r=nrc
Implement RFC 1560 behind `#![feature(item_like_imports)]`

This implements rust-lang/rfcs#1560 (cc #35120) behind the `item_like_imports` feature gate.

The [RFC text](https://github.com/rust-lang/rfcs/blob/master/text/1560-name-resolution.md#changes-to-name-resolution-rules) describes the changes to name resolution enabled by `#![feature(item_like_imports)` in detail. To summarize,
 - Items and named imports shadow glob imports.
 - Multiple globs can import the same name if the name is unused or the imports are shadowed.
 - Multiple globs can import the same name if the imports are of the same item (following re-exports).
  - The visibility of such a name is the maximum visibility of the imports.
  - Equivalently, adding a glob import will never reduce the visibility of a name, nor will removing one increase it.
 - Non-prelude private imports can be used wherever we currently allow private items to be used.
  - Prelude-imported names are unaffected, i.e. they continue to be usable only in lexical scopes.
 - Globs import all visible names, not just public names.
  - Equivalently, glob importing from an ancestor module imports all of the ancestor's names, and glob importing from other modules is unchanged.

r? @nrc

@ExpHP ExpHP referenced this pull request Sep 26, 2016

Closed

Tracking issue for `pub(restricted)` privacy (RFC #1422) #32409

6 of 6 tasks complete

homu added a commit to gfx-rs/gfx that referenced this pull request Nov 1, 2016

Auto merge of #1075 - msiglreith:pr_macro, r=kvark
Fix pipeline definition macro

`use $crate;` is not allowed due to recent restrictions and `use super::*;` won't import the gfx crate. (allowed in future but feature gated rust-lang/rfcs#1560)

Closes #1070

@jseyfried jseyfried referenced this pull request Nov 2, 2016

Closed

Tracking issue for "Macros 1.1" (RFC #1681) #35900

50 of 53 tasks complete

@petrochenkov petrochenkov referenced this pull request Feb 24, 2018

Closed

Inherit use #277

@Ixrec Ixrec referenced this pull request May 23, 2018

Open

RFC: Delegation #2393

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.