Stream: t-compiler/wg-rls-2.0

Topic: 2 assists with the same action name


Kirill Bulatov (Feb 07 2020 at 15:00, on Zulip):

pasted image

The first one is from add_import.rs and it turns the fully qualified name into an import + the end of the FQN.
The 2nd one is auto import.

I want to change this, any proposals on the naming?

Florian Diebold (Feb 07 2020 at 15:00, on Zulip):

it's weird that the 'import ast' shows up when the cursor is on cast, I think

Florian Diebold (Feb 07 2020 at 15:01, on Zulip):

on the other hand, importing ast is actually the more useful one here :thinking:

Kirill Bulatov (Feb 07 2020 at 15:10, on Zulip):

Yes, we need to auto import ast in such cases IMO, not the cast if we don't modify the string under the caret (which we don't now).

The other question is why do we propose to import the cast in the first place, that's what I tried to debug but instantly got confused by two similarly named actions :)

Florian Diebold (Feb 07 2020 at 15:16, on Zulip):

that's the add_import action, which adds an import for a fully qualified path

Florian Diebold (Feb 07 2020 at 15:17, on Zulip):

maybe we shouldn't propose that one if the path doesn't actually resolve

Kirill Bulatov (Feb 07 2020 at 15:21, on Zulip):

Damn, I just wanted to quickly ask a naming question without diving into the details :slight_smile:

ast::UserItem::cast fully resolves, the code compiles, here's the code pointer: https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_assists/src/assists/auto_import.rs#L35

Import ast::UserItem::cast is brought by the add_import.rsand is fully valid from the functionality point of view: move FQN to use statement, leave the last part of it in the code.
Import ast is brought by auto_import.rs and is not valid at all, it should not appear. I think that happens due to source_analyzer.resolve_path returning None, but that's another thing, I'll bother you about it later.

What I want now is to get rid of two equally named Import ... pop-ups hence asking your opinions on the naming.

Kirill Bulatov (Feb 07 2020 at 15:23, on Zulip):

My opinion: change the add_import.rs one to show the Add import ... pop-up.

Florian Diebold (Feb 07 2020 at 15:25, on Zulip):

Well, in this situation one of the actions shouldn't actually be there, so I'm not convinced that the name collision is actually a problem :)

Jeremy Kolb (Feb 07 2020 at 15:25, on Zulip):

Ah is one of these add_import and the other auto_import?

Jeremy Kolb (Feb 07 2020 at 15:26, on Zulip):

If so they should definitely have different labels

Kirill Bulatov (Feb 07 2020 at 15:27, on Zulip):

Well, in this situation one of the actions shouldn't actually be there, so I'm not convinced that the name collision is actually a problem :)

That actually makes sense. Still, one more argument for the renaming: if you open the action list twice (first for the unresolved path, then for the resolved path after the auto import), we'll get the same label twice which may be a bit confusing.

Jeremy Kolb (Feb 07 2020 at 15:28, on Zulip):

I think you're both right :)

Florian Diebold (Feb 07 2020 at 15:28, on Zulip):

well, you'd have imported ast, and then you'd get the option to import ast::UseItem::cast. I think they're both doing kind of the same thing, adding an import. Writing Add import foo for one and Import foo for the other doesn't really make things clearer for the user

Kirill Bulatov (Feb 07 2020 at 15:31, on Zulip):

Yeah, and I'm almost happy after hearing the latest Florian's idea, funny that I did not think about it from that perspective.

Still confuses me a lot when debugging those false-positives since sometimes I have no idea which label does what :)
But I'll concentrate on the actuall error now, thank you.

Laurențiu Nicola (Feb 07 2020 at 15:37, on Zulip):

So what's the difference between add import and auto import?

Florian Diebold (Feb 07 2020 at 15:38, on Zulip):

add import works on any qualified path (like a::b::c), adds an import for that path and replaces the original path by the last segment (so use a::b::c; (...) c).
auto import works on a name that doesn't resolve and looks for paths that you could import for it. so you'd have c and it'd find the a::b::c automatically

Laurențiu Nicola (Feb 07 2020 at 15:39, on Zulip):

I see. I wouldn't mind if they were the same action.

Florian Diebold (Feb 07 2020 at 15:40, on Zulip):

well, effectively they are (as it looks to the user)

matklad (Feb 07 2020 at 16:42, on Zulip):

While in IDE, do as IDEA does:

pasted image

"Replace qualified path with use"

Last update: Feb 25 2020 at 04:10UTC