Stream: t-compiler/wg-rls-2.0

Topic: need review


Coenen Benjamin (Apr 21 2020 at 07:17, on Zulip):

Hello @matklad this PR needs you https://github.com/rust-analyzer/rust-analyzer/pull/3954 šŸ„° can you provide me some information or your thoughts about my proposal ? I had also another idea, maybe I can not disable sortText when there is some characters after the dot for autocompletion. It will limit risks. What do you think ?

Coenen Benjamin (Apr 21 2020 at 15:16, on Zulip):

Thanks for the review. Can you let me know which tool do you use to make your gif demo of new features in rust-analyzer ? It's awesome

matklad (Apr 21 2020 at 15:18, on Zulip):

peek

matklad (Apr 21 2020 at 15:18, on Zulip):

(sorry for the long review roundtrips and pretty short reviews. I've been swamped as of lately. Probably will be for the forseeable future as well :) )

Coenen Benjamin (Apr 21 2020 at 15:20, on Zulip):

No worry ! Your work is incredible and I know you're very busy ! Thanks for all :)

Coenen Benjamin (Apr 21 2020 at 15:43, on Zulip):

ra_autocomplete.gif Here is the behavior with a simple example

std::Veetaha (Apr 21 2020 at 16:19, on Zulip):

Looks awesome, but I'd like to suggest to have a feature toggle for this one (if it doesn't already exit).
The tool is screencast mode:
image.png

Coenen Benjamin (Apr 22 2020 at 08:58, on Zulip):

About this PR https://github.com/rust-analyzer/rust-analyzer/pull/4043 is anyone to help me find a way to fetch tail expressions easily ? Or redirect me to an example where it's already used ?

Florian Diebold (Apr 22 2020 at 10:57, on Zulip):

I think right now we have no easy way of finding all tail expressions, so you probably have to walk through the syntax tree and handle it based on the syntax nodes. The match_ast macro could come in handy there though.

Florian Diebold (Apr 22 2020 at 11:10, on Zulip):

I imagine we might need something like it for some analyses later, but we don't have them yet

Coenen Benjamin (Apr 22 2020 at 11:16, on Zulip):

Ok thank you. I started to write a function to fetch it. Not so easy but at least I know that I'm not rewriting this instead of using an existing function.

Florian Diebold (Apr 22 2020 at 12:13, on Zulip):

although I wonder how much the assist should really go into expressions -- it's not clear whether turning if x { foo } else { bar } into if x { Ok(foo) } else { Ok(bar) } or Ok(if x { foo } else { bar }) is better :thinking:

apart from that, finding return expressions is important though

Laurențiu Nicola (Apr 22 2020 at 12:20, on Zulip):

But not return expressions in closures or inner functions. Also,

fn foo() -> i32 {
    loop {
        break 42;
    }
}
Coenen Benjamin (Apr 22 2020 at 13:29, on Zulip):

Yes already thought about that. For now I think it's better to go deeper instead of just returing Ok(if...). I'm not so far. I have some issues with pattern with a tail expression like a match for example and inside this match a return expression lol

Coenen Benjamin (Apr 22 2020 at 13:30, on Zulip):

example:

fn foo() -> i32 {
                let my_var = 5;
                match my_var {
                    5 => {
                        if true {
                            42i32
                        } else {
                            25i32
                        }
                    },
                    _ => {
                        let test = "test";
                        if test == "test" {
                            return 24i32;
                        }
                        53i32
                    },
                }
            }
Timo Freiberg (Apr 22 2020 at 13:41, on Zulip):

Not sure if this applies to this case, but I think when there are many valid results of an assist and the decision is a question of style, it makes sense to implement the easiest version and add another assist for converting between the different versions (example: generating qualified paths or adding a use statement & generating unqualified paths)

Florian Diebold (Apr 22 2020 at 14:14, on Zulip):

I think we did talk about having assists that move the Ok around as well, yeah

Coenen Benjamin (Apr 22 2020 at 16:20, on Zulip):

ok_wrapper.gif I updated the PR, here is an example :)

Jeremy Kolb (Apr 22 2020 at 16:46, on Zulip):

That looks nice!

Jeremy Kolb (Apr 22 2020 at 16:47, on Zulip):

bike shed: I think the label should be similar to "Wrap with Ok" or "Wrap return value with Ok" instead of "Make..."

std::Veetaha (Apr 22 2020 at 16:57, on Zulip):

I'd suggest "Change return type to Result"

Jeremy Kolb (Apr 22 2020 at 17:21, on Zulip):

good point

Coenen Benjamin (Apr 22 2020 at 17:29, on Zulip):

Yes for the function name and the label I was counting on you to make proposals haha

Coenen Benjamin (Apr 23 2020 at 19:24, on Zulip):

If you have time to take a look, this one https://github.com/rust-analyzer/rust-analyzer/pull/3954 is ready to review.
And also this one https://github.com/rust-analyzer/rust-analyzer/pull/4043

Coenen Benjamin (Apr 27 2020 at 14:53, on Zulip):

Hello @matklad let me know how can I help you to make the task easier for you to review this kind of PR https://github.com/rust-analyzer/rust-analyzer/pull/4043 ? Maybe I can add a better explanation about the implementation ? Let me know :)

Last update: Sep 22 2020 at 01:30UTC