Tracking issue for changes to name resolution (RFC 1560)

[nikomatsakis]

Tracking issue for rust-lang/rfcs#1560.

[nrc]

Nominating for stabilisation in the next cycle

[nrc]

There are potential breaking changes around importing more names in globs, e.g., in super::*, because we now import all accessible names, not just the public ones. We should do a Crater run before stabilisation. This is hard to do a warning cycle for. We should advertise the change during stabilisation. I feel like the benefits are so obvious and the breaking change so technical that it should be an easy sell.

[jseyfried]

More specifically, without #[feature(pub_restricted)] there is breakage if and only if

• a glob import of an ancestor module (e.g. use super::*) imports a non-pub name,
• the name is also imported by another glob import (making it ambiguous), and
• the name is used (triggering an ambiguity error).

If the name is not used to resolve other imports, then it is simple to emit a warning instead of a hard ambiguity error. I believe this is likely to avoid a large fraction of any breakage in practice.

To avoid the remaining breakage, we would probably need to try re-resolving the crate without item_like_imports whenever there are ambiguity errors. This is doable, but it is fairly invasive and interacts badly with macro modularization.

[nikomatsakis]

Can we get started on crater runs now? If we can't do a warning cycle, we should be aggressive about submitting PRs to affected crates to repair problems. (Also, we should investigate whether we can give good errors that link users to an issue explaining what changed and how to fix, even if they are still getting an error.)

[nrc]

Sure, I was thinking it was pretty early in the cycle and more breakage could appear later, but I guess there is no harm in doing multiple crate runs.

@jseyfried would you be able to make a PR to un-feature-gate for a Crater run please?

[jseyfried]

@nikomatsakis

If we can't do a warning cycle

A partial warning cycle is fairly simple and a full warning cycle is possible (see #35120 (comment)).

we should investigate whether we can give good errors that link users to an issue explaining what changed and how to fix

The errors we give today already include a suggestion for a fix, e.g. this gist. I could make the suggestion more detailed/concrete or improve the error in other ways -- let me know if you have ideas.

[aturon]

@nrc, we're moving these days to using @rfcbot for FCP proposals -- I'll flag it.

@rfcbot fcp merge

[rfcbot]

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

• @aturon
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @aturon, I wasn't able to add the final-comment-period label, please do so.

[rfcbot]

The final comment period is now complete.

[bstrie]

I see that the FCP is complete, but this issue is still open. What's the status? Are we keeping this open pending a deprecation period?

[jseyfried]

RFC 1560 was stabilized in #37127 (currently in beta).

[shepmaster]

This code currently fails in stable, but compiles in beta and nightly. I do not think it should:

struct Outer;

mod foo {
    use super::Outer;
    
    mod test {
        use super::*;
        
        struct Dummy(Outer);
    }
}

fn main() {}

Since use super::Outer; does not re-export Outer from foo (it should require pub to do so), I don't believe that foo::test::super::* should include Outer.

I've skimmed through the changes to name resolution rules section, but don't see what this would match up with. Anyone care to point me in the right direction?

[petrochenkov]

@shepmaster

I've skimmed through the changes to name resolution rules section, but don't see what this would match up with.

https://github.com/rust-lang/rfcs/blob/master/text/1560-name-resolution.md#non-public-imports + https://github.com/rust-lang/rfcs/blob/master/text/1560-name-resolution.md#glob-imports-of-accessible-but-not-public-names

[shepmaster]

@petrochenkov Thank you. "non-public imports" appears to be the piece I was missing; I was aware of the glob change. I guess I missed it when skimming because it uses a sibling module, not the nested module that my brain was primed for.