handle {self} when removing unused imports #17140

harrysarson · 2024-04-25T14:32:39Z

On master

mod inner {
    pub struct X();
    pub struct Y();
}

mod z {
    use super::inner::{self, X}$0;

    fn f() {
        let y = inner::Y();
    }
}

becomes

mod inner {
    pub struct X();
    pub struct Y();
}

mod z {
    use super::inner:self;

    fn f() {
        let y = inner::Y();
    }
}

with this fix it instead becomes


```rs
mod inner {
    pub struct X();
    pub struct Y();
}

mod z {
    use super::inner;

    fn f() {
        let y = inner::Y();
    }
}

Veykril · 2024-04-26T11:17:59Z

That transform is generally not valid, use foo::bar::{self} imports only the type namespace, where as use foo::bar; imports value, type and macro namespace. So transforming like that can actually cause name collisions.

Now there is still a bug here in that we produce malformed syntax but we need to keep the braces.

Veykril · 2024-04-26T11:18:12Z

crates/syntax/src/ast/node_ext.rs

@@ -376,11 +376,24 @@ impl ast::UseTreeList {
            .filter_map(|it| it.into_token().filter(|it| it.kind() == T![,]))
    }

+    pub fn is_just_self(&self) -> bool {
+        if let Some((single_subtree,)) = self.use_trees().collect_tuple() {
+            fn path_is_self(path: &ast::Path) -> bool {


This can be a closure

I looked to make this into a closure, and in the process decided to check the number of use_tree children as part of the same check (so now the closure is called has_single_subtree_that_is_not_self)

harrysarson · 2024-04-30T17:06:54Z

That transform is generally not valid, use foo::bar::{self} imports only the type namespace, where as use foo::bar; imports value, type and macro namespace. So transforming like that can actually cause name collisions.

Now there is still a bug here in that we produce malformed syntax but we need to keep the braces.

Yep. I have removed the second commit, the first commit (which fixes remove_unnecessary_braces) I think is what we need to fix the malformed syntax and keep the braces.

Before this commit `UseTree::remove_unnecessary_braces` removed the braces around `{self}` in `use x::y::{self};` but `use x::y::self;` is not valid rust.

harrysarson · 2024-04-30T17:24:26Z

That transform is generally not valid,

The normalize import IDE assist (https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-assists/src/handlers/normalize_import.rs) does transform {self}. I.e. the following unit test would pass if I added it to normalize_import.rs

    #[test]
    fn test_collapses_self() {
        check_assist_variations!("std::fmt::{self}", "std::fmt");
    }

Is that a bug?

Veykril · 2024-05-23T08:25:10Z

Unless the assist there checks that fmt only refers to a type namespace there I'd consider that a bug

Veykril · 2024-05-23T08:28:46Z

Thanks!
@bors r+

bors · 2024-05-23T08:28:49Z

📌 Commit a653ddf has been approved by Veykril

It is now in the queue for this repository.

bors · 2024-05-23T08:30:13Z

⌛ Testing commit a653ddf with merge e3e22c6...

bors · 2024-05-23T08:41:05Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing e3e22c6 to master...

It changes behaviour and can cause collisions. E.g. for the following snippet ```rs mod foo { pub mod bar {} pub const bar: i32 = 8; } // tranforming the below to `use foo::bar;` causes the error: // // the name `bar` is defined multiple times use foo::bar::{self}; const bar: u32 = 99; fn main() { let local_bar = bar; } ``` we still normalize ```rs use foo::bar; use foo::bar::{self}; ``` to `use foo::bar;` because this cannot cause collisions. See: rust-lang#17140 (comment)

do not normalize `use foo::{self}` to `use foo` It changes behaviour and can cause collisions. E.g. for the following snippet ```rs mod foo { pub mod bar {} pub const bar: i32 = 8; } // transforming the below to `use foo::bar;` causes the error: // // the name `bar` is defined multiple times use foo::bar::{self}; const bar: u32 = 99; fn main() { let local_bar = bar; } ``` we still normalize ```rs use foo::bar; use foo::bar::{self}; ``` to `use foo::bar;` because this cannot cause collisions. See: #17140 (comment)

It changes behaviour and can cause collisions. E.g. for the following snippet ```rs mod foo { pub mod bar {} pub const bar: i32 = 8; } // tranforming the below to `use foo::bar;` causes the error: // // the name `bar` is defined multiple times use foo::bar::{self}; const bar: u32 = 99; fn main() { let local_bar = bar; } ``` we still normalize ```rs use foo::bar; use foo::bar::{self}; ``` to `use foo::bar;` because this cannot cause collisions. See: rust-lang#17140 (comment)

It changes behaviour and can cause collisions. E.g. for the following snippet ```rs mod foo { pub mod bar {} pub const bar: i32 = 8; } // tranforming the below to `use foo::bar;` causes the error: // // the name `bar` is defined multiple times use foo::bar::{self}; const bar: u32 = 99; fn main() { let local_bar = bar; } ``` we still normalize ```rs use foo::bar; use foo::bar::{self}; ``` to `use foo::bar;` because this cannot cause collisions. See: rust-lang/rust-analyzer#17140 (comment)

do not normalize `use foo::{self}` to `use foo` It changes behaviour and can cause collisions. E.g. for the following snippet ```rs mod foo { pub mod bar {} pub const bar: i32 = 8; } // transforming the below to `use foo::bar;` causes the error: // // the name `bar` is defined multiple times use foo::bar::{self}; const bar: u32 = 99; fn main() { let local_bar = bar; } ``` we still normalize ```rs use foo::bar; use foo::bar::{self}; ``` to `use foo::bar;` because this cannot cause collisions. See: rust-lang/rust-analyzer#17140 (comment)

It changes behaviour and can cause collisions. E.g. for the following snippet ```rs mod foo { pub mod bar {} pub const bar: i32 = 8; } // tranforming the below to `use foo::bar;` causes the error: // // the name `bar` is defined multiple times use foo::bar::{self}; const bar: u32 = 99; fn main() { let local_bar = bar; } ``` we still normalize ```rs use foo::bar; use foo::bar::{self}; ``` to `use foo::bar;` because this cannot cause collisions. See: rust-lang/rust-analyzer#17140 (comment)

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2024

harrysarson force-pushed the harry-unused-self branch from c298bd6 to 7163d60 Compare April 25, 2024 14:50

Veykril reviewed Apr 26, 2024

View reviewed changes

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2024

harrysarson force-pushed the harry-unused-self branch from 7163d60 to 415e357 Compare April 30, 2024 17:04

braces around {self} in UseTree are not unnecessary

a653ddf

Before this commit `UseTree::remove_unnecessary_braces` removed the braces around `{self}` in `use x::y::{self};` but `use x::y::self;` is not valid rust.

harrysarson force-pushed the harry-unused-self branch from 415e357 to a653ddf Compare April 30, 2024 17:17

Veykril approved these changes May 23, 2024

View reviewed changes

bors merged commit e3e22c6 into rust-lang:master May 23, 2024
11 checks passed

philips-software-forest-releaser bot mentioned this pull request Jun 4, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#436

Merged

This was referenced Jun 11, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#445

Merged

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#447

Merged

philips-software-forest-releaser bot mentioned this pull request Jun 18, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#456

Merged

philips-software-forest-releaser bot mentioned this pull request Jun 25, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#469

Merged

harrysarson mentioned this pull request Jun 25, 2024

do not normalize use foo::{self} to use foo #17494

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle {self} when removing unused imports #17140

handle {self} when removing unused imports #17140

harrysarson commented Apr 25, 2024

Veykril commented Apr 26, 2024

Veykril Apr 26, 2024

harrysarson Apr 30, 2024 •

edited

Loading

harrysarson commented Apr 30, 2024

harrysarson commented Apr 30, 2024 •

edited

Loading

Veykril commented May 23, 2024

Veykril commented May 23, 2024

bors commented May 23, 2024

bors commented May 23, 2024

bors commented May 23, 2024

handle {self} when removing unused imports #17140

handle {self} when removing unused imports #17140

Conversation

harrysarson commented Apr 25, 2024

Veykril commented Apr 26, 2024

Veykril Apr 26, 2024

Choose a reason for hiding this comment

harrysarson Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

harrysarson commented Apr 30, 2024

harrysarson commented Apr 30, 2024 • edited Loading

Veykril commented May 23, 2024

Veykril commented May 23, 2024

bors commented May 23, 2024

bors commented May 23, 2024

bors commented May 23, 2024

harrysarson Apr 30, 2024 •

edited

Loading

harrysarson commented Apr 30, 2024 •

edited

Loading