Prefer accessible paths in 'use' suggestions

[da-x]

This PR addresses issue #26454, where use suggestions are made for paths that don't work. For example:

mod foo {
    mod bar {
        struct X;
    }
}

fn main() { X; } // suggests `use foo::bar::X;`

[rust-highfive]

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

@da-x
Could you explain the used heuristic?
I don't understand what the in_reference conditions attempt to achieve exactly?

[da-x]

@petrochenkov First, reference is the source location that caused the error, where the missing name is, that is - where the user is.

Case 1

For is_visible_to_user, we don't care whether pub mod foo or mod foo in the next example, because visibility only matters for a user outside the current module:

pub mod foo { // Or `mod foo {`
    pub struct Bla;
}

fn main() {
    let x = Bar;
}

That's why in_reference || name_binding.vis == ty::Visibility::Public controls recursion. It's only on the first inner module that we start to care about visibility. For example, if we had:

mod extra_layer {
    mod foo { // Now `pub mod foo` or `mod foo` matter for `use` potential.
        pub struct Bar;
    }
}

fn main() {
    let x = Bar;
}

Case 2

Old code skips all imports in the in_module.for_each_child block. In general we don't want to skip imports because then we miss re-exported private items. e.g. crate::foo::bar::XinBar can be private while use crate::foo::XinBar; maybe be valid to due a public re-export of it, and we want the user to know.

What I did there, is to only skip imports that are at the reference, i.e. in_reference && name_binding.is_import() && !name_binding.is_extern_crate(). However while trying to explain this I figured that in_reference && in that place needs to move to be !in_reference in the collect results based on the filter function bit. I.e. we want to recourse into the imports, but we don't want to add weird suggestions like the following: use ItemUse;, which is what would otherwise happen (and it did, see your commit d1310dc).

I'm adding this as an extra commit. See mixed-site-span.stderr and lexical-scopes.stderr that were positively affected.

It would be probably a good idea to add more tests for this, and have them both work with edition 2015 and edition 2018.

[petrochenkov]

@da-x
I see, thanks for the explanation!

#72789 shows how to pass the "reference module" in a better way and how to compare modules without spans.

Once that PR lands, could you split this PR into two PRs, one for the "suggest reexports" heuristic and one for the privacy check heuristic, so their effects on tests could be more clear.

[bors]

☔ The latest upstream changes (presumably #72975) made this pull request unmergeable. Please resolve the merge conflicts.

[petrochenkov]

#72789 has landed.

[da-x]

Yes, noticed :) Basing on this, I'm rewriting my original changes - the newer version is going to have much better test results.

…

On Thu, Jun 11, 2020, 20:35 Vadim Petrochenkov ***@***.***> wrote: #72789 <#72789> has landed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#72623 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACON6PGBSDIEDGJBAXWBADRWEIWLANCNFSM4NK65ZGQ> .

[da-x]

@petrochenkov rewrite is finished.

[da-x]

The move to use is_ancestor_of made it possible to clean up predicates further inside the loop. Also, the is_extern checks were cleaned up in a subsequent commit, as ancestry checks makes them redundant as well.

[petrochenkov]

r=me after squashing the commits.

[da-x]

Done. Also, consider for a follow-up PR, maybe to fix the following:

mod foo {
    mod bar {
        pub struct X;
    }
    pub use self::bar::X;
}

fn main() { X; }

This still suggests use foo::bar::X; instead of use foo::X. Fixing this will require not skipping imports in the in_module.for_each_child loop.

[da-x]

r? @petrochenkov

[petrochenkov]

Yeah, not skipping imports would be useful, and it can be done as a separate PR.
@bors r+

[bors]

📌 Commit fea5ab1 has been approved by petrochenkov