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 patterns in hir #12158

Closed
wants to merge 1 commit into from
Closed

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented May 5, 2022

No description provided.

@@ -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.

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.

Copy link
Member

@flodiebold flodiebold May 5, 2022

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
}

@Veykril
Copy link
Member Author

Veykril commented May 5, 2022

Ye I am not sure how we can fix this up nicely

@Veykril Veykril closed this May 5, 2022
@Veykril Veykril deleted the range-pat branch July 2, 2022 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants