Skip to content

fix: #6558 don't try to repair invalid self-imports. #6573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 11, 2025

Conversation

MarcusGrass
Copy link
Contributor

@MarcusGrass MarcusGrass commented Jun 8, 2025

Fix #6558

Current attempts break idempotence.

As the tests show, paths ending with self are now untouched, this PR just removes some code (and adds/modifies some tests).

A potential change here would be to keep this part of the code:

UseSegmentKind::Slf(None) if self.path.is_empty() && self.visibility.is_some() => {
    self.path = vec![];
    return self;
}

In that case

use self;

would be removed, but not:

use self::self;

Which woud also fix the idempotence issue, but formatting away use self;. Although maybe it's best that the compiler inform the user that they're doing something wrong instead of 'sweeping it under the rug' so to say.

@MarcusGrass MarcusGrass force-pushed the mg/6558-fix-remove-self-imports branch from 3b3fc2f to 831c2bd Compare June 9, 2025 07:35
src/imports.rs Outdated
Comment on lines 567 to 572
// Normalise foo::self -> foo.
if let UseSegmentKind::Slf(None) = last.kind {
if !self.path.is_empty() {
return self;
}
}
Copy link
Contributor

@ytmimi ytmimi Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like to normalize foo::self -> foo. Maybe we can special case this to not normalize self::self -> self to prevent idempotent issues in this really obscure case.

src/imports.rs Outdated
Comment on lines 560 to 563
UseSegmentKind::Slf(None) if self.path.is_empty() && self.visibility.is_some() => {
self.path = vec![];
return self;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it this is where we get rid of use self;? I don't think there's any harm in keeping this behavior as long as we handle use self::self in a way that's idempotent.

@MarcusGrass MarcusGrass force-pushed the mg/6558-fix-remove-self-imports branch from 831c2bd to 935a355 Compare June 10, 2025 19:05
@MarcusGrass MarcusGrass requested a review from ytmimi June 10, 2025 19:06
@MarcusGrass
Copy link
Contributor Author

A lot fewer changes to behaviour was caused this time around

@ytmimi
Copy link
Contributor

ytmimi commented Jun 10, 2025

Will take another look at this later this evening. Thanks for applying the feedback so quickly

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see that we're able to remove use self;, while special casing the use self::self; scenario.

@ytmimi
Copy link
Contributor

ytmimi commented Jun 11, 2025

Maybe in a future style_edition we could consider removing the trailing self::self::self... with a patch similar to this:

diff --git a/src/imports.rs b/src/imports.rs
index cfc2f3bd..9f30ac09 100644
--- a/src/imports.rs
+++ b/src/imports.rs
@@ -565,10 +565,18 @@ impl UseTree {
         }
 
         // Normalise foo::self -> foo.
+        // Normalise bar::self::self -> bar.
+        // Remove self::self.
         if let UseSegmentKind::Slf(None) = last.kind {
-            if !self.path.is_empty() {
-                return self;
+            while let Some(UseSegmentKind::Slf(_)) = self.path.last().map(|segment| &segment.kind) {
+                self.path.pop();
+            }
+
+            if self.path.is_empty() {
+                self.path = vec![];
             }
+
+            return self;
         }
 
         // Normalise foo::self as bar -> foo as bar.

@ytmimi ytmimi merged commit 334670e into rust-lang:master Jun 11, 2025
26 checks passed
@ytmimi ytmimi added release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Jun 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Needs an associated changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use self; gets entirely deleted
3 participants