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

Include enum path in variant suggestion #101357

Merged
merged 1 commit into from
Sep 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 27 additions & 11 deletions compiler/rustc_middle/src/thir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use rustc_middle::ty::subst::SubstsRef;
use rustc_middle::ty::{self, AdtDef, Ty, UpvarSubsts};
use rustc_middle::ty::{CanonicalUserType, CanonicalUserTypeAnnotation};
use rustc_span::def_id::LocalDefId;
use rustc_span::{Span, Symbol, DUMMY_SP};
use rustc_span::{sym, Span, Symbol, DUMMY_SP};
use rustc_target::abi::VariantIdx;
use rustc_target::asm::InlineAsmRegOrRegClass;
use std::fmt;
Expand Down Expand Up @@ -695,17 +695,32 @@ impl<'tcx> fmt::Display for Pat<'tcx> {
Ok(())
}
PatKind::Variant { ref subpatterns, .. } | PatKind::Leaf { ref subpatterns } => {
let variant = match self.kind {
PatKind::Variant { adt_def, variant_index, .. } => {
Some(adt_def.variant(variant_index))
}
_ => self.ty.ty_adt_def().and_then(|adt| {
if !adt.is_enum() { Some(adt.non_enum_variant()) } else { None }
let variant_and_name = match self.kind {
PatKind::Variant { adt_def, variant_index, .. } => ty::tls::with(|tcx| {
let variant = adt_def.variant(variant_index);
let adt_did = adt_def.did();
let name = if tcx.get_diagnostic_item(sym::Option) == Some(adt_did)
|| tcx.get_diagnostic_item(sym::Result) == Some(adt_did)
Comment on lines +702 to +703
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it might give incorrect suggestions when using #![no_implicit_prelude].

Copy link
Member Author

@compiler-errors compiler-errors Sep 3, 2022

Choose a reason for hiding this comment

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

I guess, but that could probably be said about plenty of other suggestions where we're currently rendering paths, making suggestions, etc. They're, at most, best effort.

I don't think this needs to be fixed, especially because it's basically impossible to determine what the proper shortest path to refer to an item is in any module, due to rustc_resolve not exposing import information in a way we can consume here, and it would be unnecessarily verbose to print the full untrimmed path (e.g. std::option::Option::Some).

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @estebank I remember some tests getting updated that now figure out the best import path to use, but I may be totally off here

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the change you remember @oli-obk was about the ordering of suggestions (it filters core:: paths if the std:: is available too), not for this. I think that we eventually would want to rebuild suggestions so that we can ask for paths in a specific DefId or HirId so that it is always accurate for that scope, but we're far from there.

I'm ok with landing this as is.

{
variant.name.to_string()
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use the printing machinery (that only uses the name if it is unique, otherwise the full path), then this suggestion would be mostly right in the face of local shadowing (but see previous comment about needing a function that takes a scope identifier for accurate paths, always).

} else {
format!("{}::{}", tcx.def_path_str(adt_def.did()), variant.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't variants have a def id that can be passed into def_path_str?

};
Some((variant, name))
}),
_ => self.ty.ty_adt_def().and_then(|adt_def| {
if !adt_def.is_enum() {
ty::tls::with(|tcx| {
Some((adt_def.non_enum_variant(), tcx.def_path_str(adt_def.did())))
})
} else {
None
}
}),
};

if let Some(variant) = variant {
write!(f, "{}", variant.name)?;
if let Some((variant, name)) = &variant_and_name {
write!(f, "{}", name)?;

// Only for Adt we can have `S {...}`,
// which we handle separately here.
Expand All @@ -730,8 +745,9 @@ impl<'tcx> fmt::Display for Pat<'tcx> {
}
}

let num_fields = variant.map_or(subpatterns.len(), |v| v.fields.len());
if num_fields != 0 || variant.is_none() {
let num_fields =
variant_and_name.as_ref().map_or(subpatterns.len(), |(v, _)| v.fields.len());
if num_fields != 0 || variant_and_name.is_none() {
write!(f, "(")?;
for i in 0..num_fields {
write!(f, "{}", start_or_comma())?;
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/thir/pattern/usefulness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -754,8 +754,8 @@ fn lint_non_exhaustive_omitted_patterns<'p, 'tcx>(
hir_id: HirId,
witnesses: Vec<DeconstructedPat<'p, 'tcx>>,
) {
let joined_patterns = joined_uncovered_patterns(cx, &witnesses);
cx.tcx.struct_span_lint_hir(NON_EXHAUSTIVE_OMITTED_PATTERNS, hir_id, sp, |build| {
let joined_patterns = joined_uncovered_patterns(cx, &witnesses);
let mut lint = build.build("some variants are not matched explicitly");
lint.span_label(sp, pattern_not_covered_label(&witnesses, &joined_patterns));
lint.help(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ fn main() {
let _a = || { match l1 { L1::A => (), L1::B => () } };
// (except if the match is already non-exhaustive)
let _b = || { match l1 { L1::A => () } };
//~^ ERROR: non-exhaustive patterns: `B` not covered [E0004]
//~^ ERROR: non-exhaustive patterns: `L1::B` not covered [E0004]

// l2 should not be captured as it is a non-exhaustive SingleVariant
// defined in this crate
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error[E0004]: non-exhaustive patterns: `B` not covered
error[E0004]: non-exhaustive patterns: `L1::B` not covered
--> $DIR/non-exhaustive-match.rs:26:25
|
LL | let _b = || { match l1 { L1::A => () } };
| ^^ pattern `B` not covered
| ^^ pattern `L1::B` not covered
|
note: `L1` defined here
--> $DIR/non-exhaustive-match.rs:12:14
Expand All @@ -12,8 +12,8 @@ LL | enum L1 { A, B }
= note: the matched value is of type `L1`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
|
LL | let _b = || { match l1 { L1::A => (), B => todo!() } };
| ++++++++++++++
LL | let _b = || { match l1 { L1::A => (), L1::B => todo!() } };
| ++++++++++++++++++

error[E0004]: non-exhaustive patterns: type `E1` is non-empty
--> $DIR/non-exhaustive-match.rs:37:25
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/empty/empty-never-array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ enum Helper<T, U> {

fn transmute<T, U>(t: T) -> U {
let Helper::U(u) = Helper::T(t, []);
//~^ ERROR refutable pattern in local binding: `T(_, _)` not covered
//~^ ERROR refutable pattern in local binding: `Helper::T(_, _)` not covered
u
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/empty/empty-never-array.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error[E0005]: refutable pattern in local binding: `T(_, _)` not covered
error[E0005]: refutable pattern in local binding: `Helper::T(_, _)` not covered
--> $DIR/empty-never-array.rs:10:9
|
LL | let Helper::U(u) = Helper::T(t, []);
| ^^^^^^^^^^^^ pattern `T(_, _)` not covered
| ^^^^^^^^^^^^ pattern `Helper::T(_, _)` not covered
|
= note: `let` bindings require an "irrefutable pattern", like a `struct` or an `enum` with only one variant
= note: for more information, visit https://doc.rust-lang.org/book/ch18-02-refutability.html
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/error-codes/E0004.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error[E0004]: non-exhaustive patterns: `HastaLaVistaBaby` not covered
error[E0004]: non-exhaustive patterns: `Terminator::HastaLaVistaBaby` not covered
--> $DIR/E0004.rs:9:11
|
LL | match x {
| ^ pattern `HastaLaVistaBaby` not covered
| ^ pattern `Terminator::HastaLaVistaBaby` not covered
|
note: `Terminator` defined here
--> $DIR/E0004.rs:2:5
Expand All @@ -15,7 +15,7 @@ LL | HastaLaVistaBaby,
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
|
LL ~ Terminator::TalkToMyHand => {}
LL + HastaLaVistaBaby => todo!()
LL + Terminator::HastaLaVistaBaby => todo!()
|

error: aborting due to previous error
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ fn main() {
Foo::A => {}
Foo::B => {}
}
//~^^^^ ERROR non-exhaustive patterns: `C` not covered
//~^^^^ ERROR non-exhaustive patterns: `Foo::C` not covered

match Foo::A {
Foo::A => {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,11 @@ LL | #[warn(non_exhaustive_omitted_patterns)]
= note: see issue #89554 <https://github.com/rust-lang/rust/issues/89554> for more information
= help: add `#![feature(non_exhaustive_omitted_patterns_lint)]` to the crate attributes to enable

error[E0004]: non-exhaustive patterns: `C` not covered
error[E0004]: non-exhaustive patterns: `Foo::C` not covered
--> $DIR/feature-gate-non_exhaustive_omitted_patterns_lint.rs:20:11
|
LL | match Foo::A {
| ^^^^^^ pattern `C` not covered
| ^^^^^^ pattern `Foo::C` not covered
|
note: `Foo` defined here
--> $DIR/feature-gate-non_exhaustive_omitted_patterns_lint.rs:12:15
Expand All @@ -116,7 +116,7 @@ LL | A, B, C,
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
|
LL ~ Foo::B => {}
LL + C => todo!()
LL + Foo::C => todo!()
|

error: aborting due to previous error; 10 warnings emitted
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/issue-94866.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error[E0004]: non-exhaustive patterns: `B` not covered
error[E0004]: non-exhaustive patterns: `Enum::B` not covered
--> $DIR/issue-94866.rs:10:11
|
LL | match Enum::A {
| ^^^^^^^ pattern `B` not covered
| ^^^^^^^ pattern `Enum::B` not covered
|
note: `Enum` defined here
--> $DIR/issue-94866.rs:7:16
Expand All @@ -13,7 +13,7 @@ LL | enum Enum { A, B }
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
|
LL ~ Enum::A => m!(),
LL + B => todo!()
LL + Enum::B => todo!()
|

error: aborting due to previous error
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/match/match_non_exhaustive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ fn main() {
match l { L::A => (), L::B => () };
// (except if the match is already non-exhaustive)
match l { L::A => () };
//~^ ERROR: non-exhaustive patterns: `B` not covered [E0004]
//~^ ERROR: non-exhaustive patterns: `L::B` not covered [E0004]

// E1 is not visibly uninhabited from here
let (e1, e2) = bar();
Expand Down
8 changes: 4 additions & 4 deletions src/test/ui/match/match_non_exhaustive.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error[E0004]: non-exhaustive patterns: `B` not covered
error[E0004]: non-exhaustive patterns: `L::B` not covered
--> $DIR/match_non_exhaustive.rs:23:11
|
LL | match l { L::A => () };
| ^ pattern `B` not covered
| ^ pattern `L::B` not covered
|
note: `L` defined here
--> $DIR/match_non_exhaustive.rs:10:13
Expand All @@ -12,8 +12,8 @@ LL | enum L { A, B }
= note: the matched value is of type `L`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
|
LL | match l { L::A => (), B => todo!() };
| ++++++++++++++
LL | match l { L::A => (), L::B => todo!() };
| +++++++++++++++++

error[E0004]: non-exhaustive patterns: type `E1` is non-empty
--> $DIR/match_non_exhaustive.rs:28:11
Expand Down
8 changes: 4 additions & 4 deletions src/test/ui/pattern/usefulness/doc-hidden-non-exhaustive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,22 @@ fn main() {
HiddenEnum::A => {}
HiddenEnum::C => {}
}
//~^^^^ non-exhaustive patterns: `B` not covered
//~^^^^ non-exhaustive patterns: `HiddenEnum::B` not covered

match HiddenEnum::A {
HiddenEnum::A => {}
}
//~^^^ non-exhaustive patterns: `B` and `_` not covered
//~^^^ non-exhaustive patterns: `HiddenEnum::B` and `_` not covered

match None {
None => {}
Some(HiddenEnum::A) => {}
}
//~^^^^ non-exhaustive patterns: `Some(B)` and `Some(_)` not covered
//~^^^^ non-exhaustive patterns: `Some(HiddenEnum::B)` and `Some(_)` not covered

match InCrate::A {
InCrate::A => {}
InCrate::B => {}
}
//~^^^^ non-exhaustive patterns: `C` not covered
//~^^^^ non-exhaustive patterns: `InCrate::C` not covered
}
24 changes: 12 additions & 12 deletions src/test/ui/pattern/usefulness/doc-hidden-non-exhaustive.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ LL ~ HiddenEnum::B => {}
LL + _ => todo!()
|

error[E0004]: non-exhaustive patterns: `B` not covered
error[E0004]: non-exhaustive patterns: `HiddenEnum::B` not covered
--> $DIR/doc-hidden-non-exhaustive.rs:21:11
|
LL | match HiddenEnum::A {
| ^^^^^^^^^^^^^ pattern `B` not covered
| ^^^^^^^^^^^^^ pattern `HiddenEnum::B` not covered
|
note: `HiddenEnum` defined here
--> $DIR/auxiliary/hidden.rs:3:5
Expand All @@ -34,14 +34,14 @@ LL | B,
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
|
LL ~ HiddenEnum::C => {}
LL + B => todo!()
LL + HiddenEnum::B => todo!()
|

error[E0004]: non-exhaustive patterns: `B` and `_` not covered
error[E0004]: non-exhaustive patterns: `HiddenEnum::B` and `_` not covered
--> $DIR/doc-hidden-non-exhaustive.rs:27:11
|
LL | match HiddenEnum::A {
| ^^^^^^^^^^^^^ patterns `B` and `_` not covered
| ^^^^^^^^^^^^^ patterns `HiddenEnum::B` and `_` not covered
|
note: `HiddenEnum` defined here
--> $DIR/auxiliary/hidden.rs:3:5
Expand All @@ -55,14 +55,14 @@ LL | B,
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern, a match arm with multiple or-patterns as shown, or multiple match arms
|
LL ~ HiddenEnum::A => {}
LL + B | _ => todo!()
LL + HiddenEnum::B | _ => todo!()
|

error[E0004]: non-exhaustive patterns: `Some(B)` and `Some(_)` not covered
error[E0004]: non-exhaustive patterns: `Some(HiddenEnum::B)` and `Some(_)` not covered
--> $DIR/doc-hidden-non-exhaustive.rs:32:11
|
LL | match None {
| ^^^^ patterns `Some(B)` and `Some(_)` not covered
| ^^^^ patterns `Some(HiddenEnum::B)` and `Some(_)` not covered
|
note: `Option<HiddenEnum>` defined here
--> $SRC_DIR/core/src/option.rs:LL:COL
Expand All @@ -76,14 +76,14 @@ LL | Some(#[stable(feature = "rust1", since = "1.0.0")] T),
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern, a match arm with multiple or-patterns as shown, or multiple match arms
|
LL ~ Some(HiddenEnum::A) => {}
LL + Some(B) | Some(_) => todo!()
LL + Some(HiddenEnum::B) | Some(_) => todo!()
|

error[E0004]: non-exhaustive patterns: `C` not covered
error[E0004]: non-exhaustive patterns: `InCrate::C` not covered
--> $DIR/doc-hidden-non-exhaustive.rs:38:11
|
LL | match InCrate::A {
| ^^^^^^^^^^ pattern `C` not covered
| ^^^^^^^^^^ pattern `InCrate::C` not covered
|
note: `InCrate` defined here
--> $DIR/doc-hidden-non-exhaustive.rs:11:5
Expand All @@ -97,7 +97,7 @@ LL | C,
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
|
LL ~ InCrate::B => {}
LL + C => todo!()
LL + InCrate::C => todo!()
|

error: aborting due to 5 previous errors
Expand Down
Loading