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

Ambiguity errors in 2018 uniform import paths are not technically necessary #56414

Closed
petrochenkov opened this issue Dec 1, 2018 · 9 comments · Fixed by #112086
Closed

Ambiguity errors in 2018 uniform import paths are not technically necessary #56414

petrochenkov opened this issue Dec 1, 2018 · 9 comments · Fixed by #112086
Labels
A-resolve Area: Name resolution C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

petrochenkov commented Dec 1, 2018

Right now import resolution on 2018 edition fails with an ambiguity error if multiple candidates are found:

struct S; // Outer

fn main() {
    struct S; // Inner

    use S as Z; // ERROR `S` is ambiguous (name vs any other name during import resolution)

    let s = S; // OK, the Inner S is preferred
}

This is different from non-import paths in which resolutions from closer scopes are preferred without errors.

This restriction is technically unnecessary and exists because some people were uncomfortable with disambiguation happening in import paths specifically (see previous threads, ... many of them, #55618 being the latest one).

This restriction can be lifted fully or partially if it causes too much trouble in practice with ambiguity errors, or with addition of new names to libraries being backward-incompatible due to new ambiguities.

(By "partially" I mean e.g. "disambiguation is okay, unless one of the candidates is an extern crate" or something like that.)

@petrochenkov petrochenkov added A-resolve Area: Name resolution T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Dec 1, 2018
@joshtriplett
Copy link
Member

👍 for "remove ambiguity warnings for any case except local name versus extern crate".

@Centril Centril added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jan 24, 2019
@ajrouvoet
Copy link

If I understand your proposal correctly, then I believe that the following example (albeit perhaps contrived) would render quite paradoxical:

pub mod a {    // outer decl
  pub mod a {} // inner decl
}
pub mod b { 
  use super::*; // outer use
  pub fn f() {     // inner use
    use a::*;
  }
}

How does the name a in the inner use statement resolve? If the top level module declaration is in scope (by the outer use statement), then the inner module declaration should also be available by means of the inner use statement. By lifting the ambiguity restriction, this declaration should shadow the outer declaration from being in scope.

In short: the outer declaration is only in scope, if it is not in scope. Or something like that :)
This is "Rustell's paradox", and the restriction that you describe seems to ensure that it is only barely dodged in the Rust name semantics.

FWIW, I am not a Rust programmer, so forgive me if I misunderstood the semantics or your proposal. I am interested in name binding semantics however and I figured the above example was worth keeping in mind.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jun 24, 2019

@ajrouvoet
The "name vs any other name during import resolution" error is not the last line of defense against paradoxes like this :)

This example will likely cause the "glob import vs any other name from outer scope during import/macro resolution" error if the "name vs any other name during import resolution" restriction is removed.

I can check today, the fix for this issue is a one-line change in the compiler (this if branch https://github.com/rust-lang/rust/blob/master/src/librustc_resolve/macros.rs#L782 needs to be removed).

@ajrouvoet
Copy link

Thanks for having a look; I'm very curious about this.

I can see how the type-checker may guard against this specific scenarios. At the same time I wonder if one could still give a declarative semantics of name resolution in Rust for the result. It may be unwanted that name resolution can only be explained in the end in terms of what the implemented algorithm can/cannot resolve. This seems important for reasoning about the language, for explaining rust to programmers, but also for tools that strive for conformance with the reference rust compiler.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jun 24, 2019

I wonder if one could still give a declarative semantics of name resolution in Rust for the result.

It should certainly be possible to compact the specification of import resolution behavior into something shorter than its implementation in the compiler.
AFAIK, there were some plans to build an isolated name resolution model outside of the compiler, but I'm not aware of any concrete results following from those plans.

The general idea behind the ambiguity errors in particular is that we report an error if some name in inner scope can "materialize" later (due to import dependencies, or macro expansion) than the same name in outer scope.

#53778 (comment) describes how this general idea applies to macros.
With globs the situation is simpler - a name can "materialize" from a glob arbitrarily late, so a name from a glob in inner scope can never coincide with any name in outer scopes.

#![feature(decl_macro)]

mod m { pub macro a() {} }

macro a() {} // Outer scope

fn main() {
    use m::*; // Inner scope
    
    a!(); // error[E0659]: `a` is ambiguous (glob import vs any other name from outer scope during import/macro resolution)
}

@sgrif
Copy link
Contributor

sgrif commented Dec 17, 2019

There's a similar issue when an item being imported has the same name as the path being used.

mod users {
    pub struct table;
    pub mod dsl {
        pub use super::table as users;
    }
}

fn main() {
    use users::dsl::*;
}

Outputs:

error[E0659]: `users` is ambiguous (name vs any other name during import resolution)
 --> src/main.rs:9:9
  |
9 |     use users::dsl::*;
  |         ^^^^^ ambiguous name
  |
note: `users` could refer to the struct imported here
 --> src/main.rs:9:9
  |
9 |     use users::dsl::*;
  |         ^^^^^^^^^^^^^
note: `users` could also refer to the module defined here
 --> src/main.rs:1:1
  |
1 | / mod users {
2 | |     pub struct table;
3 | |     pub mod dsl {
4 | |         pub use super::table as users;
5 | |     }
6 | | }
  | |_^

An import should never be ambiguous with itself.

@joshtriplett joshtriplett added the S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. label Jun 8, 2022
@joshtriplett
Copy link
Member

joshtriplett commented Jun 8, 2022

We discussed this in today's @rust-lang/lang meeting. We think that this could potentially be removed at this point, given that we've long-since settled on a specific module-system model, but we'd like to confirm which cases still produce errors (in order to confirm that those errors aren't actually catching anything of value), and whether further changes have reduced the set of things that produce errors.

@pnkfelix
Copy link
Member

pnkfelix commented Jun 8, 2022

Another way of putting this:

  • Is the check sufficiently annoying to users that we should remove it?
  • Another take: does it provide so little utility that we should remove it because it is more trouble to continue supporting it?

@petrochenkov
Copy link
Contributor Author

I implemented this change in #112086.

compiler-errors added a commit to compiler-errors/rust that referenced this issue Jun 30, 2023
resolve: Remove artificial import ambiguity errors

Fixes rust-lang#56414.

FCP report: rust-lang#112086 (comment)
@bors bors closed this as completed in be0a96f Jun 30, 2023
ehuss added a commit to ehuss/reference that referenced this issue Jun 30, 2024
This was changed in rust-lang/rust#56414
to favor in-scope items.
ehuss added a commit to ehuss/reference that referenced this issue Jul 25, 2024
This was changed in rust-lang/rust#56414
to favor in-scope items.
Tiger0202 added a commit to Tiger0202/rust-lang that referenced this issue Dec 11, 2024
This was changed in rust-lang/rust#56414
to favor in-scope items.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-resolve Area: Name resolution C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants