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.

Sign up for GitHub

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

Jump to bottom

fix: lower range patterns in hir #12158

Closed
wants to merge 1 commit into from
Closed

fix: lower range patterns in hir #12158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions crates/hir-def/src/body/lower.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,8 +905,11 @@ impl ExprCollector<'_> {
}
None => Pat::Missing,
},
// FIXME: implement
ast::Pat::RangePat(_) => Pat::Missing,
ast::Pat::RangePat(pat) => {
let start = pat.start().map(|pat| self.collect_pat(pat));
let end = pat.end().map(|pat| self.collect_pat(pat));
Pat::Range { start, end }
}
};
let ptr = AstPtr::new(&pat);
self.alloc_pat(pattern, Either::Left(ptr))
Expand Down
2 changes: 1 addition & 1 deletion crates/hir-def/src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ pub enum Pat {
Tuple { args: Box<[PatId]>, ellipsis: Option<usize> },
Or(Box<[PatId]>),
Record { path: Option<Box<Path>>, args: Box<[RecordFieldPat]>, ellipsis: bool },
Range { start: ExprId, end: ExprId },
Copy link
Member Author

Choose a reason for hiding this comment

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

Was there a reason for this being ExprId? I couldn't think of a reason as to why this was the case, since it should be PatId imo

Copy link
Member

Choose a reason for hiding this comment

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

The values in a range pattern aren't patterns themselves, are they? They're the endpoints of the range you're checking for. You e.g. can't capture them with bind patterns (that wouldn't even make any sense).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well ye, not all are allowed there but the things accepted there are specifically a subset of patterns are they not (literals and path patterns)? https://doc.rust-lang.org/reference/patterns.html#range-patterns

Copy link
Member

Choose a reason for hiding this comment

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

Hm. If rustc does it this way, I guess we can as well, but it doesn't make sense to me. The range bounds aren't matched against; they're expressions whose value is used to construct the range. But maybe I have the wrong mental model here.

Copy link
Member

Choose a reason for hiding this comment

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

Rustc's HIR uses Expr though:
https://github.com/rust-lang/rust/blob/12d3f107c1634ed41a800e220ccf99b665d906d8/compiler/rustc_hir/src/hir.rs#L1077

Copy link
Member

Choose a reason for hiding this comment

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

IMO adjusting the AST would be the right thing to do. The mention of PathPattern in the reference seems like an error to me; Rustc directly parses a path or qualified path in this place, and produces an Expr in the AST.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ye, the main problem for us is how to do that. We first parse a pattern, then check if a range op follows. At this point we have to change the already parsed syntax. That is we need to change IdentPat into a path at that point for example which I don't think we can easily do.

Copy link
Member

Choose a reason for hiding this comment

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

rust-lang/reference#1204

Copy link
Member

@flodiebold flodiebold May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm maybe @matklad has a good idea how to do this in the parser? But I guess it might need some refactoring of how we parse patterns, yeah.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, to me it seems correct to use pat..pat in the syntax, but expr..expr in the hir.

Syntactically, they are patterns, as .. is an infix binary operator. We could add some LR-style re-structuring of the tree, where we don't create a pat until we haven't seen .., but that seems complicated. Feels more natural to parse a pat, than notice an .., and parse another pat.

But yeah, in semantics we want to keep those as expressions, so during lowering we need to figure out that, what was a pattern syntactically, actually is an expression semantically.

This feel dual to recently stabilized destructive assignment, where we parse stuff like (a, b) = (b, a) as expr = expr, but, during lowering, lower the LHS as a pattern.

Though, seeing the two example side-by-side, maybe we should bite the bullet and just don't distinguish patterns and expressions at the level of syntax? That is, we'd use ast::Expr for both patterns and expressions. We'd then have an API like

impl ast::Expr {
    fn classify(&self) -> IsPattern | IsExpression | Ambiguous
}

impl Semnatics {
    fn classify_expr(&self, expr: &ast::Expr) -> IsPattern | IsExpression
}

Range { start: Option<PatId>, end: Option<PatId> },
Slice { prefix: Box<[PatId]>, slice: Option<PatId>, suffix: Box<[PatId]> },
Path(Box<Path>),
Lit(ExprId),
Expand Down
7 changes: 5 additions & 2 deletions crates/hir-ty/src/infer/pat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,11 @@ impl<'a> InferenceContext<'a> {
}
Pat::Wild => expected.clone(),
Pat::Range { start, end } => {
let start_ty = self.infer_expr(*start, &Expectation::has_type(expected.clone()));
self.infer_expr(*end, &Expectation::has_type(start_ty))
let start_ty = start.map(|pat| self.infer_pat(pat, &expected, default_bm));
let end_ty = end.as_ref().map(|&pat| {
self.infer_pat(pat, start_ty.as_ref().unwrap_or(&expected), default_bm)
});
end_ty.unwrap_or_else(|| expected.clone())
}
Pat::Lit(expr) => self.infer_expr(*expr, &Expectation::has_type(expected.clone())),
Pat::Box { inner } => match self.resolve_boxed_box() {
Expand Down
8 changes: 8 additions & 0 deletions crates/hir-ty/src/tests/patterns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,12 +172,20 @@ fn infer_range_pattern() {
17..75 '{ ...2 {} }': ()
23..45 'if let...u32 {}': ()
26..42 'let 1....= 2u32': bool
30..31 '1': u32
30..31 '1': u32
30..35 '1..76': u32
33..35 '76': u32
33..35 '76': u32
38..42 '2u32': u32
43..45 '{}': ()
50..73 'if let...u32 {}': ()
53..70 'let 1....= 2u32': bool
57..58 '1': u32
57..58 '1': u32
57..63 '1..=76': u32
61..63 '76': u32
61..63 '76': u32
66..70 '2u32': u32
71..73 '{}': ()
"#]],
Expand Down