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

IDE: add imports after pasting code #7597

Merged
merged 1 commit into from Oct 1, 2022

Conversation

Kobzol
Copy link
Member

@Kobzol Kobzol commented Jul 27, 2021

This PR adds a copy/paste postprocessor that tries to import unresolved items from the pasted code. There is also some crude support for selecting between ambiguous imports.

I took inspiration from the Kotlin plugin, mainly with the data flavor thing, I'm not sure how that's supposed to work.

auto-import

Fixes: #7241

changelog: Automatically import unresolved items after copy/pasting Rust code.

@Kobzol Kobzol added the feature label Jul 27, 2021
@Kobzol Kobzol requested a review from vlad20012 July 27, 2021 21:28
@Kobzol Kobzol added this to In Progress in To test via automation Jul 27, 2021
@mchernyavsky mchernyavsky self-assigned this Nov 2, 2021
Copy link
Member

@mchernyavsky mchernyavsky left a comment

Choose a reason for hiding this comment

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

The copy-paste processor adds wrong import in this case:

  • Before
mod a {
    pub struct S;
    pub fn foo1() -> S { S }
}

mod b {
    struct S;
    fn foo2() -> S { S } // copied
}

/*caret*/
  • After
use crate::a::S;

mod a {
    pub struct S;
    pub fn foo1() -> S { S }
}

mod b {
    struct S;
    fn foo2() -> S { S }
}

fn foo2() -> S { S }

@Kobzol
Copy link
Member Author

Kobzol commented Nov 8, 2021

Right. In this case, b::S cannot be imported, because it's private. In the current implementation, if there is only a single import candidate, it is always imported on a best-effort basis, the import map is only consulted if there are multiple candidates.

Should I change it so that if the original item cannot be imported, the auto import won't do anything? Or should we display some kind of conflict dialog like in the move refactoring?

@mchernyavsky
Copy link
Member

@Kobzol Kotlin plugin adds imports for items even if they are private. We can add use's with fully-qualified names to help users to understand what items are missed.

@Undin What do you think?

@dima74
Copy link
Member

dima74 commented Dec 13, 2021

In the current implementation, if there is only a single import candidate, it is always imported on a best-effort basis, the import map is only consulted if there are multiple candidates.

I think import map should be used even if there is a single candidate, to ensure that we add import for the correct item

@Kobzol
Copy link
Member Author

Kobzol commented Jan 31, 2022

@dima74 How should this work after #8266 is implemented? Should it use a separate implementation (this PR) or somehow build on top of #8266 and add support for ambiguous imports?

@dima74
Copy link
Member

dima74 commented Jan 31, 2022

@Kobzol I thinks these are two independent features, and you should keep current implementation (taking into account #7597 (comment))

@Kobzol
Copy link
Member Author

Kobzol commented Aug 3, 2022

I fixed some things and added support for pat bindings.

@dima74 dima74 self-assigned this Aug 3, 2022
override fun visitPatBinding(binding: RsPatBinding) {
if (data.importMap.elementToFqn(binding, range) != null) {
val referenceName = binding.referenceName
val isNameInScope = binding.hasInScope(referenceName, VALUES)
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about this check. If we do the check, then import will not be added, and we may end up with binding recognized as variable name (user will see the warning about other match arms unreachable). If we don't do the check, import will be added and we end up with duplicated names in scope (user will see the corresponding error). I don't know which error is easier to fix (user need to change code after paste anyway). What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that we should not do the check after all. Duplicated names are a more explicit error that makes it easy to see where the problem is, the other error is a bit more subtle.

@Kobzol
Copy link
Member Author

Kobzol commented Aug 4, 2022

Currently, if the thing that we would normally import is private, it will not be imported. What about this: if we encounter an element at some relative position which should be resolved to a fully qualified item, but the item is not offered by the auto import fix (since it is probably private), we would instead fully qualify that path. Then instead of an unresolved path (or a path resolved to some unrelated local item), the user would be left with a fully qualified path and a red underline with a quick action to make that item public.

A similar approach with fully qualifying the name could also be used if the name to be imported is already present in the target scope. But this can be left for another PR.

@dima74
Copy link
Member

dima74 commented Aug 4, 2022

Currently, if the thing that we would normally import is private, it will not be imported. What about this: if we encounter an element at some relative position which should be resolved to a fully qualified item, but the item is not offered by the auto import fix (since it is probably private), we would instead fully qualify that path. Then instead of an unresolved path (or a path resolved to some unrelated local item), the user would be left with a fully qualified path and a red underline with a quick action to make that item public.

I like this idea, but lets do it in another PR for easier review process

@Kobzol
Copy link
Member Author

Kobzol commented Sep 30, 2022

@dima74 I rebased on master, squashed the commits and integrated code from #9153.

Copy link
Member

@dima74 dima74 left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 1, 2022

Build succeeded:

@bors bors bot merged commit 564b790 into intellij-rust:master Oct 1, 2022
To test automation moved this from In Progress to Test Oct 1, 2022
@github-actions github-actions bot added this to the v180 milestone Oct 2, 2022
@Kobzol Kobzol deleted the ide-auto-import-on-paste branch October 2, 2022 06:46
@mili-l mili-l self-assigned this Oct 5, 2022
@mili-l mili-l moved this from Test to Done in To test Oct 7, 2022
bors bot added a commit that referenced this pull request Oct 17, 2022
9556: IDE: Temporary disable adding imports after pasting code r=dima74 a=dima74

Looks like ["Insert imports on paste"](#7597) may be slow (https://youtrack.jetbrains.com/issue/VIM-2769, #9441 (comment)), so lets disable it by default for now


Co-authored-by: Dmitry Murzin <diralik@yandex.ru>
bors bot added a commit that referenced this pull request Oct 17, 2022
9556: IDE: Temporary disable adding imports after pasting code r=dima74 a=dima74

Looks like ["Insert imports on paste"](#7597) may be slow (https://youtrack.jetbrains.com/issue/VIM-2769, #9441 (comment)), so lets disable it by default for now


Co-authored-by: Dmitry Murzin <diralik@yandex.ru>
bors bot added a commit that referenced this pull request Oct 17, 2022
9556: IDE: Temporary disable adding imports after pasting code r=dima74 a=dima74

Looks like ["Insert imports on paste"](#7597) may be slow (https://youtrack.jetbrains.com/issue/VIM-2769, #9441 (comment)), so lets disable it by default for now


Co-authored-by: Dmitry Murzin <diralik@yandex.ru>
bors bot added a commit that referenced this pull request Oct 17, 2022
9556: IDE: Temporary disable adding imports after pasting code r=dima74 a=dima74

Looks like ["Insert imports on paste"](#7597) may be slow (https://youtrack.jetbrains.com/issue/VIM-2769, #9441 (comment)), so lets disable it by default for now


Co-authored-by: Dmitry Murzin <diralik@yandex.ru>
bors bot added a commit that referenced this pull request Jan 7, 2023
9441: IDE: qualify unimportable elements in import after paste r=dima74 a=Kobzol

This is an improvement of #7597. Changes:
- Fixes proper import of non-trivial paths (like `a::b`).
- Unimportable paths are now fully qualified (if the items are in the same crate).

changelog: Fully qualify paths from the local crate that cannot be imported after paste.

Co-authored-by: Jakub Beránek <berykubik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
To test
  
Done
Development

Successfully merging this pull request may close these issues.

"Import all" context action / quick fix
4 participants