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

REF: introduce constant refactoring #4985

Merged
merged 1 commit into from
May 11, 2020

Conversation

Kobzol
Copy link
Member

@Kobzol Kobzol commented Feb 17, 2020

In this PR I'd like to tackle the Introduce constant refactoring. The current state is just a sketch, because I have a few questions:

  • Should this work on arbitrary expressions (and types) or just literals?
  • Where should the corresponding constant be generated? Maybe we should show the user a short dialog (similar to the Introduce parameter refactoring) to let him select? For example here:
//x
mod x {
    //x
    fn foo() {
        //x
        fn bar() {
            //x
            let x = 5;
        }   
    }
}

We could offer these 4 locations (inner function, outer function, inside module, outside module).

  • Should literals with the same type and value in the given context (function/block) be extracted to use the newly created constant (same as with the Introduce Variable refactoring)?

Related issue: #4246

@vlad20012
Copy link
Member

vlad20012 commented Feb 17, 2020

  1. I think we can follow Java behavior. It work for expressions if they can be extracted to a constant. Otherwise it shows error message:
    image
  2. Java implementation by default uses class level, but if there are multiple classes it shows choose dialog
    image
    But I like how scope selection works in kotlin's extract veriable:
    image
    In addition to a chooser, it highlights the selection scope. Maybe it is overkill for constant extraction b/c scopes are usually large.
  3. Java implementation offers a nice dialog with options =)
    image

@vlad20012 vlad20012 self-assigned this Feb 19, 2020
@Kobzol
Copy link
Member Author

Kobzol commented Feb 19, 2020

I tried two variants of the highlighting, the first one simply selects the scope:
https://gifyu.com/image/76Sh

And the second one inserts a placeholder comment (or something else) into the corresponding place:
https://gifyu.com/image/762e

  • here obviously the places are wrong, that would be fixed
  • the comments should disappear when the user selects a different place, however they seem to be immune to .delete()

What do you think? This selection would be shown after you select whether you want to replace occurences or not.

@Kobzol
Copy link
Member Author

Kobzol commented Feb 19, 2020

Added expression selection, auto import and interactive renaming (those features should be independent of highlighting).
https://gifyu.com/image/76pa

For some reason this works in Clion, but not in tests:

let x = /*caret*/5 + 5;

const i: i32 = 5 + 5; /// IDE
const X1: _ = 5 + 5; /// tests

In tests the BinaryExpr is not inferred to be of type i32 (it's TyUnknown instead).

@Kobzol Kobzol changed the title WIP: introduce constant refactoring REF: introduce constant refactoring Mar 29, 2020
@Kobzol
Copy link
Member Author

Kobzol commented Mar 29, 2020

Rebased over master.

@Kobzol
Copy link
Member Author

Kobzol commented May 9, 2020

I fixed the tests, they needed the stdlib ProjectDescriptor.

Here's how the refactoring currently looks like: https://gifyu.com/image/76pa. @vlad20012 let me know if there are any other changes that I should make.

Copy link
Member

@vlad20012 vlad20012 left a comment

Choose a reason for hiding this comment

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

👍 LGTM in general.

I'd want to see a more informative message if the expression is not extractable
image

@vlad20012
Copy link
Member

Could you please squash these commits?

@vlad20012
Copy link
Member

👍 Thanks!
bors r+

@bors
Copy link
Contributor

bors bot commented May 11, 2020

Build succeeded:

@bors bors bot merged commit b662553 into intellij-rust:master May 11, 2020
@Kobzol Kobzol deleted the introduce-constant branch May 11, 2020 10:30
@vlad20012 vlad20012 added this to the v122 milestone May 11, 2020
@vlad20012 vlad20012 added the to be documented Pull requests that should be documented label May 11, 2020
@MarinaKalashina MarinaKalashina removed the to be documented Pull requests that should be documented label Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants