INSP: make items from std appear first when auto import #6501

gfreezy · 2020-12-11T08:57:54Z

changelog: make items from std appear first when auto import

when have async-std as dependency, auto import Arc should use std::sync::Arc instead of async_std::sync::Arc

gfreezy · 2020-12-14T04:52:54Z

@Undin i have no idea how to fix the regressions.

Undin

It would be great to provide tests.
See org.rust.ide.inspections.import.AutoImportFixTestBase as base class for all auto import tests. I suppose org.rust.ide.inspections.import.AutoImportFixTestBase#checkAutoImportFixByTextWithMultipleChoice is most suitable to check order (but it requires to change Set to List as type of expectedElements collection)

Undin · 2021-01-11T15:04:18Z

src/main/kotlin/org/rust/ide/utils/import/ImportCandidatesCollector.kt

@@ -61,6 +61,14 @@ object ImportCandidatesCollector {
            // check that result after import can be resolved and resolved element is suitable
            // if no, don't add it in candidate list
            .filter { canBeResolvedToSuitableItem(importingPathText, importContext, it.info) }
+            // make std appear first
+            .sortedBy {
+                if (it.qualifiedNamedItem.containingCrate?.origin == PackageOrigin.STDLIB) {


What's about code from your workspace?
If we want to make any sorting here, we want to put items from workspace first

Undin · 2021-01-11T15:19:37Z

src/main/kotlin/org/rust/ide/utils/import/ImportCandidatesCollector.kt

+                    "0${it.qualifiedNamedItem}"
+                } else {
+                    "1${it.qualifiedNamedItem}"


The current approach creates a new string object on each comparison. Let's try to avoid it.
You can create a composite comparator to compare origin first and only after that compare qualified paths.
compareBy and thenBy should help you to create the corresponding comparator and sortedWith method to use it.
Also, I'd recommend not to use it.qualifiedNamedItem.toString() to get qualified name (currently it's used implicitly to construct "1${it.qualifiedNamedItem}" string) and use ImportInfo.usePath for it

As an alternative, I can suggest implementing Comparable for ImportCandidate class

Undin · 2021-01-11T15:22:03Z

src/main/kotlin/org/rust/ide/utils/import/ImportCandidatesCollector.kt

+            .sortedBy {
+                if (it.qualifiedNamedItem.containingCrate?.origin == PackageOrigin.STDLIB) {
+                    "0${it.qualifiedNamedItem}"
+                } else {
+                    "1${it.qualifiedNamedItem}"
+                }
+            }


Exactly the same code as above. Let's create single comparator for ImportCandidate or implement Comparable interface for ImportCandidate

gfreezy · 2021-01-13T16:39:46Z

src/test/kotlin/org/rust/ide/inspections/import/AutoImportFixStdTest.kt

+        }
+
+        fn foo(t: <error descr="Unresolved reference: `AtomicUsize`">AtomicUsize/*caret*/</error>) {}
+    """, listOf("crate::bar::AtomicUsize", "dep_lib_target::foo::AtomicUsize", "std::sync::atomic::AtomicUsize"),


ERROR: expected:<[crate::bar::AtomicUsize, dep_lib_target::foo::AtomicUsize, std::sync::atomic::AtomicUsize]> but was:<[crate::bar::AtomicUsize, dep_lib_target::foo::AtomicUsize]>

why std is not available?

Because all Atomic* structs are generated by macros that are not expanded in these tests. To make it work, we use ExpandMacros annotation. But I suggest not to use here because macro expansion slows down test execution without actual necessity.
Let's just use another stdlib struct, for example, std::sync::Arc

gfreezy · 2021-01-13T16:40:10Z

src/test/kotlin/org/rust/ide/inspections/import/AutoImportFixStdTest.kt

+        Testmarks.autoInjectedStdCrate)
+
+    @MockEdition(CargoWorkspace.Edition.EDITION_2018)
+    fun `test import item from std over extern crate`() = checkAutoImportFixByFileTreeWithMultipleChoice("""


failed with the same problem.

Undin

@gfreezy Could you adjust failed tests, please?. As I can see, they fail because we started to check the order of suggested items and in these tests order is wrong

gfreezy · 2021-01-18T02:42:11Z

@Undin need help to fix the regression

Undin

@Undin need help to fix the regression

Unfortunately, regression-2 is broken currently for some reason. So, just ignore it.

Could you squash commits into a single one after all necessary fixes, please?

Undin · 2021-01-18T08:58:28Z

src/main/kotlin/org/rust/ide/utils/import/ImportCandidatesCollector.kt

+    override fun compareTo(other: ImportCandidate): Int {
+        val origin = qualifiedNamedItem.containingCrate?.origin
+        val otherOrigin = other.qualifiedNamedItem.containingCrate?.origin
+        val userPath = info.usePath


Suggested change

val userPath = info.usePath

val usePath = info.usePath

Undin · 2021-01-18T09:02:49Z

src/main/kotlin/org/rust/ide/utils/import/ImportCandidatesCollector.kt

+        val origin = qualifiedNamedItem.containingCrate?.origin
+        val otherOrigin = other.qualifiedNamedItem.containingCrate?.origin
+        val userPath = info.usePath
+        val otherUsePath = info.usePath


Suggested change

val otherUsePath = info.usePath

val otherUsePath = other.info.usePath

It's a reason of weird candidates order in tests

Undin

Thanks!
bors r+

bors · 2021-01-18T20:06:51Z

Build succeeded:

check

gfreezy changed the title ~~make items from std appear first when auto import~~ INSP: make items from std appear first when auto import Dec 11, 2020

Undin self-assigned this Dec 11, 2020

Undin requested changes Jan 11, 2021

View reviewed changes

Undin added the fix Pull requests that fix some bug(s) label Jan 11, 2021

gfreezy force-pushed the auto-import-order branch from 9701166 to 8c65d56 Compare January 13, 2021 16:38

gfreezy commented Jan 13, 2021

View reviewed changes

gfreezy force-pushed the auto-import-order branch from 8c65d56 to f9ac9cb Compare January 14, 2021 10:22

Undin reviewed Jan 15, 2021

View reviewed changes

Undin requested changes Jan 18, 2021

View reviewed changes

gfreezy force-pushed the auto-import-order branch 2 times, most recently from 36664bc to 84dba6a Compare January 18, 2021 15:22

INSP: make items from std appear first when auto import

5413a79

gfreezy force-pushed the auto-import-order branch from 84dba6a to 5413a79 Compare January 18, 2021 15:28

Undin added this to the v140 milestone Jan 18, 2021

Undin approved these changes Jan 18, 2021

View reviewed changes

bors bot merged commit 9b7f095 into intellij-rust:master Jan 18, 2021

gfreezy deleted the auto-import-order branch January 19, 2021 02:40

	val otherUsePath = info.usePath
	val otherUsePath = other.info.usePath

INSP: make items from std appear first when auto import #6501

INSP: make items from std appear first when auto import #6501

Uh oh!

Conversation

gfreezy commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gfreezy commented Dec 14, 2020

Uh oh!

Undin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Undin Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Undin left a comment

Choose a reason for hiding this comment

Uh oh!

gfreezy commented Jan 18, 2021

Uh oh!

Undin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Undin left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Jan 18, 2021

Uh oh!

Uh oh!

gfreezy commented Dec 11, 2020 •

edited

Loading

Undin Jan 13, 2021 •

edited

Loading