fix(extract_module) resolving import panics and improve import resolution

[feniljain]

• Should solve server panicked on extract module action #11766
• While adding a test case for this issue, I observed another issue:
  For this test case:

            mod x {
                pub struct Foo;
                pub struct Bar;
            }

            use x::{Bar, Foo};

            $0type A = (Foo, Bar);$0

extract module should yield this:

            mod x {
                pub struct Foo;
                pub struct Bar;
            }

            use x::{};

            mod modname {
                use super::x::Bar;

                use super::x::Foo;

                pub(crate) type A = (Foo, Bar);
            }

instead it gave:

            mod x {
                pub struct Foo;
                pub struct Bar;
            }

            use x::{};

            mod modname {
                use x::Bar;

                use x::Foo;

                pub(crate) type A = (Foo, Bar);
            }

So fixed this problem with second commit

[Veykril]

This can probably be simplified like. Note that this code will not work correctly if import_path intersects with more than one range in the vec, thoug your version also has problems with that (yours just creates overlapping ranges in the vec again)

Suggested change

	let mut found_intersection = false;
	for i in 0..import_paths_to_be_removed.len() {
	if let Some(_) = import_paths_to_be_removed[i].intersect(import_path) {
	import_paths_to_be_removed[i] = import_paths_to_be_removed[i].cover(import_path);
	found_intersection = true;
	}
	}
	if !found_intersection {
	import_paths_to_be_removed.push(import_path);
	}
	let r = import_paths_to_be_removed.position(|it| it.intersect(import_path));
	match r {
	Some(it) => import_paths_to_be_removed[it] = import_paths_to_be_removed[it].cover(import_path),
	None => import_paths_to_be_removed.push(import_path),
	}

[feniljain]

This was my thought process while writing this:

It checks from the very start ( when there are no elements in the loop ). So even the first element is added in this loop. Let's say next element added has intersection, tho it merges with the current one and due to this recurring process no two elements in the loop have intersection at any given time, which helps when new element is added.

[feniljain]

So for example:

For these set of imports:

use first::{A, B, C};
use second::D;

array initially -> []
A is parsed
no intersections
[A]
B is parsed
found intersection with A
[{A-B}]
let's say now D is parsed for some reason
no intersection found
[{A-B}, D]
now C is parsed
Found intersection with {A-B}
[{A-B-C}, D]

[Veykril]

Assuming the following: [0..5, 5..10] and we now have the import_range 4..6 we will end up with [0..6, 4..10] which are overlapping. (This happens for both your and my snippet, since mine is just a simplification of your loop). But as I said, this might be a non-issue given how the overall algo works (that is whether the assumption is there that the won't be a range inserted that will ever collide with more than one existing range)

[feniljain]

Ah now I see what you meant by overlaps.

Though, let's try to recreate the example you gave in real code.

use first::{A, B, C}

( Counting numbers from curly braces: "A," -> 0..1 & "B," -> 3..4 & ", C" -> 4:6 )

Here it is sure B, will be counted and not , B as this over here:

rust-analyzer/crates/ide-assists/src/handlers/remove_unused_param.rs

Lines 144 to 149 in 6c9fc4f

	let up_to_comma = next_prev().find_map(|dir| {
	node.siblings_with_tokens(dir)
	.filter_map(|it| it.into_token())
	.find(|it| it.kind() == T![,])
	.map(|it| (dir, it))
	});

uses next_prev() which returns Next before Prev

rust-analyzer/crates/ide-assists/src/utils.rs

Line 268 in 6c9fc4f

[Direction::Next, Direction::Prev].into_iter()

Now let's say A and C get pushed in first so we have an array something like this:

[0..1, 4..6]

Now "B" arrives, it has a range of 3..4 which only collides with C, this seems to be extensible to any number of imports between the braces now making it safe.

So there seems to be no case where 1..5 and 5..10 exists at the same time in the array, they get detected by intersect() and merged.

Am I missing some case here again, something I am not thinking of?

If not I would move forward with integrating all the suggestion changes, also I will keep a link to this PR in comments so that anyone in future can understand it.

[Veykril]

@bors r+

[bors]

📌 Commit 8a1ef52 has been approved by Veykril

[bors]

⌛ Testing commit 8a1ef52 with merge 2f0814e...

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 2f0814e to master...