Skip to content

Commit fd92d9d

Browse files
committed
fix: Improve label on add_missing_match_arms assist
"Fill match arms" is an extremely helpful assist, but the name is pretty opaque to new users (at least it was to me). We actually renamed the file in #10299 from `fill_match_arms.rs` to `add_missing_match_arms.rs` but the label hasn't changed from its original text in #733. Instead, show "Add N missing match arms" if there are multiple missing match arms, and show "Add missing match arm `Foo::Bar`" when there's only a single missing match arm. Partially generated by Claude Opus.
1 parent 512db74 commit fd92d9d

File tree

2 files changed

+97
-26
lines changed

2 files changed

+97
-26
lines changed

crates/ide-assists/src/handlers/add_missing_match_arms.rs

Lines changed: 79 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::iter::{self, Peekable};
1+
use std::iter;
22

33
use either::Either;
44
use hir::{Adt, AsAssocItem, Crate, FindPathConfig, HasAttrs, ModuleDef, Semantics};
@@ -93,8 +93,8 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
9393
} else {
9494
None
9595
};
96-
let (mut missing_pats, is_non_exhaustive, has_hidden_variants): (
97-
Peekable<Box<dyn Iterator<Item = (ast::Pat, bool)>>>,
96+
let (missing_pats, is_non_exhaustive, has_hidden_variants): (
97+
Vec<(ast::Pat, bool)>,
9898
bool,
9999
bool,
100100
) = if let Some(enum_def) = resolve_enum_def(&ctx.sema, &expr, self_ty.as_ref()) {
@@ -117,15 +117,15 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
117117
.filter(|(variant_pat, _)| is_variant_missing(&top_lvl_pats, variant_pat));
118118

119119
let option_enum = FamousDefs(&ctx.sema, module.krate(ctx.db())).core_option_Option();
120-
let missing_pats: Box<dyn Iterator<Item = _>> = if matches!(enum_def, ExtendedEnum::Enum { enum_: e, .. } if Some(e) == option_enum)
120+
let missing_pats: Vec<_> = if matches!(enum_def, ExtendedEnum::Enum { enum_: e, .. } if Some(e) == option_enum)
121121
{
122122
// Match `Some` variant first.
123123
cov_mark::hit!(option_order);
124-
Box::new(missing_pats.rev())
124+
missing_pats.rev().collect()
125125
} else {
126-
Box::new(missing_pats)
126+
missing_pats.collect()
127127
};
128-
(missing_pats.peekable(), is_non_exhaustive, has_hidden_variants)
128+
(missing_pats, is_non_exhaustive, has_hidden_variants)
129129
} else if let Some(enum_defs) = resolve_tuple_of_enum_def(&ctx.sema, &expr, self_ty.as_ref()) {
130130
let is_non_exhaustive = enum_defs
131131
.iter()
@@ -169,12 +169,9 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
169169

170170
(ast::Pat::from(make.tuple_pat(patterns)), is_hidden)
171171
})
172-
.filter(|(variant_pat, _)| is_variant_missing(&top_lvl_pats, variant_pat));
173-
(
174-
(Box::new(missing_pats) as Box<dyn Iterator<Item = _>>).peekable(),
175-
is_non_exhaustive,
176-
has_hidden_variants,
177-
)
172+
.filter(|(variant_pat, _)| is_variant_missing(&top_lvl_pats, variant_pat))
173+
.collect();
174+
(missing_pats, is_non_exhaustive, has_hidden_variants)
178175
} else if let Some((enum_def, len)) =
179176
resolve_array_of_enum_def(&ctx.sema, &expr, self_ty.as_ref())
180177
{
@@ -205,33 +202,41 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
205202

206203
(ast::Pat::from(make.slice_pat(patterns)), is_hidden)
207204
})
208-
.filter(|(variant_pat, _)| is_variant_missing(&top_lvl_pats, variant_pat));
209-
(
210-
(Box::new(missing_pats) as Box<dyn Iterator<Item = _>>).peekable(),
211-
is_non_exhaustive,
212-
has_hidden_variants,
213-
)
205+
.filter(|(variant_pat, _)| is_variant_missing(&top_lvl_pats, variant_pat))
206+
.collect();
207+
(missing_pats, is_non_exhaustive, has_hidden_variants)
214208
} else {
215209
return None;
216210
};
217211

218212
let mut needs_catch_all_arm = is_non_exhaustive && !has_catch_all_arm;
219213

220214
if !needs_catch_all_arm
221-
&& ((has_hidden_variants && has_catch_all_arm) || missing_pats.peek().is_none())
215+
&& ((has_hidden_variants && has_catch_all_arm) || missing_pats.is_empty())
222216
{
223217
return None;
224218
}
225219

220+
let visible_count = missing_pats.iter().filter(|(_, hidden)| !hidden).count();
221+
let label = if visible_count == 0 {
222+
"Add missing catch-all match arm `_`".to_owned()
223+
} else if visible_count == 1 {
224+
let pat = &missing_pats.iter().find(|(_, hidden)| !hidden).unwrap().0;
225+
format!("Add missing match arm `{pat}`")
226+
} else {
227+
format!("Add {visible_count} missing match arms")
228+
};
229+
226230
acc.add(
227231
AssistId::quick_fix("add_missing_match_arms"),
228-
"Fill match arms",
232+
label,
229233
ctx.sema.original_range(match_expr.syntax()).range,
230234
|builder| {
231235
// having any hidden variants means that we need a catch-all arm
232236
needs_catch_all_arm |= has_hidden_variants;
233237

234238
let mut missing_arms = missing_pats
239+
.into_iter()
235240
.filter(|(_, hidden)| {
236241
// filter out hidden patterns because they're handled by the catch-all arm
237242
!hidden
@@ -635,7 +640,7 @@ mod tests {
635640
use crate::AssistConfig;
636641
use crate::tests::{
637642
TEST_CONFIG, check_assist, check_assist_not_applicable, check_assist_target,
638-
check_assist_unresolved, check_assist_with_config,
643+
check_assist_unresolved, check_assist_with_config, check_assist_with_label,
639644
};
640645

641646
use super::add_missing_match_arms;
@@ -1828,8 +1833,10 @@ fn foo(t: Test) {
18281833

18291834
#[test]
18301835
fn lazy_computation() {
1831-
// Computing a single missing arm is enough to determine applicability of the assist.
1832-
cov_mark::check_count!(add_missing_match_arms_lazy_computation, 1);
1836+
// We now collect all missing arms eagerly, so we can show the count
1837+
// of missing arms.
1838+
cov_mark::check_count!(add_missing_match_arms_lazy_computation, 4);
1839+
18331840
check_assist_unresolved(
18341841
add_missing_match_arms,
18351842
r#"
@@ -1841,6 +1848,54 @@ fn foo(tuple: (A, A)) {
18411848
);
18421849
}
18431850

1851+
#[test]
1852+
fn label_single_missing_arm() {
1853+
check_assist_with_label(
1854+
add_missing_match_arms,
1855+
r#"
1856+
enum A { One, Two }
1857+
fn foo(a: A) {
1858+
match $0a {
1859+
A::One => {}
1860+
}
1861+
}
1862+
"#,
1863+
"Add missing match arm `A::Two`",
1864+
);
1865+
}
1866+
1867+
#[test]
1868+
fn label_multiple_missing_arms() {
1869+
check_assist_with_label(
1870+
add_missing_match_arms,
1871+
r#"
1872+
enum A { One, Two, Three }
1873+
fn foo(a: A) {
1874+
match $0a {}
1875+
}
1876+
"#,
1877+
"Add 3 missing match arms",
1878+
);
1879+
}
1880+
1881+
#[test]
1882+
fn label_catch_all_only() {
1883+
check_assist_with_label(
1884+
add_missing_match_arms,
1885+
r#"
1886+
//- /main.rs crate:main deps:e
1887+
fn foo(t: ::e::E) {
1888+
match $0t {
1889+
e::E::A => {}
1890+
}
1891+
}
1892+
//- /e.rs crate:e
1893+
pub enum E { A, #[doc(hidden)] B, }
1894+
"#,
1895+
"Add missing catch-all match arm `_`",
1896+
);
1897+
}
1898+
18441899
#[test]
18451900
fn adds_comma_before_new_arms() {
18461901
check_assist(

crates/ide-assists/src/tests.rs

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,15 @@ pub(crate) fn check_assist_target(
207207
check(assist, ra_fixture, ExpectedResult::Target(target), None);
208208
}
209209

210+
#[track_caller]
211+
pub(crate) fn check_assist_with_label(
212+
assist: Handler,
213+
#[rust_analyzer::rust_fixture] ra_fixture: &str,
214+
label: &str,
215+
) {
216+
check(assist, ra_fixture, ExpectedResult::Label(label), None);
217+
}
218+
210219
#[track_caller]
211220
pub(crate) fn check_assist_not_applicable(
212221
assist: Handler,
@@ -307,6 +316,7 @@ enum ExpectedResult<'a> {
307316
Unresolved,
308317
After(&'a str),
309318
Target(&'a str),
319+
Label(&'a str),
310320
}
311321

312322
#[track_caller]
@@ -335,7 +345,7 @@ fn check_with_config(
335345

336346
let ctx = AssistContext::new(sema, &config, frange);
337347
let resolve = match expected {
338-
ExpectedResult::Unresolved => AssistResolveStrategy::None,
348+
ExpectedResult::Unresolved | ExpectedResult::Label(_) => AssistResolveStrategy::None,
339349
_ => AssistResolveStrategy::All,
340350
};
341351
let mut acc = Assists::new(&ctx, resolve);
@@ -404,14 +414,20 @@ fn check_with_config(
404414
let range = assist.target;
405415
assert_eq_text!(&text_without_caret[range], target);
406416
}
417+
(Some(assist), ExpectedResult::Label(label)) => {
418+
assert_eq!(assist.label.to_string(), label);
419+
}
407420
(Some(assist), ExpectedResult::Unresolved) => assert!(
408421
assist.source_change.is_none(),
409422
"unresolved assist should not contain source changes"
410423
),
411424
(Some(_), ExpectedResult::NotApplicable) => panic!("assist should not be applicable!"),
412425
(
413426
None,
414-
ExpectedResult::After(_) | ExpectedResult::Target(_) | ExpectedResult::Unresolved,
427+
ExpectedResult::After(_)
428+
| ExpectedResult::Target(_)
429+
| ExpectedResult::Label(_)
430+
| ExpectedResult::Unresolved,
415431
) => {
416432
panic!("code action is not applicable")
417433
}

0 commit comments

Comments
 (0)