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: Fresh bindings in match/let are preferred to "import ambiguity items" #46079

Closed
petrochenkov opened this issue Nov 18, 2017 · 2 comments · Fixed by #70006
Closed

resolve: Fresh bindings in match/let are preferred to "import ambiguity items" #46079

petrochenkov opened this issue Nov 18, 2017 · 2 comments · Fixed by #70006
Labels
A-resolve Area: Path resolution C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@petrochenkov
Copy link
Contributor

petrochenkov commented Nov 18, 2017

In the example below two glob imports bring two conflicting names V into scope.

enum E1 { V }
use E1::*;

enum E2 { V }
use E2::*; // OK, no conflict with `use E1::*;`

fn main() {
    let v = V; // ERROR `V` is ambiguous
    match v {
        V => {} // No error, `V` is a fresh binding
    }
}

According to RFC 1560 an error is not immediately reported when two globs conflict, it is reported "on use" only if the ambiguous name is actually used.
Internally such an ambiguity creates an "erroneous item" (Def::Err) with name V used exclusively for reporting diagnostics. (However, see #36837, which is an issue closely related to this one.)
For other purposes the name V doesn't exist in our module.

In the example above we can see that in let v = V; V is properly reported as ambiguous on use.

In match however the meaning of V is determined in two-step disambiguation process. First we search for an existing item named V, then if no item named V exists we create a new variable.
Diagnostics-only Def::Err items do not exist from this disambiguation process' point of view, so the first step fails for V and we interpret it as a new variable without any "ambiguous name" warning.

In many cases however, a warning about unused imports will be reported for use E1::*; and use E2::*;, but this happens only if the imports aren't actually used anywhere else and the warnings are not always noticed (see #46078).

Two possible solutions are:

  • Issue some "did you mean the ambiguous import, not new variable?" warning for V in match.
  • Make "import ambiguity items" actual items, affecting not only diagnostics, but name resolution behavior in general, including disambiguation for fresh bindings. This alternative may be better for resolving item_like_imports: Can "ambiguity error" items be reexported? #36837.
@petrochenkov petrochenkov added A-resolve Area: Path resolution C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Nov 18, 2017
@arielb1
Copy link
Contributor

arielb1 commented Nov 18, 2017

@petrochenkov

I don't like the look of either this or #36837 - import resolution should be a "least fixed point" algorithm, and for that more ambiguity always needs to introduce more ambiguity.

@petrochenkov
Copy link
Contributor Author

Treatment of ambiguity items was changed in #57199.
Now they do not result in Def::Err and are mostly treated as their primary item (modulo reporting ambiguity errors), this corresponds to the second of the "possible solutions".

This means that the original reproducer now reports an error because V in the pattern is treated as a variant E1::V.

The only case that wasn't fixed is the case with a primary item being shadowable by fresh binding (only DefKind::Fn items have this property).
#70006 fixes it as well.

@petrochenkov petrochenkov removed their assignment Mar 14, 2020
@bors bors closed this as completed in d986a70 Mar 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-resolve Area: Path resolution C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants