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

Improve the remaining rough corners of the module system #1289

Open
Kimundi opened this issue Sep 21, 2015 · 6 comments
Open

Improve the remaining rough corners of the module system #1289

Kimundi opened this issue Sep 21, 2015 · 6 comments
Assignees
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.

Comments

@Kimundi
Copy link
Member

Kimundi commented Sep 21, 2015

Motivation

The module system (specifically name resolution) still has a few rough edges, footguns and corner cases that can be removed in a way that doesn't change its core semantics. Similarly, error messages and doc generation need to be smarter about the module system in some areas.

This issue proposes all changes that the author regards as improvements, and they would build on prior work like #116 and #385.

This is not a RFC due to its size, and lack of details. Rather, the idea is to paint the big picture, and have a central place to discuss wether this is a direction the module system should be taken or not.

Proposals

  • Change wildcard import semantics to be shadow-able by other items and non-wildcard imports.

    • The shadowing applies to any access to paths in the module, including other use items.
    • Don't error on non-disjoint wildcard imports, only error on access of a name
      publicly visible through more more than one of them
      if the names refer to different logical items.
      Recommend to use an explicit import to choose between them in the error message.
    • This would mitigate the dangers of extending the set of exported items in a module, since
      downstream users using wildcard imports can always clarify or guard in advance
      with an explicit import.
    • This would enable the liberal use of wildcard imports for things like custom preludes, test modules, etc.
    • This would address issue Multiple globs are basically unusable #553 and
      Allow pub use re-imports/exports of already privately imported symbols #1285
    • This should be completely backwards compatible to implement.
    • The unused import lint would trigger for any wildcard import through
      which zero names are resolved.
  • Expose private imports as regular importable items to submodules

    • This makes imports non-special in regard to privacy and other imports.
    • This enables seamless moving around of items by replacing them with a import,
      breaking no other code on the inside of a module.
    • This allows concise patterns:
      • A single use super::*; in a test submodule to import everything
        needed to test the parent.
      • use self::submodule::*; to flatten a module hierarchy.
      • mod my_prelude { pub use other_prelude::*; pub use my_items::*; }
        to easily merge modules together.
    • This is backwards compatible if implemented at the same time as the wildcard import change,
      since any names becoming visible with this change would not conflict with existing imports:
      • Existing non-wildcard imports would shadow the additional names in existing wildcard imports.
      • Existing non-wildcard imports will not change meaning and
        existing modules will not fail to compile since imports are currently strictly
        forbidden from shadowing other items.
  • Make imports in non-module scopes see items defined in non-module scopes

  • Address recursive mutual import issues.

    • This is basically a bugfix.
    • This has, however, caused enough trouble to affect decisions about
      language semantics in the past.
    • I am not informed enough about resolve itself to know wether
      this is algorithmically feasible to implement in rustc, but
      as far as I abstractly know about the problem, it should be doable.
      (Say with a fixpoint algorithm gathering the set of possible imports from wildcards, kept finite by normalizing the paths to the canonical path of a item)
    • Mutual recursive wildcard imports that actually reach items
      should become a non-error with the new wildcard import semantics.
      This should work, and doesn't today:
      mod a { pub use b::*; pub struct a_item; }
      mod b { pub use a::*; pub struct b_item; }
      use a::b_item;
      use b::a_item;
    • Mutual recursive (wildcard) imports should be detected and produce a
      nice error message. This should fail to compile
      (and already does today, though not with a nice message):
      mod a { pub use b::*; }
      mod b { pub use a::*; }
      use a::b_item;
      use b::a_item;
      mod a { pub use b::c; }
      mod b { pub use a::c; }
  • Error messages and rustdoc output should treat imports and paths of imported
    items smarter.

    • This does not change language semantics.
    • This describes heuristics to improve error messages and docs.
    • Each item has a "canonical path" in its crate that uniquely describes its definition:
      mod a {
          pub struct b; // canonical path <crate>::a::b
      }
      use a::b; // non-canonical path <crate>::b for canonical path <crate>::a::b
    • Each resolvable name is potentially a chain of reexports "terminating"
      in a non-import item:
      mod a {
          pub struct b;
      }
      use a::b as c;
      use c as d;
      // Visible names in <crate>:
      // a              terminating in a
      // a::b           terminating in a::b
      // c -> a::b      terminating in a::b
      // d -> c -> a::b terminating in a::b

    extern crate statements count as a reexport here.

    • Each chain of imports in a resolvable name potentially terminates in a
      item private from the location of the start of the chain.
      In this case, the last visible reexport in the chain is defined
      as "visibly-terminating":
      mod a {
          pub use self::c as b;
          struct c;
      }
      // Visible names in <crate>:
      // a            terminating in a,    visibly-terminating in a
      // a::b -> a::c terminating in a::c, visibly-terminating in a::b
    • Currently, if the compiler prints the path of an item in a error message
      it is <name_of_the_crate>::<canonical_path_in_the_crate>. Example:
      use std::rc::Rc as RefCount;
    
      fn main() {
          let () = RefCount::new(5);
      }
      <anon>:4:9: 4:11 error: mismatched types:
      expected `alloc::rc::Rc<_>`,
         found `()`
      (expected struct `alloc::rc::Rc`,
                 found ()) [E0308]
    
    • A more useful heuristic would be to print
      <resolved-name-in-current-contex> defined at <visibly-terminating-path>.
      For example:
      use std::rc::Rc as RefCount;
    
      fn main() {
          let () = RefCount::new(5);
      }
      <anon>:4:9: 4:11 error: mismatched types:
      expected `RefCount<_>` defined at std::rc::Rc,
         found `()`
      (expected struct `RefCount` defined at `std::rc::Rc`,
                 found ()) [E0308]
    
    • If there is no visibly-terminating-item, print
      extern crate <name_of_the_crate>::<visibly-terminating-path-in-that-crate>
      similarly to before. Maybe also add a hash or version number to clear up
      issues like in Two different versions of a crate interacting leads to unhelpful error messages rust#22750.
      • This indicates to the user that the mentioned item is not part of the
        current crate in any way, and that he would have to add a extern crate
        item or resolve a version mismatch to change that.
    • Rustdoc should, for each crate it generates documentation for:
      • Treat all use statements that are visibly-terminating in the
        reexport chain of any other use item in that crate as its terminating item.
        • This uniformly resolves issues like presenting the libstd facade in the
          docs.
        • If the terminating item is defined in another crate and rustdoc
          currently generates documentation for that one as well (to make cross-crate links),
          also append a "defined at "
          note that links there.
          • This documents the relationship between dependencies
      • Treat all other use statements uniformly, for example by showing them simply
        as a "reexport" linking to the visibly-terminating item.
      • Treat items or links to items that don't have a visibly-terminating import
        referring to them in the current crate like today.
        • This handles the common situation of visible definitions or types
          defined in another crate that is not also reexported,
          like the return type of a pub fn foo() -> Rc<u32> in a crate
          not reexporting std or alloc.

Precedence

@felixc
Copy link

felixc commented Sep 26, 2015

As discussed in #1285, a revision of these features would greatly ease the pain of writing wrapper libraries that wish to glob-import the items being wrapped, but also publicly re-export a subset of them.

The ability to do:

use foo::*;
pub use foo::Bar;

would be very much appreciated.

👍

@ticki
Copy link
Contributor

ticki commented Sep 28, 2015

👍

@nrc
Copy link
Member

nrc commented Aug 22, 2016

Several of these suggestions are addressed by #1560

@Kimundi
Copy link
Member Author

Kimundi commented Aug 23, 2016

Indeed, a lot has been done in the last year :)

@ebkalderon
Copy link

Just curious, what is the current status of these efforts?

@sfackler
Copy link
Member

sfackler commented Apr 4, 2017

"Expose private imports as regular importable items to submodules" is done, as is much of "Address recursive mutual import issues". Not sure about the others off the top of my head.

bors added a commit to rust-lang/rust that referenced this issue Nov 14, 2018
[beta] resolve: Implement uniform paths 2.0

With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible.

The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`.
This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.

All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).

Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.
This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities.
This is clearly a breaking change for 2018 edition as well, but also a minor one.

The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.

This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)

Why beta:
- This is a breaking change on 2018 edition.
- This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta.

cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668

r? @ghost
bors added a commit to rust-lang/rust that referenced this issue Nov 18, 2018
[beta] resolve: Implement uniform paths 2.0

With this PR paths in imports on 2018 edition are resolved as relative in the scope in which they are written, similarly to any other paths.
The previous implementation worked differently - it tried to resolve the import in the current module (`self::import`) and in the "crate universe" (`::import`), and then merge these two resolutions if possible.

The difference is that the new scheme can refer to strictly larger set of names in scope - names from unnamed blocks, names from all kinds of preludes, including macros imported with `#[macro_use] extern crate`, built-in types/macros, macros introduced with `macro_rules`.
This means strictly more potential ambiguities and therefore ambiguity errors, since we keep the rule that any two different candidate names in scope conflict with each other during import resolution. So this is a breaking change for 2018 edition, but it should be relatively minor.

All paths that don't start with an extern crate are also gated with the `uniform_paths` feature, paths that refer to extern crates are not gated (so we effectively get something like "future-proofed anchored paths" on stable).

Another difference is treatment of paths in visibilities (`pub(in path)`). Few people remember about paths in visibilities, so before this PR they still used the old 2015 rules even on 2018 edition. Namely, paths in visibilities were crate-relative, analogous to 2015 edition imports.
This PR resolves paths in visibilities as uniform as well, or rather future proofs them in this direction.
Paths in visibilities are restricted to parent modules, so relative paths almost never make sense there, and `pub(in a)` needs to be rewritten as `pub(in crate::a)` in the uniform scheme, de-facto cementing the discouraged status of non-`pub(crate)` and non-`pub(super)` fine-grained visibilities.
This is clearly a breaking change for 2018 edition as well, but also a minor one.

The in-scope resolution strategy for import paths mirrors what is currently done for macro paths on stable (on both editions), so it will continue working even if the "ambiguity always means error" restriction is relaxed in the future.

This PR also significantly improves diagnostics for all kinds of resolution ambiguities, from the newly introduced import ones to pretty old "glob vs glob" conflicts. (That's probably what I've spent most of the time on.)

Why beta:
- This is a breaking change on 2018 edition.
- This is a large PR, it's less risky to forward-port it to nightly, than back-port to beta.

cc #55618
cc #53130
cc rust-lang/rfcs#1289
Closes #18084
Closes #54525
Fixes #54390
Fixes #55668

r? @ghost
@petrochenkov petrochenkov self-assigned this Feb 4, 2019
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 RFC.
Projects
None yet
Development

No branches or pull requests

7 participants