Skip to content

Glob Time Travel #74556

Description

@vlad20012

I tried this code:

mod foo {
    pub mod bar {
        pub mod bar {
            pub fn foobar() {}
        }
    }
}

use foo::*;
use bar::bar;
use bar::foobar;



fn main() {
    bar::foobar();
}

I expected to see this happen: Compilation error. This use bar::bar; shadows one (glob-imported) bar with another bar. As far as I know, this should be forbidden.

Instead, this happened: this code successfully compiled.

Meta

rustc --version --verbose:

rustc 1.47.0-nightly (d7f945163 2020-07-19)
binary: rustc
commit-hash: d7f94516345a36ddfcd68cbdf1df835d356795c3
commit-date: 2020-07-19
host: x86_64-unknown-linux-gnu
release: 1.47.0-nightly
LLVM version: 10.0

c.c. @matklad
c.c. @petrochenkov

Activity

  1. added
    A-resolveArea: Name/path resolution done by `rustc_resolve` specifically
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    T-langRelevant to the language team
    on Jul 20, 2020
  2. 197g commented on Jul 21, 2020

    @197g
    Contributor

    This use bar::bar; shadows one (glob-imported) bar with another bar. As far as I know, this should be forbidden.

    Shadowing a glob import with a specific item is allowed and will unambiguously resolve to the specific one. This is also how the prelude works. The following compiles fine as well:

    // implicit: use std::prelude::v1::*; containing Vec
    struct Vec;
    
    fn a() -> Vec { unimplemented!() }

    playground.

  3. matklad commented on Jul 21, 2020

    @matklad
    Contributor

    To clarify, the problem is not shadowing, but time travel. We shadow identifier which have been already used to resolve another import, so we end up with bar meaning two things at the same time in the same scope.

  4. petrochenkov commented on Jul 21, 2020

    @petrochenkov
    Contributor

    This is a change from Rust 1.44, in Rust 1.43 this is an ICE "inconsistent resolution for an import".
    Possibly a consequence of #70236.

  5. self-assigned this
    on Jul 21, 2020
  6. petrochenkov commented on Jul 21, 2020

    @petrochenkov
    Contributor

    I'm not sure this is a bug.

    When resolving use bar::bar; we are looking at all names in scope except for the names introduced by use bar::bar; itself. Otherwise we'd have cycles even in trivial cases like use my_crate; where my_crate is a name from extern prelude.

    If you take this detail into consideration, then all paths seem to be resolved correctly.

  7. petrochenkov commented on Jul 21, 2020

    @petrochenkov
    Contributor

    cc #62769 (somewhat related)

  8. vlad20012 commented on Jul 21, 2020

    @vlad20012
    ContributorAuthor

    I think, the difference with use my_crate; is that use my_crate; doesn't replace one resolution with another. We already have a name my_crate in the scope that is resolved to a particular extern crate.

    In my example, use bar::bar; uses bar name from the scope, and then rebinds this bar name to another item.

    I can complicate my example a bit.

    mod foo {
        pub mod bar {
            pub mod bar {
                pub fn foobar() { println!("111") }
            }
            pub fn foobar() { println!("222") }
        }
    }
    
    use foo::*;
    use bar::bar;
    use bar::foobar;
    
    fn main() {
        foobar();
    }

    What this program prints, "111" or "222"? It'd say, it depends on imports resolution order, but looks like it always prints "111".

  9. petrochenkov commented on Jul 21, 2020

    @petrochenkov
    Contributor

    It'd say, it depends on imports resolution order

    The intent for the resolution results is to never depend on internal resolution order.
    (cc #53778 (comment), order-dependence can probably happen in practice due to the current not very principled implementation, but hopefully still can be eliminated in backward-compatible-in-practice way by rewriting the main resolution/expansion loop more carefully.)

    but looks like it always prints "111"

    That's what I'd expect.

    • foobar resolves to use bar::foobar
    • bar in bar::foobar resolves to use bar::bar (because non-globs shadow globs)
    • first bar in bar::bar resolves to use foo::* (which the only bar in scope after excluding use bar::bar itself)
  10. petrochenkov commented on Jul 21, 2020

    @petrochenkov
    Contributor

    I'd still be ok with replacing the former ICE with an error though, since that's a more conservative choice.

  11. petrochenkov commented on Jul 21, 2020

    @petrochenkov
    Contributor

    Actually enabling the property "name resolve to the same thing whether some items were excluded during its resolution to avoid cycles or not" may be useful for using something like "inference variables" for unresolved imports and making import resolution more like unification in type inference.

    That's an idea I had for quite some, but I hadn't realized that merging #70236 went against it.

  12. petrochenkov commented on Sep 30, 2020

    @petrochenkov
    Contributor

    Sigh, the issue already affects three stable releases - from 1.44 to 1.46.
    I'll try to prioritize it.

  13. added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Oct 1, 2020
  14. petrochenkov commented on Oct 1, 2020

    @petrochenkov
    Contributor

    Addressed in #77421.

  15. removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Oct 1, 2020
  16. camelid commented on Oct 2, 2020

    @camelid
    Member

    Assigning P-high so this isn't lost. See the relevant discussion.

  17. petrochenkov commented on Oct 5, 2020

    @petrochenkov
    Contributor

    The fix caused some regressions - #77586.

  18. vlad20012 commented on Mar 10, 2023

    @vlad20012
    ContributorAuthor

    @petrochenkov The code in the issue produce no errors using Rust 1.67.1. Should we re-open the issue or is it not considered a bug now?

  19. petrochenkov commented on Mar 10, 2023

    @petrochenkov
    Contributor

    @vlad20012
    The fix was reverted due to breakage (#78784) and the current behavior is considered by design since then.

  20. petrochenkov commented on Mar 10, 2023

    @petrochenkov
    Contributor

    (Also it's not exactly a time travel - #77586 (comment).)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

A-resolveArea: Name/path resolution done by `rustc_resolve` specificallyC-bugCategory: This is a bug.P-highHigh priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.T-langRelevant to the language teamregression-from-stable-to-stablePerformance or correctness regression from one stable version to another.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions