Replacing self overwriting with proper resolution

[mu001999]

View all comments

As a follow-up PR to #146972 (step 1), after this PR:
1. Trailing self can appear in paths (as the consensus in #146972 (comment)) (in future)
2. E0429 will be no longer emitted, use ...::self [as target]; will be equivalent to use ...::{self [as target]}; (in future)
3. Things like struct S {}; use S::{self as Other}; will be rejected

---

This PR used to add a new lint redundant_self, which would lint use ...::self [as target]; and use ...::{self [as target]};, and the last commit fixes all warnings emitted by this lint.

But this lint and clippy lint unnecessary_self_imports have some overlap. And use std::io::self; is not equivalent to use std::io in fact for now, the new lint will also cause the following known issue:

Removing ::{self} will cause any non-module items at the same path to also be imported. This might cause a naming conflict (rust-lang/rustfmt#3568).

So I removed this lint, and I think what it does should be done by extending the clippy lint unnecessary_self_imports.

r? petrochenkov

[petrochenkov]

It looks like #146972 (comment) suggests to support trailing self in all paths, not just import paths. We can start with import paths if that's more convenient though.

However, if we are doing it, then I think it's time to abandon the whole "self rewriting" hack, and just properly resolve self inside modules.

[mu001999]

It looks like #146972 (comment) suggests to support trailing self in all paths, not just import paths.

Semantics of trailing self in import paths, like use foo::self;, is explicit and same to use foo::{self};, and this will require foo is in type namespace. So I think trailing self in import paths is usefull to replace use foo::{self}; when we want to import only items in type namespace.

But what will trailing self in non-import paths be? Do we need things like Foo::self? 🤔

[mu001999]

@rustbot author

[mu001999]

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[theemathas]

This code currently compiles in stable rust:

struct Thing;
use Thing::{self as Alias};

If this code will stop compiling with this PR, then this PR needs a crater run.

[petrochenkov]

Yes, we'll certainly run crater.

[petrochenkov]

Let's perhaps split this into 2 steps.

• Replacing self overwriting with proper resolution.
• Relaxing the rules to allow trailing self in more contexts.

The first part may require a new deprecation lint for the use Struct::{self} cases, we'll see after running crater.
(All the test additions may go into the first part as usual.)
@rustbot author

[mu001999]

@rustbot review

[petrochenkov]

Could you limit this PR to just the first step from #152996 (comment)?
Remove the overwriting, add the ident.name == kw::SelfLower case to resolve_ident_in_module, and then do just the bare minimum to keep old code compiling while preventing any new code from compiling.
@rustbot author

[mu001999]

@rustbot review

[petrochenkov]

Thanks!
@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 2c4819a (2c4819a380dbddb1f2a0f4405a49fb6ed90983ae, parent: 3b1b0ef4d80d3117924d91352c8b6ca528708b3c)

[blyxyas]

Is it safe to remove the T-Clippy label?

[mu001999]

Is it safe to remove the T-Clippy label?

I think it is now :)

[petrochenkov]

@craterbot check

[craterbot]

👌 Experiment pr-152996 created and queued.
🤖 Automatically detected try build 2c4819a
🔍 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

[craterbot]

🚧 Experiment pr-152996 is now running

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

[craterbot]

🎉 Experiment pr-152996 is completed!
📊 12 regressed and 1 fixed (847762 total)
📊 2473 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-152996/retry-regressed-list.txt ↩

[petrochenkov]

9 non-spurious regressions:

• Ilegal-Beagle.rust-chat.a6ddebdcf92bec7ee885fc3064179de991070c0c - use crate::App::{self}; // error: App is a struct, not a module
• JamesZoft.sokoban_rust.d5473f163b089ceff523977cfab0912074c68924 - Terminal::{self}, // error: Terminal is a struct, not a module
• meshya.mesh.9ba400fda5f399bf423606d3b9ab649878ca0649 - use curl::easy::{Easy::{self}}; // error: Easy is a struct, not a module
• michaelblawrence.together-rs.f83ac31916198cdc93dd5fb4b1a132e1a5dad51d - pub use subprocess_impl::SbProcess::{self as Process}; // error: SbProcess is a struct, not a module
• sukkis.emed.0896e2d0d8dcdb3005ca6961eae8cbaa13946dc0 - use crate::Theme::{self}; // error: Theme is a struct, not a module
• violeshnv.tetris.e8fcc939b590d12300d031c6781c1735672b542e - KeyModifiers::{self as Modifiers}, // error: KeyModifiers is a struct, not a module
• carbon-now-0.0.1 - use syntect::highlighting::Color::{self as SyntectColor}; // error: Color is a struct, not a module
• ramu_rs-0.1.1 - SimpleMemory::{self}, // error: SimpleMemory is a struct, not a module
• together-rs-0.4.0 - pub use subprocess_impl::SbProcess::{self as Process}; // error: SbProcess is a struct, not a module

[petrochenkov]

Description for lang team

This PR implements the first part of the lang team guidance from #146972 (comment).
It migrates from treating self in use foo::{self [as bar]} as a part of braced import { ... } syntax (#146972 (comment)) to treating it as a part of the path foo::self and resolving it properly.

It results in minor breakage as seen above - #152996 (comment).
Currently the invariant is that any segment of an import path, except for the last, always resolves to a module (in nameres sense, including enums and traits).
So things like MyStruct::self break, because MyStruct is not a module, we had a FIXME about this in tests/ui/resolve/resolve-bad-import-prefix.rs, but no open issues, AFAIK.
Technically we can add a hack to support this, but I'd prefer not to do it, self only has meaning in module contexts in the current model.

[joshtriplett]

Some cases that came up in discussion in the @rust-lang/lang meeting:

use some_module::SomeEnum::{self, Variant1, Variant2};
use some_module::SomeEnum::{self as MyEnum, Variant1, Variant2};

There was a general feeling that those should work.

Is this what you mean by "always resolves to a module (in nameres sense, including enums and traits)"?

[theemathas]

@joshtriplett My reading is that MyStruct::{self} will break with this PR, but MyEnum::{self} or MyTrait::{self} will continue to work.

[joshtriplett]

There's been a long-standing request from @rust-lang/libs-api to support use of things like associated constants, and lang has a desire to support that in the future.

Would making this change make that much harder in the future, or just take an already-hard thing and make it marginally harder?

(In any case, this shouldn't be taken as precedent for "we will never ever want a struct to act as a module for nameres purposes", even if we end up making this particular proposed change for now.)

[petrochenkov]

Is this what you mean by "always resolves to a module (in nameres sense, including enums and traits)"?

Yes, enums and traits are modules in name resolution sense, so use some_module::SomeEnum::{self} will continue to work.

There's been a long-standing request from rust-lang/libs-api to support use of things like associated constants, and lang has a desire to support that in the future.
Would making this change make that much harder in the future, or just take an already-hard thing and make it marginally harder?

I don't think there's much connection.
Supporting use MyStruct::AssocItem; is still equally hard, but supporting it will break the "prefix is a module" invariant too, that's true. (MyStruct::self is not an associated item, so supporting it will require a separate special code path.)