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 review

[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