Skip to content
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

feat: Bool to enum assist #15484

Merged
merged 10 commits into from Sep 22, 2023
Merged

Conversation

rmehri01
Copy link
Contributor

This adds the bool_to_enum assist, which converts the type of boolean local variables, fields, constants and statics to a new enum type, making it easier to distinguish the meaning of true and false by renaming the variants.

Closes #14779

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2023

/// Replaces all usages of the target identifier, both when read and written to.
fn replace_usages(edit: &mut SourceChangeBuilder, usages: &UsageSearchResult) {
for (_, references) in usages.iter() {
Copy link
Contributor Author

@rmehri01 rmehri01 Aug 19, 2023

Choose a reason for hiding this comment

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

I think it would be nice to handle cross file and module references too, especially in the case of constants. I had a version before where I would just check to see if the ctx file is different than the usage file, then I would insert an import to the new enum, which seemed to work out alright for basic files but this doesn't handle modules within a file so I was wondering if there is another approach that would be nicer?

Copy link
Contributor Author

@rmehri01 rmehri01 Aug 20, 2023

Choose a reason for hiding this comment

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

Maybe handling a test like this could be nice?

    #[test]
    fn const_in_module() {
        check_assist(
            bool_to_enum,
            r#"
fn main() {
    if foo::FOO {
        println!("foo");
    }
}

mod foo {
    pub const $0FOO: bool = true;
}
"#,
            r#"
use foo::Bool;

fn main() {
    if foo::FOO == Bool::True {
        println!("foo");
    }
}

mod foo {
    #[derive(PartialEq, Eq)]
    pub enum $0Bool { True, False }

    pub const FOO: Bool = Bool::True;
}
"#,
        )
    }

Copy link
Contributor Author

@rmehri01 rmehri01 Aug 21, 2023

Choose a reason for hiding this comment

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

Two approaches may be:

  1. Use ctx.sema.scope to check if the definition and usage are in different modules and then do the appropriate insert_use.
  2. Use is_ methods for each of the variants, as done in generate_enum_is_method so that there are no imports needed.

Copy link
Member

Choose a reason for hiding this comment

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

Handling it cross modules properly would be great yes, you might be able to check on crates\ide-assists\src\handlers\extract_struct_from_enum_variant.rs how things are done there. Iirc it needs to import stuff as well when rewriting other files

Copy link
Contributor Author

@rmehri01 rmehri01 Sep 9, 2023

Choose a reason for hiding this comment

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

I see, thanks for the pointer! I think I've got it to work in a similar way now using the import approach. The only problem is I can't seem to get the tabstop snippet to show up in the right place because of the added import, so I've just removed the tabstop for now.

crates/ide-assists/src/handlers/bool_to_enum.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/bool_to_enum.rs Outdated Show resolved Hide resolved

/// Replaces all usages of the target identifier, both when read and written to.
fn replace_usages(edit: &mut SourceChangeBuilder, usages: &UsageSearchResult) {
for (_, references) in usages.iter() {
Copy link
Member

Choose a reason for hiding this comment

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

Handling it cross modules properly would be great yes, you might be able to check on crates\ide-assists\src\handlers\extract_struct_from_enum_variant.rs how things are done there. Iirc it needs to import stuff as well when rewriting other files

crates/ide-assists/src/handlers/bool_to_enum.rs Outdated Show resolved Hide resolved
@Veykril 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 Sep 8, 2023
@bors
Copy link
Collaborator

bors commented Sep 8, 2023

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

@rmehri01
Copy link
Contributor Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2023
crates/ide-assists/src/handlers/bool_to_enum.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/bool_to_enum.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/bool_to_enum.rs Outdated Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented Sep 22, 2023

Thanks! Really looking forward to using this assist 🎉
@bors r+

@bors
Copy link
Collaborator

bors commented Sep 22, 2023

📌 Commit 93562dd has been approved by Veykril

It is now in the queue for this repository.

@Veykril Veykril changed the title Bool to enum assist feat: Bool to enum assist Sep 22, 2023
@bors
Copy link
Collaborator

bors commented Sep 22, 2023

⌛ Testing commit 93562dd with merge df75809...

@bors
Copy link
Collaborator

bors commented Sep 22, 2023

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

@bors bors merged commit df75809 into rust-lang:master Sep 22, 2023
10 checks passed
@rmehri01 rmehri01 deleted the 14779_bool_to_enum_assist branch October 2, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assist idea bool to enum
4 participants