Skip to content

feat: min_exhaustive_patterns #17853

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

Merged
merged 3 commits into from
Aug 13, 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
25 changes: 12 additions & 13 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 10 additions & 11 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,11 @@ tt = { path = "./crates/tt", version = "0.0.0" }
vfs-notify = { path = "./crates/vfs-notify", version = "0.0.0" }
vfs = { path = "./crates/vfs", version = "0.0.0" }

ra-ap-rustc_lexer = { version = "0.53.0", default-features = false }
ra-ap-rustc_parse_format = { version = "0.53.0", default-features = false }
ra-ap-rustc_index = { version = "0.53.0", default-features = false }
ra-ap-rustc_abi = { version = "0.53.0", default-features = false }
ra-ap-rustc_pattern_analysis = { version = "0.53.0", default-features = false }
ra-ap-rustc_lexer = { version = "0.63.0", default-features = false }
ra-ap-rustc_parse_format = { version = "0.63.0", default-features = false }
ra-ap-rustc_index = { version = "0.63.0", default-features = false }
ra-ap-rustc_abi = { version = "0.63.0", default-features = false }
ra-ap-rustc_pattern_analysis = { version = "0.63.0", default-features = false }

# local crates that aren't published to crates.io. These should not have versions.
test-fixture = { path = "./crates/test-fixture" }
Expand Down Expand Up @@ -125,11 +125,11 @@ memmap2 = "0.5.4"
nohash-hasher = "0.2.0"
oorandom = "11.1.3"
object = { version = "0.33.0", default-features = false, features = [
"std",
"read_core",
"elf",
"macho",
"pe",
"std",
"read_core",
"elf",
"macho",
"pe",
] }
process-wrap = { version = "8.0.2", features = ["std"] }
pulldown-cmark-to-cmark = "10.0.4"
Expand Down Expand Up @@ -159,7 +159,6 @@ url = "2.3.1"
xshell = "0.2.5"



# We need to freeze the version of the crate, as the raw-api feature is considered unstable
dashmap = { version = "=5.5.3", features = ["raw-api"] }

Expand Down
65 changes: 57 additions & 8 deletions crates/hir-ty/src/diagnostics/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,33 @@

use std::fmt;

use chalk_solve::rust_ir::AdtKind;
use either::Either;
use hir_def::lang_item::LangItem;
use hir_def::{resolver::HasResolver, AdtId, AssocItemId, DefWithBodyId, HasModule};
use hir_def::{ItemContainerId, Lookup};
use hir_def::{
lang_item::LangItem,
resolver::{HasResolver, ValueNs},
AdtId, AssocItemId, DefWithBodyId, HasModule, ItemContainerId, Lookup,
};
use intern::sym;
use itertools::Itertools;
use rustc_hash::FxHashSet;
use rustc_pattern_analysis::constructor::Constructor;
use syntax::{ast, AstNode};
use syntax::{
ast::{self, UnaryOp},
AstNode,
};
use tracing::debug;
use triomphe::Arc;
use typed_arena::Arena;

use crate::Interner;
use crate::{
db::HirDatabase,
diagnostics::match_check::{
self,
pat_analysis::{self, DeconstructedPat, MatchCheckCtx, WitnessPat},
},
display::HirDisplay,
InferenceResult, Ty, TyExt,
Adjust, InferenceResult, Interner, Ty, TyExt, TyKind,
};

pub(crate) use hir_def::{
Expand Down Expand Up @@ -236,7 +241,12 @@ impl ExprValidator {
return;
}

let report = match cx.compute_match_usefulness(m_arms.as_slice(), scrut_ty.clone()) {
let known_valid_scrutinee = Some(self.is_known_valid_scrutinee(scrutinee_expr, db));
let report = match cx.compute_match_usefulness(
m_arms.as_slice(),
scrut_ty.clone(),
known_valid_scrutinee,
) {
Ok(report) => report,
Err(()) => return,
};
Expand All @@ -253,6 +263,45 @@ impl ExprValidator {
}
}

// [rustc's `is_known_valid_scrutinee`](https://github.com/rust-lang/rust/blob/c9bd03cb724e13cca96ad320733046cbdb16fbbe/compiler/rustc_mir_build/src/thir/pattern/check_match.rs#L288)
//
// While the above function in rustc uses thir exprs, r-a doesn't have them.
// So, the logic here is getting same result as "hir lowering + match with lowered thir"
// with "hir only"
fn is_known_valid_scrutinee(&self, scrutinee_expr: ExprId, db: &dyn HirDatabase) -> bool {
if self
.infer
.expr_adjustments
.get(&scrutinee_expr)
.is_some_and(|adjusts| adjusts.iter().any(|a| matches!(a.kind, Adjust::Deref(..))))
{
return false;
}

match &self.body[scrutinee_expr] {
Expr::UnaryOp { op: UnaryOp::Deref, .. } => false,
Expr::Path(path) => {
let value_or_partial = self
.owner
.resolver(db.upcast())
.resolve_path_in_value_ns_fully(db.upcast(), path);
value_or_partial.map_or(true, |v| !matches!(v, ValueNs::StaticId(_)))
}
Expr::Field { expr, .. } => match self.infer.type_of_expr[*expr].kind(Interner) {
TyKind::Adt(adt, ..)
if db.adt_datum(self.owner.krate(db.upcast()), *adt).kind == AdtKind::Union =>
{
false
}
_ => self.is_known_valid_scrutinee(*expr, db),
},
Expr::Index { base, .. } => self.is_known_valid_scrutinee(*base, db),
Expr::Cast { expr, .. } => self.is_known_valid_scrutinee(*expr, db),
Expr::Missing => false,
_ => true,
}
}

Copy link
Member Author

@ShoyuVanilla ShoyuVanilla Aug 11, 2024

Choose a reason for hiding this comment

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

fn validate_block(&mut self, db: &dyn HirDatabase, expr: &Expr) {
let (Expr::Block { statements, .. }
| Expr::Async { statements, .. }
Expand Down Expand Up @@ -285,7 +334,7 @@ impl ExprValidator {
has_guard: false,
arm_data: (),
};
let report = match cx.compute_match_usefulness(&[match_arm], ty.clone()) {
let report = match cx.compute_match_usefulness(&[match_arm], ty.clone(), None) {
Ok(v) => v,
Err(e) => {
debug!(?e, "match usefulness error");
Expand Down
25 changes: 11 additions & 14 deletions crates/hir-ty/src/diagnostics/match_check/pat_analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,22 +69,20 @@ pub(crate) struct MatchCheckCtx<'db> {
body: DefWithBodyId,
pub(crate) db: &'db dyn HirDatabase,
exhaustive_patterns: bool,
min_exhaustive_patterns: bool,
}

impl<'db> MatchCheckCtx<'db> {
pub(crate) fn new(module: ModuleId, body: DefWithBodyId, db: &'db dyn HirDatabase) -> Self {
let def_map = db.crate_def_map(module.krate());
let exhaustive_patterns = def_map.is_unstable_feature_enabled(&sym::exhaustive_patterns);
let min_exhaustive_patterns =
def_map.is_unstable_feature_enabled(&sym::min_exhaustive_patterns);
Self { module, body, db, exhaustive_patterns, min_exhaustive_patterns }
Self { module, body, db, exhaustive_patterns }
}

pub(crate) fn compute_match_usefulness(
&self,
arms: &[MatchArm<'db>],
scrut_ty: Ty,
known_valid_scrutinee: Option<bool>,
) -> Result<UsefulnessReport<'db, Self>, ()> {
if scrut_ty.contains_unknown() {
return Err(());
Expand All @@ -95,8 +93,7 @@ impl<'db> MatchCheckCtx<'db> {
}
}

// FIXME: Determine place validity correctly. For now, err on the safe side.
let place_validity = PlaceValidity::MaybeInvalid;
let place_validity = PlaceValidity::from_bool(known_valid_scrutinee.unwrap_or(true));
Copy link
Member Author

Choose a reason for hiding this comment

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

// Measured to take ~100ms on modern hardware.
let complexity_limit = Some(500000);
compute_match_usefulness(self, arms, scrut_ty, place_validity, complexity_limit)
Expand Down Expand Up @@ -307,7 +304,8 @@ impl<'db> MatchCheckCtx<'db> {
&Str(void) => match void {},
Wildcard | NonExhaustive | Hidden | PrivateUninhabited => PatKind::Wild,
Never => PatKind::Never,
Missing | F32Range(..) | F64Range(..) | Opaque(..) | Or => {
Missing | F16Range(..) | F32Range(..) | F64Range(..) | F128Range(..) | Opaque(..)
| Or => {
never!("can't convert to pattern: {:?}", pat.ctor());
PatKind::Wild
}
Expand All @@ -327,9 +325,6 @@ impl<'db> PatCx for MatchCheckCtx<'db> {
fn is_exhaustive_patterns_feature_on(&self) -> bool {
self.exhaustive_patterns
}
fn is_min_exhaustive_patterns_feature_on(&self) -> bool {
self.min_exhaustive_patterns
}

fn ctor_arity(
&self,
Expand All @@ -356,8 +351,9 @@ impl<'db> PatCx for MatchCheckCtx<'db> {
},
Ref => 1,
Slice(..) => unimplemented!(),
Never | Bool(..) | IntRange(..) | F32Range(..) | F64Range(..) | Str(..)
| Opaque(..) | NonExhaustive | PrivateUninhabited | Hidden | Missing | Wildcard => 0,
Never | Bool(..) | IntRange(..) | F16Range(..) | F32Range(..) | F64Range(..)
| F128Range(..) | Str(..) | Opaque(..) | NonExhaustive | PrivateUninhabited
| Hidden | Missing | Wildcard => 0,
Or => {
never!("The `Or` constructor doesn't have a fixed arity");
0
Expand Down Expand Up @@ -419,8 +415,9 @@ impl<'db> PatCx for MatchCheckCtx<'db> {
}
},
Slice(_) => unreachable!("Found a `Slice` constructor in match checking"),
Never | Bool(..) | IntRange(..) | F32Range(..) | F64Range(..) | Str(..)
| Opaque(..) | NonExhaustive | PrivateUninhabited | Hidden | Missing | Wildcard => {
Never | Bool(..) | IntRange(..) | F16Range(..) | F32Range(..) | F64Range(..)
| F128Range(..) | Str(..) | Opaque(..) | NonExhaustive | PrivateUninhabited
| Hidden | Missing | Wildcard => {
smallvec![]
}
Or => {
Expand Down
38 changes: 38 additions & 0 deletions crates/ide-diagnostics/src/handlers/missing_match_arms.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1032,6 +1032,44 @@ fn f() {
check_diagnostics_no_bails(&code);
}

#[test]
fn min_exhaustive() {
check_diagnostics(
r#"
//- minicore: result
fn test(x: Result<i32, !>) {
match x {
Ok(_y) => {}
}
}
"#,
);
check_diagnostics(
r#"
//- minicore: result
fn test(ptr: *const Result<i32, !>) {
unsafe {
match *ptr {
//^^^^ error: missing match arm: `Err(!)` not covered
Ok(_x) => {}
}
}
}
"#,
);
check_diagnostics(
r#"
//- minicore: result
fn test(x: Result<i32, &'static !>) {
match x {
//^ error: missing match arm: `Err(_)` not covered
Ok(_y) => {}
}
}
"#,
);
}

mod rust_unstable {
use super::*;

Expand Down
22 changes: 22 additions & 0 deletions crates/ide-diagnostics/src/handlers/non_exhaustive_let.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,28 @@ fn main() {
//^^^^ error: non-exhaustive pattern: `Some(_)` not covered
}
}
"#,
);
}

#[test]
fn min_exhaustive() {
check_diagnostics(
r#"
//- minicore: result
fn test(x: Result<i32, !>) {
let Ok(_y) = x;
}
"#,
);
Copy link
Member Author

@ShoyuVanilla ShoyuVanilla Aug 11, 2024

Choose a reason for hiding this comment

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

The reason that this test doesn't have a pointer deref case is because the following code;

fn test(ptr: *const Result<i32, !>) {
    unsafe {
        let Ok(_x) = *ptr;
    }
}

is getting a block with no stmts but tail one in here(thus, no diagnostic error),

fn validate_block(&mut self, db: &dyn HirDatabase, expr: &Expr) {
let Expr::Block { statements, .. } = expr else { return };

while the following is getting a block with a single stmt without tail 🤔

fn test(x: Result<i32, &'static !>) {
    let Ok(_y) = x;
}

I'll make a more deep inspection and file this as a new issue

Copy link
Member

Choose a reason for hiding this comment

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

is getting a block with no stmts but tail one in here(thus, no diagnostic error),

oh no that sounds very wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it looks very wrong 🤔 I'll check the entire lowering process from AST to hir once I get back home

Copy link
Member Author

@ShoyuVanilla ShoyuVanilla Aug 12, 2024

Choose a reason for hiding this comment

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

This is because of #17865

Copy link
Member Author

Choose a reason for hiding this comment

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

I got into this 😂 rust-lang/rust#129009


check_diagnostics(
r#"
//- minicore: result
fn test(x: Result<i32, &'static !>) {
let Ok(_y) = x;
//^^^^^^ error: non-exhaustive pattern: `Err(_)` not covered
}
"#,
);
}
Expand Down