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

feat: add non-exhaustive-let diagnostic #16303

Merged
merged 2 commits into from
Feb 19, 2024
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
80 changes: 64 additions & 16 deletions crates/hir-ty/src/diagnostics/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use hir_expand::name;
use itertools::Itertools;
use rustc_hash::FxHashSet;
use rustc_pattern_analysis::usefulness::{compute_match_usefulness, ValidityConstraint};
use tracing::debug;
use triomphe::Arc;
use typed_arena::Arena;

Expand Down Expand Up @@ -44,6 +45,10 @@ pub enum BodyValidationDiagnostic {
match_expr: ExprId,
uncovered_patterns: String,
},
NonExhaustiveLet {
pat: PatId,
uncovered_patterns: String,
},
RemoveTrailingReturn {
return_expr: ExprId,
},
Expand All @@ -57,26 +62,25 @@ impl BodyValidationDiagnostic {
let _p =
tracing::span!(tracing::Level::INFO, "BodyValidationDiagnostic::collect").entered();
let infer = db.infer(owner);
let mut validator = ExprValidator::new(owner, infer);
let body = db.body(owner);
let mut validator = ExprValidator { owner, body, infer, diagnostics: Vec::new() };
validator.validate_body(db);
validator.diagnostics
}
}

struct ExprValidator {
owner: DefWithBodyId,
body: Arc<Body>,
infer: Arc<InferenceResult>,
pub(super) diagnostics: Vec<BodyValidationDiagnostic>,
diagnostics: Vec<BodyValidationDiagnostic>,
}

impl ExprValidator {
fn new(owner: DefWithBodyId, infer: Arc<InferenceResult>) -> ExprValidator {
ExprValidator { owner, infer, diagnostics: Vec::new() }
}

fn validate_body(&mut self, db: &dyn HirDatabase) {
let body = db.body(self.owner);
let mut filter_map_next_checker = None;
// we'll pass &mut self while iterating over body.exprs, so they need to be disjoint
let body = Arc::clone(&self.body);

if matches!(self.owner, DefWithBodyId::FunctionId(_)) {
self.check_for_trailing_return(body.body_expr, &body);
Expand Down Expand Up @@ -106,6 +110,9 @@ impl ExprValidator {
Expr::If { .. } => {
self.check_for_unnecessary_else(id, expr, &body);
}
Expr::Block { .. } => {
self.validate_block(db, expr);
}
_ => {}
}
}
Expand Down Expand Up @@ -162,8 +169,6 @@ impl ExprValidator {
arms: &[MatchArm],
db: &dyn HirDatabase,
) {
let body = db.body(self.owner);

let scrut_ty = &self.infer[scrutinee_expr];
if scrut_ty.is_unknown() {
return;
Expand Down Expand Up @@ -191,12 +196,12 @@ impl ExprValidator {
.as_reference()
.map(|(match_expr_ty, ..)| match_expr_ty == pat_ty)
.unwrap_or(false))
&& types_of_subpatterns_do_match(arm.pat, &body, &self.infer)
&& types_of_subpatterns_do_match(arm.pat, &self.body, &self.infer)
{
// If we had a NotUsefulMatchArm diagnostic, we could
// check the usefulness of each pattern as we added it
// to the matrix here.
let pat = self.lower_pattern(&cx, arm.pat, db, &body, &mut has_lowering_errors);
let pat = self.lower_pattern(&cx, arm.pat, db, &mut has_lowering_errors);
let m_arm = pat_analysis::MatchArm {
pat: pattern_arena.alloc(pat),
has_guard: arm.guard.is_some(),
Expand Down Expand Up @@ -234,20 +239,63 @@ impl ExprValidator {
if !witnesses.is_empty() {
self.diagnostics.push(BodyValidationDiagnostic::MissingMatchArms {
match_expr,
uncovered_patterns: missing_match_arms(&cx, scrut_ty, witnesses, arms),
uncovered_patterns: missing_match_arms(&cx, scrut_ty, witnesses, m_arms.is_empty()),
});
}
}

fn validate_block(&mut self, db: &dyn HirDatabase, expr: &Expr) {
let Expr::Block { statements, .. } = expr else { return };
let pattern_arena = Arena::new();
let cx = MatchCheckCtx::new(self.owner.module(db.upcast()), self.owner, db);
for stmt in &**statements {
let &Statement::Let { pat, initializer, else_branch: None, .. } = stmt else {
continue;
};
let Some(initializer) = initializer else { continue };
let ty = &self.infer[initializer];

let mut have_errors = false;
let deconstructed_pat = self.lower_pattern(&cx, pat, db, &mut have_errors);
let match_arm = rustc_pattern_analysis::MatchArm {
pat: pattern_arena.alloc(deconstructed_pat),
has_guard: false,
arm_data: (),
};
if have_errors {
continue;
}

let report = match compute_match_usefulness(
&cx,
&[match_arm],
ty.clone(),
ValidityConstraint::ValidOnly,
) {
Ok(v) => v,
Err(e) => {
debug!(?e, "match usefulness error");
continue;
}
};
let witnesses = report.non_exhaustiveness_witnesses;
if !witnesses.is_empty() {
self.diagnostics.push(BodyValidationDiagnostic::NonExhaustiveLet {
pat,
uncovered_patterns: missing_match_arms(&cx, ty, witnesses, false),
});
}
}
}

fn lower_pattern<'p>(
&self,
cx: &MatchCheckCtx<'p>,
pat: PatId,
db: &dyn HirDatabase,
body: &Body,
have_errors: &mut bool,
) -> DeconstructedPat<'p> {
let mut patcx = match_check::PatCtxt::new(db, &self.infer, body);
let mut patcx = match_check::PatCtxt::new(db, &self.infer, &self.body);
let pattern = patcx.lower_pattern(pat);
let pattern = cx.lower_pat(&pattern);
if !patcx.errors.is_empty() {
Expand Down Expand Up @@ -448,7 +496,7 @@ fn missing_match_arms<'p>(
cx: &MatchCheckCtx<'p>,
scrut_ty: &Ty,
witnesses: Vec<WitnessPat<'p>>,
arms: &[MatchArm],
arms_is_empty: bool,
) -> String {
struct DisplayWitness<'a, 'p>(&'a WitnessPat<'p>, &'a MatchCheckCtx<'p>);
impl fmt::Display for DisplayWitness<'_, '_> {
Expand All @@ -463,7 +511,7 @@ fn missing_match_arms<'p>(
Some((AdtId::EnumId(e), _)) => !cx.db.enum_data(e).variants.is_empty(),
_ => false,
};
if arms.is_empty() && !non_empty_enum {
if arms_is_empty && !non_empty_enum {
format!("type `{}` is non-empty", scrut_ty.display(cx.db))
} else {
let pat_display = |witness| DisplayWitness(witness, cx);
Expand Down
23 changes: 23 additions & 0 deletions crates/hir/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ diagnostics![
MissingUnsafe,
MovedOutOfRef,
NeedMut,
NonExhaustiveLet,
NoSuchField,
PrivateAssocItem,
PrivateField,
Expand Down Expand Up @@ -280,6 +281,12 @@ pub struct MissingMatchArms {
pub uncovered_patterns: String,
}

#[derive(Debug)]
pub struct NonExhaustiveLet {
pub pat: InFile<AstPtr<ast::Pat>>,
pub uncovered_patterns: String,
}

#[derive(Debug)]
pub struct TypeMismatch {
pub expr_or_pat: InFile<AstPtr<Either<ast::Expr, ast::Pat>>>,
Expand Down Expand Up @@ -456,6 +463,22 @@ impl AnyDiagnostic {
Err(SyntheticSyntax) => (),
}
}
BodyValidationDiagnostic::NonExhaustiveLet { pat, uncovered_patterns } => {
match source_map.pat_syntax(pat) {
Ok(source_ptr) => {
if let Some(ast_pat) = source_ptr.value.cast::<ast::Pat>() {
return Some(
NonExhaustiveLet {
pat: InFile::new(source_ptr.file_id, ast_pat),
uncovered_patterns,
}
.into(),
);
}
}
Err(SyntheticSyntax) => {}
}
}
BodyValidationDiagnostic::RemoveTrailingReturn { return_expr } => {
if let Ok(source_ptr) = source_map.expr_syntax(return_expr) {
// Filters out desugared return expressions (e.g. desugared try operators).
Expand Down
2 changes: 1 addition & 1 deletion crates/ide-diagnostics/src/handlers/mutability_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ fn f() {
//- minicore: option
fn f(_: i32) {}
fn main() {
let ((Some(mut x), None) | (_, Some(mut x))) = (None, Some(7));
let ((Some(mut x), None) | (_, Some(mut x))) = (None, Some(7)) else { return };
//^^^^^ 💡 warn: variable does not need to be mutable
f(x);
}
Expand Down
47 changes: 47 additions & 0 deletions crates/ide-diagnostics/src/handlers/non_exhaustive_let.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
use crate::{Diagnostic, DiagnosticCode, DiagnosticsContext};

// Diagnostic: non-exhaustive-let
//
// This diagnostic is triggered if a `let` statement without an `else` branch has a non-exhaustive
// pattern.
pub(crate) fn non_exhaustive_let(
ctx: &DiagnosticsContext<'_>,
d: &hir::NonExhaustiveLet,
) -> Diagnostic {
Diagnostic::new_with_syntax_node_ptr(
ctx,
DiagnosticCode::RustcHardError("E0005"),
format!("non-exhaustive pattern: {}", d.uncovered_patterns),
d.pat.map(Into::into),
)
}

#[cfg(test)]
mod tests {
rosefromthedead marked this conversation as resolved.
Show resolved Hide resolved
use crate::tests::check_diagnostics;

#[test]
fn option_nonexhaustive() {
check_diagnostics(
r#"
//- minicore: option
fn main() {
let None = Some(5);
//^^^^ error: non-exhaustive pattern: `Some(_)` not covered
}
"#,
);
}

#[test]
fn option_exhaustive() {
check_diagnostics(
r#"
//- minicore: option
fn main() {
let Some(_) | None = Some(5);
}
"#,
);
}
}
2 changes: 2 additions & 0 deletions crates/ide-diagnostics/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ mod handlers {
pub(crate) mod moved_out_of_ref;
pub(crate) mod mutability_errors;
pub(crate) mod no_such_field;
pub(crate) mod non_exhaustive_let;
pub(crate) mod private_assoc_item;
pub(crate) mod private_field;
pub(crate) mod remove_trailing_return;
Expand Down Expand Up @@ -359,6 +360,7 @@ pub fn diagnostics(
AnyDiagnostic::MissingUnsafe(d) => handlers::missing_unsafe::missing_unsafe(&ctx, &d),
AnyDiagnostic::MovedOutOfRef(d) => handlers::moved_out_of_ref::moved_out_of_ref(&ctx, &d),
AnyDiagnostic::NeedMut(d) => handlers::mutability_errors::need_mut(&ctx, &d),
AnyDiagnostic::NonExhaustiveLet(d) => handlers::non_exhaustive_let::non_exhaustive_let(&ctx, &d),
AnyDiagnostic::NoSuchField(d) => handlers::no_such_field::no_such_field(&ctx, &d),
AnyDiagnostic::PrivateAssocItem(d) => handlers::private_assoc_item::private_assoc_item(&ctx, &d),
AnyDiagnostic::PrivateField(d) => handlers::private_field::private_field(&ctx, &d),
Expand Down
Loading