resolve: allow super in module in block to refer to block items

[davidhewitt]

This PR attempts to resolve #79260 by changing the way the resolver interprets super in mod items that are children of blocks.

Most notably, this means that doctests with modules which use other items from the snippet can now compile:

//! ```rust
//! struct Foo;
//!
//! mod foobar {
//!     use super::*;
//!
//!     fn bar() -> Foo { Foo }
//! }
//!
//! ```

At the moment this doctest will fail to compile. (playground)

Background

At the moment, when an inline module is inside a block, the super keyword refers to the parent module of the block. The result of this is that the inline module has no way to refer to its sibling items inside the block.

fn foo() {
    const FOO: u64 = 0;
    mod foo {
        pub use super::FOO; //~ ERROR no `Foo` in the root
    }
}

(playground)

The current interpretation of the super keyword is theoretically pure, however I want to argue it has practical drawbacks. As well as issues with doctests as per the first example, it means that in general code of the following form is interpreted differently depending on whether it is inside a block:

const FOO: u64 = 0;
mod foo {
    pub use super::FOO;
}

In isolation it seems fair to me to assume that most readers would state this code compiles and that foo::FOO can resolve. However, if this code is actually inside a block, it doesn't compile.

Proposed Design

This PR modifies the behaviour of the super keyword in examples like the above such that usage inside a module which is a child of (one of more) blocks will attempt to resolve items from those blocks as well as the parent module of those blocks.

Ambiguity is resolved by always preferring items that are closer to the module in the hierarchy. E.g. in the following example:

const FOO: u64 = 0;
fn main() {
    const FOO: u64 = 1;
    mod foo {
        pub use super::FOO;
    }
}

the item foo::FOO will resolve to have the value 1.

⚠️ Breakage ⚠️

This example just above is unfortunately also an example where current code could break under these changes. Today the compiler would not see the FOO constant which is the child of main from its sibling mod foo, and so the item foo::FOO today would instead resolve to have the value 0.

(Assuming, of course, that the snippet just above were not itself wholly inside a block, in which case the compiler today would not consider the FOO on the first line either 🙃)

I would argue inline modules inside blocks are rare. (I myself only found this case while testing a macro which created a module and discovering it did not compile when inside a block.) So this breakage is not likely to be widespread. Nevertheless we should assess carefully.

If we were to take a conservative approach it should be possible to change this PR to be backwards compatible. I'd propose in that case to push the complete change to the proposed design in this PR via an edition boundary, with a lint to warn of cases that will change.

Alternatives

I'm quite happy to throw this PR away in favour of an alternative design. I'll try to list them here as I see them:

• @petrochenkov has proposed that a #[transparent] mod foo could behave as if it did not create a name boundary, which would allow these modules in blocks to share scope with their parent block. (See Cannot refer to item inside function from module inside function #79260 (comment))
• @Aaron1011 wrote a pre-RFC on a fn modifier in use statements. (See https://internals.rust-lang.org/t/pre-rfc-add-fn-path-qualifier/10900)
• Also in that thread was the proposal of plain use;. I'd personally prefer it was a new keyword e.g. use scope::*; or use scope::foo; - that probably needs a new edition though as a new keyword.

TODO

• I think it's quite likely a change like this could be worthy of a wider design review. Please feel free to ask me to first present this as an RFC. (I started with the implementation so I could learn about this design and play with it.)
• Feature gate? (I 100% assume this change will need one.)
• Make a decision on the backwards-compatibility story, especially if it might want to become part of the 2021 edition.
• Refactoring in the implementation of this PR - at the moment I kept to the most direct implementation I could find because I'm not familar with the rustc_resolve codebase. Please be unafraid to encourage me to rewrite this PR multiple times to get the cleanest result!

[rust-highfive]

r? @eddyb

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

[Aaron1011]

I made a Pre-RFC for adding a new fn path qualifier to the language, which would allow fixing this without a breaking change: https://internals.rust-lang.org/t/pre-rfc-add-fn-path-qualifier/10900/1
However, I didn't end up opening an RFC.

[davidhewitt]

Thanks, that's a really interesting thread which I'd totally missed at the time. Explains why I couldn't find much about this on the Rust issue tracker!

The fn modifier is an interesting proposal; if I were to add my two cents though I'd prefer it was use scope::super::foo; or use(scoped) super::foo;. Because this affects modules-in-blocks just as much as modules-in-functions.

The other proposal I saw in that thread was plain use; to pull in all items from the parent scope. I think this is semantically equivalent to @petrochenkov 's #[transparent] proposal. I'm not sure which is the better of the two.

use; on its own does seem a bit magical. With a new keyword e.g. use scope::*; it's quite clear what's going on. I think I'd personally prefer use scope::*; over a #[transparent] attribute because I think transparent already has good meaning in #[repr(transparent)] and I wouldn't want to overload it here.

I've added these to the alternatives above.

[oli-obk]

r? @petrochenkov

[petrochenkov]

I don't this is a desirable change, regardless of breakage.
It brings inconsistencies to the language and fits poorly into the existing system.
Marking as waiting on lang team.

[davidhewitt]

👍 I'm very happy to close this PR and try a different implementation depending on direction from the lang team.

[bors]

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

[joshtriplett]

This seems potentially reasonable to me, and the rationale makes sense. I have a few questions, which I don't see answered here:

• This is the first time the "scope" referred to by a qualified path can be the inside of a function, where other kinds of declarations can exist. Could you add a test to make sure that, for instance, let x: i32 = 42; mod y { ... x ... } doesn't work, or any other reference to a local declaration that couldn't appear outside a function?
• Can we do a crater run to see if this leads to any breakage in practice? If this passes a full crater run, I think it might be reasonable. If it triggers any real-world breakage, then I'm not sure it's worth the transition/migration cost.
• @petrochenkov Can you say more about your concerns, please? To me, this seems like it removes an inconsistency, by making code work the same way whether written inside or outside of a function.

[bors]

☀️ Try build successful - checks-actions
Build commit: ea25d69b4f541a9b667f37fb212716386b7f9a54 (ea25d69b4f541a9b667f37fb212716386b7f9a54)

[petrochenkov]

@joshtriplett

Can you say more about your concerns, please? To me, this seems like it removes an inconsistency, by making code work the same way whether written inside or outside of a function.

Some language features simply work at module granularity, otherwise it would not make sense for modules to exist.

Privacy works at module granularity - you can have a pub(super) item restricted to the parent module, or pub(crate) item restricted to the current crate, but you cannot have items with visibility restricted to specific functions or blocks.

Paths work at module granularity, unnamed blocks or functions do not participate in paths like a::b::c, all of the components are modules (unless it's an associated item and the path is type-relative).
That applies to self and super, both are well defined as path components that refer to the current module and the parent module, the code already behaves the same way regardless of whether it's written inside or outside of a function.
I simply suggest not to break this existing internal logic.

Would you want to make self more fine-grained as well, so it refers to the current block rather than module? Or would it become inconsistent with super instead?

Do you have any examples of other languages where you can refer to outer unnamed scopes (like blocks) by name?
From what I remember at least from C-like languages (including Java), paths in them are also exist at "coarse grained" namespace/package level, and if you shadowed or otherwise lost some name in an outer unnamed scope, then it's indeed lost.

[joshtriplett]

@petrochenkov That's a compelling argument. In particular, I appreciate your observation about paths: there's a valuable property that if you're in a::b::c, super:: is a::b.

I want to reiterate that while I think there may be some value in improving this particular case, it also seems like a sufficiently special-case scenario that it isn't worth adding substantial complexity for. I'd like to explore possible solutions, but I do think we should set a high bar for adopting one of them.

As far as I can tell, the scheme proposed by the PR would say that if you're inside a module that's nested inside a function, when you go up via super, your scope is logically inside that function; thus, just like code written directly in that function.

In other words, with this PR, super would have the effect of "resolve this from the perspective of wherever my mod statement is".

That seems like a relatively understandable property from a language perspective. That said, from a compiler perspective, is that utterly at odds with how name/path resolution works?

[joshtriplett]

I'm going to go ahead and run this through craterbot, and if this causes any errors at all, I think we probably wouldn't want to pursue it further.

@craterbot check

[craterbot]

👌 Experiment pr-79309 created and queued.
🤖 Automatically detected try build ea25d69b4f541a9b667f37fb212716386b7f9a54
⚠️ Try build based on commit ec7f8d9, but latest commit is 68b9cdf. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[Havvy]

I don't like the proposal as is, for basically the same reasons as @petrochenkov. I'd like to see it be explicit. I think making a path separator is right out (e.g. I postulated super::fn::) but howabout a parenthetical value on super? super(item)::

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting.

The general sentiment was that we don't want to further complicate name resolution in this way, for this use case.

We'd be open to hearing proposals for alternative solutions for the same use case that don't complicate name resolution or involve a potentially breaking transition. You may wish to take detailed discussions of potential alternatives to Zulip or Internals. We'd suggest proposing a new RFC for that.

@craterbot abort
@rfcbot close

[rfcbot]

Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

[craterbot]

🗑️ Experiment pr-79309 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[cramertj]

Specifically, I dislike the idea that super::... inside an inline mod would not match the behavior of self::.... outside of that module. In functions today, self::... refers to items at the module scope, not inside the function, so it seems inconsistent that inline modules inside of a function would be able to refer to the function scope with super::....

[rfcbot]

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

[davidhewitt]

Thanks to all in this thread (and the lang team) for discussing this.

I think your concerns with this approach are valid. In particular I think that if we change super as this PR does, the logical conclusion following @petrochenkov and @cramertj 's points are that we should also modify self to act similarly. However I fully agree that changing the behaviour of self is highly undesirable, and so I think it's right to abandon this PR in the current form.

Of the alternatives presented (#[transparent] attribute on module, or some explicit alternative use / path syntax), my preference would be to have a parenthetical qualifier on the keywords: use super(scope)::{...}; and use self(scope)::{...};.

While explicit solutions are likely better, it might also be nice if an eventual solution resolved the doctest case presented in the OP without requiring users to be aware that doctests expand inside functions. These two goals are probably at odds.

I'll continue to ponder and write a pre-RFC on internals if I settle on something I like.

[rfcbot]

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.