Skip to content

internal: calculate pattern adjustments #9105

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 2 commits into from
Jun 3, 2021
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
23 changes: 13 additions & 10 deletions crates/hir_ty/src/diagnostics/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,17 +357,20 @@ impl<'a, 'b> ExprValidator<'a, 'b> {
infer: &infer,
db,
pattern_arena: &pattern_arena,
eprint_panic_context: &|| {
panic_context: &|| {
use syntax::AstNode;
if let Ok(scrutinee_sptr) = source_map.expr_syntax(match_expr) {
let root = scrutinee_sptr.file_syntax(db.upcast());
if let Some(match_ast) = scrutinee_sptr.value.to_node(&root).syntax().parent() {
eprintln!(
"Match checking is about to panic on this expression:\n{}",
match_ast.to_string(),
);
}
}
let match_expr_text = source_map
.expr_syntax(match_expr)
.ok()
.and_then(|scrutinee_sptr| {
let root = scrutinee_sptr.file_syntax(db.upcast());
scrutinee_sptr.value.to_node(&root).syntax().parent()
})
.map(|node| node.to_string());
format!(
"expression:\n{}",
match_expr_text.as_deref().unwrap_or("<synthesized expr>")
)
},
};
let report = compute_match_usefulness(&cx, &m_arms);
Expand Down
28 changes: 26 additions & 2 deletions crates/hir_ty/src/diagnostics/match_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,19 @@ impl<'a> PatCtxt<'a> {
}

pub(crate) fn lower_pattern(&mut self, pat: hir_def::expr::PatId) -> Pat {
// FIXME: implement pattern adjustments (implicit pattern dereference; "RFC 2005-match-ergonomics")
// XXX(iDawer): Collecting pattern adjustments feels imprecise to me.
// When lowering of & and box patterns are implemented this should be tested
// in a manner of `match_ergonomics_issue_9095` test.
// Pattern adjustment is part of RFC 2005-match-ergonomics.
// More info https://github.com/rust-lang/rust/issues/42640#issuecomment-313535089
let unadjusted_pat = self.lower_pattern_unadjusted(pat);
unadjusted_pat
self.infer.pat_adjustments.get(&pat).map(|it| &**it).unwrap_or_default().iter().rev().fold(
unadjusted_pat,
|subpattern, ref_ty| Pat {
ty: ref_ty.clone(),
kind: Box::new(PatKind::Deref { subpattern }),
},
)
}

fn lower_pattern_unadjusted(&mut self, pat: hir_def::expr::PatId) -> Pat {
Expand Down Expand Up @@ -1236,6 +1245,21 @@ fn main(f: Foo) {
);
}

#[test]
fn match_ergonomics_issue_9095() {
check_diagnostics(
r#"
enum Foo<T> { A(T) }
fn main() {
match &Foo::A(true) {
_ => {}
Foo::A(_) => {}
}
}
"#,
);
}

mod false_negatives {
//! The implementation of match checking here is a work in progress. As we roll this out, we
//! prefer false negatives to false positives (ideally there would be no false positives). This
Expand Down
5 changes: 2 additions & 3 deletions crates/hir_ty/src/diagnostics/match_check/usefulness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ pub(crate) struct MatchCheckCtx<'a> {
pub(crate) db: &'a dyn HirDatabase,
/// Lowered patterns from arms plus generated by the check.
pub(crate) pattern_arena: &'a RefCell<PatternArena>,
pub(crate) eprint_panic_context: &'a dyn Fn(),
pub(crate) panic_context: &'a dyn Fn() -> String,
}

impl<'a> MatchCheckCtx<'a> {
Expand Down Expand Up @@ -331,8 +331,7 @@ impl<'a> MatchCheckCtx<'a> {

#[track_caller]
pub(super) fn bug(&self, info: &str) -> ! {
Copy link
Member

@flodiebold flodiebold Jun 3, 2021

Choose a reason for hiding this comment

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

(note: I haven't checked where this is called, but in general we want to be a bit more resilient about panicking on errors than rustc: Showing some inaccurate information in the IDE is often better than breaking completely. Hence the always! macro etc. that panic on debug assertions, but only log and continue in release builds. In the case of match check, it'd be ideal if we could just skip the match if we encounter some bug condition, if it's not too complicated 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.

Interesting. This is worth some experimenting with fallible implementation then including it into future libraryfied match checking.
For now it just relies on correct lowering and filtering out erroneous input in match_check::PatCtxt::lower_pattern. But in case of bugs, yeah.. that's bad.

Copy link
Member

Choose a reason for hiding this comment

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

Note, it's not worth making functions return results to avoid assertions. It's just, if there's a straightforward way to e.g. return no errors instead of panicking on an assertion, we should do that.

(self.eprint_panic_context)();
panic!("bug: {}", info);
panic!("bug: {}\n{}", info, (self.panic_context)());
}
}

Expand Down
2 changes: 2 additions & 0 deletions crates/hir_ty/src/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@ pub struct InferenceResult {
type_mismatches: FxHashMap<ExprOrPatId, TypeMismatch>,
/// Interned Unknown to return references to.
standard_types: InternedStandardTypes,
/// Stores the types which were implicitly dereferenced in pattern binding modes.
pub pat_adjustments: FxHashMap<PatId, Vec<Ty>>,
}

impl InferenceResult {
Expand Down
7 changes: 7 additions & 0 deletions crates/hir_ty/src/infer/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,21 @@ impl<'a> InferenceContext<'a> {
let mut expected = self.resolve_ty_shallow(expected);

if is_non_ref_pat(&body, pat) {
let mut pat_adjustments = Vec::new();
while let Some((inner, _lifetime, mutability)) = expected.as_reference() {
pat_adjustments.push(expected.clone());
expected = self.resolve_ty_shallow(inner);
default_bm = match default_bm {
BindingMode::Move => BindingMode::Ref(mutability),
BindingMode::Ref(Mutability::Not) => BindingMode::Ref(Mutability::Not),
BindingMode::Ref(Mutability::Mut) => BindingMode::Ref(mutability),
}
}

if !pat_adjustments.is_empty() {
pat_adjustments.shrink_to_fit();
self.result.pat_adjustments.insert(pat, pat_adjustments);
}
} else if let Pat::Ref { .. } = &body[pat] {
cov_mark::hit!(match_ergonomics_ref);
// When you encounter a `&pat` pattern, reset to Move.
Expand Down