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

fix: Lower range pattern bounds to expressions #18995

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
10 changes: 5 additions & 5 deletions crates/hir-def/src/body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,9 @@ pub struct BodySourceMap {
// AST expressions can create patterns in destructuring assignments. Therefore, `ExprSource` can also map
// to `PatId`, and `PatId` can also map to `ExprSource` (the other way around is unaffected).
expr_map: FxHashMap<ExprSource, ExprOrPatId>,
expr_map_back: ArenaMap<ExprId, ExprSource>,
expr_map_back: ArenaMap<ExprId, ExprOrPatSource>,

pat_map: FxHashMap<PatSource, PatId>,
pat_map: FxHashMap<PatSource, ExprOrPatId>,
pat_map_back: ArenaMap<PatId, ExprOrPatSource>,

label_map: FxHashMap<LabelSource, LabelId>,
Expand Down Expand Up @@ -738,12 +738,12 @@ impl Index<TypeRefId> for Body {
impl BodySourceMap {
pub fn expr_or_pat_syntax(&self, id: ExprOrPatId) -> Result<ExprOrPatSource, SyntheticSyntax> {
match id {
ExprOrPatId::ExprId(id) => self.expr_syntax(id).map(|it| it.map(AstPtr::wrap_left)),
ExprOrPatId::ExprId(id) => self.expr_syntax(id),
ExprOrPatId::PatId(id) => self.pat_syntax(id),
}
}

pub fn expr_syntax(&self, expr: ExprId) -> Result<ExprSource, SyntheticSyntax> {
pub fn expr_syntax(&self, expr: ExprId) -> Result<ExprOrPatSource, SyntheticSyntax> {
self.expr_map_back.get(expr).cloned().ok_or(SyntheticSyntax)
}

Expand Down Expand Up @@ -771,7 +771,7 @@ impl BodySourceMap {
self.self_param
}

pub fn node_pat(&self, node: InFile<&ast::Pat>) -> Option<PatId> {
pub fn node_pat(&self, node: InFile<&ast::Pat>) -> Option<ExprOrPatId> {
self.pat_map.get(&node.map(AstPtr::new)).cloned()
}

Expand Down
59 changes: 42 additions & 17 deletions crates/hir-def/src/body/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ use crate::{
FormatPlaceholder, FormatSign, FormatTrait,
},
Array, Binding, BindingAnnotation, BindingId, BindingProblems, CaptureBy, ClosureKind,
Expr, ExprId, Item, Label, LabelId, Literal, LiteralOrConst, MatchArm, Movability,
OffsetOf, Pat, PatId, RecordFieldPat, RecordLitField, Statement,
Expr, ExprId, Item, Label, LabelId, Literal, MatchArm, Movability, OffsetOf, Pat, PatId,
RecordFieldPat, RecordLitField, Statement,
},
item_scope::BuiltinShadowMode,
lang_item::LangItem,
Expand Down Expand Up @@ -1716,23 +1716,39 @@ impl ExprCollector<'_> {
self.collect_macro_call(call, macro_ptr, true, |this, expanded_pat| {
this.collect_pat_opt(expanded_pat, binding_list)
});
self.source_map.pat_map.insert(src, pat);
self.source_map.pat_map.insert(src, pat.into());
return pat;
}
None => Pat::Missing,
},
// FIXME: implement in a way that also builds source map and calculates assoc resolutions in type inference.
ast::Pat::RangePat(p) => {
let mut range_part_lower = |p: Option<ast::Pat>| {
p.and_then(|it| match &it {
ast::Pat::LiteralPat(it) => {
Some(Box::new(LiteralOrConst::Literal(pat_literal_to_hir(it)?.0)))
}
pat @ (ast::Pat::IdentPat(_) | ast::Pat::PathPat(_)) => {
let subpat = self.collect_pat(pat.clone(), binding_list);
Some(Box::new(LiteralOrConst::Const(subpat)))
let mut range_part_lower = |p: Option<ast::Pat>| -> Option<ExprId> {
p.and_then(|it| {
let ptr = PatPtr::new(&it);
match &it {
ast::Pat::LiteralPat(it) => Some(self.alloc_expr_from_pat(
Expr::Literal(pat_literal_to_hir(it)?.0),
ptr,
)),
ast::Pat::IdentPat(ident) => {
if ident.is_simple_ident() {
Comment on lines +1733 to +1734
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ast::Pat::IdentPat(ident) => {
if ident.is_simple_ident() {
// treat a bare ident as a path
ast::Pat::IdentPat(ident) if ident.is_simple_ident() => {

Then we could drop the return

return ident
.name()
.map(|name| name.as_name())
.map(Path::from)
.map(|path| {
self.alloc_expr_from_pat(Expr::Path(path), ptr)
});
}

None
}
ast::Pat::PathPat(p) => p
.path()
.and_then(|path| self.parse_path(path))
.map(|parsed| self.alloc_expr_from_pat(Expr::Path(parsed), ptr)),
_ => None,
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe add a comment here (or on top of the match) regarding the unhandled variants.

We only need to handle literal, ident (if bare) and path patterns here, as any other pattern as a range pattern operand is semantically invalid.

}
_ => None,
})
};
let start = range_part_lower(p.start());
Expand Down Expand Up @@ -1795,7 +1811,7 @@ impl ExprCollector<'_> {
}
});
if let Some(pat) = pat.left() {
self.source_map.pat_map.insert(src, pat);
self.source_map.pat_map.insert(src, pat.into());
}
pat
}
Expand Down Expand Up @@ -2412,7 +2428,7 @@ impl ExprCollector<'_> {
fn alloc_expr(&mut self, expr: Expr, ptr: ExprPtr) -> ExprId {
let src = self.expander.in_file(ptr);
let id = self.body.exprs.alloc(expr);
self.source_map.expr_map_back.insert(id, src);
self.source_map.expr_map_back.insert(id, src.map(AstPtr::wrap_left));
self.source_map.expr_map.insert(src, id.into());
id
}
Expand All @@ -2424,7 +2440,7 @@ impl ExprCollector<'_> {
fn alloc_expr_desugared_with_ptr(&mut self, expr: Expr, ptr: ExprPtr) -> ExprId {
let src = self.expander.in_file(ptr);
let id = self.body.exprs.alloc(expr);
self.source_map.expr_map_back.insert(id, src);
self.source_map.expr_map_back.insert(id, src.map(AstPtr::wrap_left));
// We intentionally don't fill this as it could overwrite a non-desugared entry
// self.source_map.expr_map.insert(src, id);
id
Expand All @@ -2448,11 +2464,20 @@ impl ExprCollector<'_> {
self.source_map.pat_map_back.insert(id, src.map(AstPtr::wrap_left));
id
}

fn alloc_expr_from_pat(&mut self, expr: Expr, ptr: PatPtr) -> ExprId {
let src = self.expander.in_file(ptr);
let id = self.body.exprs.alloc(expr);
self.source_map.pat_map.insert(src, id.into());
self.source_map.expr_map_back.insert(id, src.map(AstPtr::wrap_right));
id
}

fn alloc_pat(&mut self, pat: Pat, ptr: PatPtr) -> PatId {
let src = self.expander.in_file(ptr);
let id = self.body.pats.alloc(pat);
self.source_map.pat_map_back.insert(id, src.map(AstPtr::wrap_right));
self.source_map.pat_map.insert(src, id);
self.source_map.pat_map.insert(src, id.into());
id
}
// FIXME: desugared pats don't have ptr, that's wrong and should be fixed somehow.
Expand Down
16 changes: 3 additions & 13 deletions crates/hir-def/src/body/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@ use itertools::Itertools;
use span::Edition;

use crate::{
hir::{
Array, BindingAnnotation, CaptureBy, ClosureKind, Literal, LiteralOrConst, Movability,
Statement,
},
hir::{Array, BindingAnnotation, CaptureBy, ClosureKind, Literal, Movability, Statement},
pretty::{print_generic_args, print_path, print_type_ref},
};

Expand Down Expand Up @@ -656,11 +653,11 @@ impl Printer<'_> {
}
Pat::Range { start, end } => {
if let Some(start) = start {
self.print_literal_or_const(start);
self.print_expr(*start);
}
w!(self, "..=");
if let Some(end) = end {
self.print_literal_or_const(end);
self.print_expr(*end);
}
}
Pat::Slice { prefix, slice, suffix } => {
Expand Down Expand Up @@ -757,13 +754,6 @@ impl Printer<'_> {
}
}

fn print_literal_or_const(&mut self, literal_or_const: &LiteralOrConst) {
match literal_or_const {
LiteralOrConst::Literal(l) => self.print_literal(l),
LiteralOrConst::Const(c) => self.print_pat(*c),
}
}

fn print_literal(&mut self, literal: &Literal) {
match literal {
Literal::String(it) => w!(self, "{:?}", it),
Expand Down
45 changes: 43 additions & 2 deletions crates/hir-def/src/body/tests.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
mod block;

use crate::{hir::MatchArm, test_db::TestDB, ModuleDefId};
use expect_test::{expect, Expect};
use test_fixture::WithFixture;

use crate::{test_db::TestDB, ModuleDefId};

use super::*;

fn lower(#[rust_analyzer::rust_fixture] ra_fixture: &str) -> (TestDB, Arc<Body>, DefWithBodyId) {
Expand Down Expand Up @@ -459,3 +458,45 @@ async fn foo(a: (), b: i32) -> u32 {
expect!["fn foo(�: (), �: i32) -> impl ::core::future::Future::<Output = u32> �"]
.assert_eq(&printed);
}

#[test]
fn range_bounds_are_hir_exprs() {
let (_, body, _) = lower(
r#"
pub const L: i32 = 6;
mod x {
pub const R: i32 = 100;
}
const fn f(x: i32) -> i32 {
match x {
-1..=5 => x * 10,
L..=x::R => x * 100,
_ => x,
}
}"#,
);

let mtch_arms = body
.exprs
.iter()
.find_map(|(_, expr)| {
if let Expr::Match { arms, .. } = expr {
return Some(arms);
}

None
})
.unwrap();

let MatchArm { pat, .. } = mtch_arms[1];
match body.pats[pat] {
Pat::Range { start, end } => {
let hir_start = &body.exprs[start.unwrap()];
let hir_end = &body.exprs[end.unwrap()];

assert!(matches!(hir_start, Expr::Path { .. }));
assert!(matches!(hir_end, Expr::Path { .. }));
}
_ => {}
}
}
12 changes: 10 additions & 2 deletions crates/hir-def/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,20 @@ impl ExprOrPatId {
}
}

pub fn is_expr(&self) -> bool {
matches!(self, Self::ExprId(_))
}

pub fn as_pat(self) -> Option<PatId> {
match self {
Self::PatId(v) => Some(v),
_ => None,
}
}

pub fn is_pat(&self) -> bool {
matches!(self, Self::PatId(_))
}
}
stdx::impl_from!(ExprId, PatId for ExprOrPatId);

Expand Down Expand Up @@ -574,8 +582,8 @@ pub enum Pat {
ellipsis: bool,
},
Range {
start: Option<Box<LiteralOrConst>>,
end: Option<Box<LiteralOrConst>>,
start: Option<ExprId>,
end: Option<ExprId>,
},
Slice {
prefix: Box<[PatId]>,
Expand Down
4 changes: 3 additions & 1 deletion crates/hir-ty/src/diagnostics/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,9 @@ impl ExprValidator {
return;
};
let root = source_ptr.file_syntax(db.upcast());
let ast::Expr::IfExpr(if_expr) = source_ptr.value.to_node(&root) else {
let either::Left(ast::Expr::IfExpr(if_expr)) =
source_ptr.value.to_node(&root)
else {
return;
};
let mut top_if_expr = if_expr;
Expand Down
25 changes: 9 additions & 16 deletions crates/hir-ty/src/mir/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use hir_def::{
body::{Body, HygieneId},
data::adt::{StructKind, VariantData},
hir::{
ArithOp, Array, BinaryOp, BindingAnnotation, BindingId, ExprId, LabelId, Literal,
LiteralOrConst, MatchArm, Pat, PatId, RecordFieldPat, RecordLitField,
ArithOp, Array, BinaryOp, BindingAnnotation, BindingId, ExprId, LabelId, Literal, MatchArm,
Pat, PatId, RecordFieldPat, RecordLitField,
},
lang_item::{LangItem, LangItemTarget},
path::Path,
Expand Down Expand Up @@ -1358,20 +1358,10 @@ impl<'ctx> MirLowerCtx<'ctx> {
Ok(())
}

fn lower_literal_or_const_to_operand(
&mut self,
ty: Ty,
loc: &LiteralOrConst,
) -> Result<Operand> {
match loc {
LiteralOrConst::Literal(l) => self.lower_literal_to_operand(ty, l),
LiteralOrConst::Const(c) => {
let c = match &self.body.pats[*c] {
Pat::Path(p) => p,
_ => not_supported!(
"only `char` and numeric types are allowed in range patterns"
),
};
fn lower_literal_or_const_to_operand(&mut self, ty: Ty, loc: &ExprId) -> Result<Operand> {
match &self.body.exprs[*loc] {
Expr::Literal(l) => self.lower_literal_to_operand(ty, l),
Expr::Path(c) => {
let edition = self.edition();
let unresolved_name =
|| MirLowerError::unresolved_path(self.db, c, edition, &self.body.types);
Expand All @@ -1392,6 +1382,9 @@ impl<'ctx> MirLowerCtx<'ctx> {
}
}
}
_ => {
not_supported!("only `char` and numeric types are allowed in range patterns");
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions crates/hir-ty/src/mir/lower/pattern_matching.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! MIR lowering for patterns

use hir_def::{hir::LiteralOrConst, AssocItemId};
use hir_def::{hir::ExprId, AssocItemId};

use crate::{
mir::{
Expand Down Expand Up @@ -207,7 +207,7 @@ impl MirLowerCtx<'_> {
)?
}
Pat::Range { start, end } => {
let mut add_check = |l: &LiteralOrConst, binop| -> Result<()> {
let mut add_check = |l: &ExprId, binop| -> Result<()> {
let lv =
self.lower_literal_or_const_to_operand(self.infer[pattern].clone(), l)?;
let else_target = *current_else.get_or_insert_with(|| self.new_basic_block());
Expand Down
Loading
Loading