Skip to content

Fix code suggestion for local enum patterns in non-exhaustive matches #137783

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
10 changes: 5 additions & 5 deletions compiler/rustc_mir_build/src/thir/pattern/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -538,8 +538,7 @@ impl<'p, 'tcx> MatchVisitor<'p, 'tcx> {
self.error = Err(report_non_exhaustive_match(
&cx,
self.thir,
scrut.ty,
scrut.span,
scrut,
witnesses,
arms,
braces_span,
Expand Down Expand Up @@ -1205,12 +1204,13 @@ fn pat_is_catchall(pat: &DeconstructedPat<'_, '_>) -> bool {
fn report_non_exhaustive_match<'p, 'tcx>(
cx: &PatCtxt<'p, 'tcx>,
thir: &Thir<'tcx>,
scrut_ty: Ty<'tcx>,
sp: Span,
scrut: &Expr<'tcx>,
witnesses: Vec<WitnessPat<'p, 'tcx>>,
arms: &[ArmId],
braces_span: Option<Span>,
) -> ErrorGuaranteed {
let scrut_ty = scrut.ty;
let sp = scrut.span;
let is_empty_match = arms.is_empty();
let non_empty_enum = match scrut_ty.kind() {
ty::Adt(def, _) => def.is_enum() && !def.variants().is_empty(),
Expand Down Expand Up @@ -1323,7 +1323,7 @@ fn report_non_exhaustive_match<'p, 'tcx>(
let suggested_arm = if suggest_the_witnesses {
let pattern = witnesses
.iter()
.map(|witness| cx.print_witness_pat(witness))
.map(|witness| cx.print_witness_pat_with_scrut(witness, Some(scrut)))
.collect::<Vec<String>>()
.join(" | ");
if witnesses.iter().all(|p| p.is_never_pattern()) && cx.tcx.features().never_patterns() {
Expand Down
11 changes: 10 additions & 1 deletion compiler/rustc_pattern_analysis/src/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -805,10 +805,18 @@ impl<'p, 'tcx: 'p> RustcPatCtxt<'p, 'tcx> {
}
}

pub fn print_witness_pat(&self, pat: &WitnessPat<'p, 'tcx>) -> String {
self.print_witness_pat_with_scrut(pat, None)
}

/// Prints a [`WitnessPat`] to an owned string, for diagnostic purposes.
///
/// This panics for patterns that don't appear in diagnostics, like float ranges.
pub fn print_witness_pat(&self, pat: &WitnessPat<'p, 'tcx>) -> String {
pub fn print_witness_pat_with_scrut(
&self,
pat: &WitnessPat<'p, 'tcx>,
scrut: Option<&thir::Expr<'tcx>>,
) -> String {
let cx = self;
let print = |p| cx.print_witness_pat(p);
match pat.ctor() {
Expand Down Expand Up @@ -847,6 +855,7 @@ impl<'p, 'tcx: 'p> RustcPatCtxt<'p, 'tcx> {
pat.ty().inner(),
&enum_info,
&subpatterns,
scrut,
)
.unwrap();
s
Expand Down
38 changes: 35 additions & 3 deletions compiler/rustc_pattern_analysis/src/rustc/print.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Pattern analysis sometimes wants to print patterns as part of a user-visible
//! diagnostic.
//!
//! Historically it did so by creating a synthetic [`thir::Pat`](rustc_middle::thir::Pat)
//! Historically it did so by creating a synthetic [`thir::Pat`]
//! and printing that, but doing so was making it hard to modify the THIR pattern
//! representation for other purposes.
//!
Expand All @@ -12,8 +12,8 @@
use std::fmt;

use rustc_abi::{FieldIdx, VariantIdx};
use rustc_middle::bug;
use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt};
use rustc_middle::{bug, thir};
use rustc_span::sym;

#[derive(Clone, Debug)]
Expand Down Expand Up @@ -44,12 +44,33 @@ pub(crate) enum EnumInfo<'tcx> {
NotEnum,
}

fn erase_path_if_local<'tcx>(
Copy link
Member

Choose a reason for hiding this comment

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

Can you document what this function is meant to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
fn erase_path_if_local<'tcx>(
/// Determines whether to trim the enum path.
///
/// This decision is based on whether the `scrutinee` expression and
/// the enum are within the same function or method.
fn trim_paths_if_local<'tcx>(

Copy link
Member

Choose a reason for hiding this comment

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

Doc looks good, I would name it something like should_trim_enum_paths.

tcx: TyCtxt<'_>,
adt_def: AdtDef<'tcx>,
scrut: &thir::Expr<'tcx>,
) -> bool {
let enum_parent_def_id = tcx.parent(adt_def.did());
let scrut_parent_def_id = if let thir::ExprKind::Scope { region_scope: _, lint_level, value: _ } =
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super familiar with how lint scopes work in THIR, but this feels very un-robust: there seem to be many reasons for this expression not to be a scope. Can you not use self.match_lint_level (which is the hir id of the match I believe)?

Copy link
Contributor Author

@makai410 makai410 Mar 16, 2025

Choose a reason for hiding this comment

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

This function wants a scope expression surrounding the scrutinee, like:

match v {}
      ^ -- scope surrounding this

Then we can get the def id of the scrutinee's owner.

Sorry that I can't find an approach to get the def id by not using lint_level here. :(

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure a match and its scrutinee always have the same owner def_id; you should be able to use self.match_lint_level.owner.to_def_id().

scrut.kind
&& let thir::LintLevel::Explicit(hir_id) = lint_level
{
Some(hir_id.owner.to_def_id())
} else {
None
};
if scrut_parent_def_id == Some(enum_parent_def_id) {
return true;
}
false
}

pub(crate) fn write_struct_like<'tcx>(
f: &mut impl fmt::Write,
tcx: TyCtxt<'_>,
ty: Ty<'tcx>,
enum_info: &EnumInfo<'tcx>,
subpatterns: &[FieldPat],
scrut: Option<&thir::Expr<'tcx>>,
) -> fmt::Result {
let variant_and_name = match *enum_info {
EnumInfo::Enum { adt_def, variant_index } => {
Expand All @@ -60,7 +81,18 @@ pub(crate) fn write_struct_like<'tcx>(
{
variant.name.to_string()
} else {
format!("{}::{}", tcx.def_path_str(adt_def.did()), variant.name)
let enum_and_variant = if let Some(scrut) = scrut
&& erase_path_if_local(tcx, adt_def, scrut)
{
ty::print::with_forced_trimmed_paths!(format!(
"{}::{}",
tcx.def_path_str(adt_def.did()),
variant.name
))
Comment on lines +87 to +91
Copy link
Member

Choose a reason for hiding this comment

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

This feels quite ad-hoc: the underlying issue is that def_path_str does not give a path that can be used in code. Shouldn't there be a function that can return usable paths for a target def_id in the context of a source def_id?

Copy link
Contributor Author

@makai410 makai410 Mar 16, 2025

Choose a reason for hiding this comment

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

My idea here is that if the scrutinee and the enum both have the same owner (which means they are at the same function or method), then trimming the path is ok because we just want the enum name here(I guess).

Copy link
Contributor Author

@makai410 makai410 Mar 18, 2025

Choose a reason for hiding this comment

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

Oh perhaps I got what you were worrying about, cc #137845.
It seems that they try to suggest a trimmed paths name in code only requiring that the name is unique, even if the things are in a different module.
Once users apply these code suggestions, then just let the compiler emit the suggestions like "use xxx;" to take users to the right way, which may be the idea of the design (btw it's just my guess, i ain't investigated it for now).

So here the problem is that the path is totally wrong because it is a function name which should not appear.

Idk if I got to the right way but I appreciate it if you're willing to point my problems out.

Copy link
Member

Choose a reason for hiding this comment

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

My worry is that this is a very special-cased fix, where the general underlying issue is "how to emit correct paths in suggestions". Maybe @estebank you would know about this? What's the correct way to get a path that can be used in a suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see... No clues at hand right now. But would be happy to be mentored to solve this.

Copy link
Member

Choose a reason for hiding this comment

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

You could open a thread in the T-compiler/diagnostics stream asking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I just opened a thread to talk about it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, sorry for leaving you without a clear path forward

} else {
format!("{}::{}", tcx.def_path_str(adt_def.did()), variant.name)
};
enum_and_variant
};
Some((variant, name))
}
Expand Down
13 changes: 13 additions & 0 deletions tests/ui/rfcs/rfc-2008-non-exhaustive/shadowed-non-exhaustive.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
struct Shadowed {}

fn main() {
let v = Shadowed::Foo;

match v {
//~^ non-exhaustive patterns: `main::Shadowed::Foo` not covered [E0004]
}

enum Shadowed {
Foo,
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
error[E0004]: non-exhaustive patterns: `main::Shadowed::Foo` not covered
--> $DIR/shadowed-non-exhaustive.rs:6:11
|
LL | match v {
| ^ pattern `main::Shadowed::Foo` not covered
|
note: `main::Shadowed` defined here
--> $DIR/shadowed-non-exhaustive.rs:10:10
|
LL | enum Shadowed {
| ^^^^^^^^
LL | Foo,
| --- not covered
= note: the matched value is of type `main::Shadowed`
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 v {
LL + Shadowed::Foo => todo!(),
LL + }
|

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0004`.
Loading