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

Lower range patterns from AST to HIR #12210

Open
Veykril opened this issue May 10, 2022 · 6 comments · May be fixed by #18995
Open

Lower range patterns from AST to HIR #12210

Veykril opened this issue May 10, 2022 · 6 comments · May be fixed by #18995
Assignees
@alibektas
Labels
A-hir hir and hir-def related A-pattern pattern handling related things C-feature Category: feature request

Comments

@Veykril
Copy link
Member

Veykril commented May 10, 2022

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
}

Originally posted by @matklad in #12158 (comment)
@Veykril Veykril added the A-hir hir and hir-def related label May 20, 2022
@Veykril Veykril mentioned this issue May 25, 2023
Merged
@HKalbasi
Copy link
Member

How go to definition and semantic highlighting is working in those pattern currently? In this code:

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,
    }
}

Everything works for R, but L is detected as an ident pattern.

@Veykril
Copy link
Member Author

Veykril commented May 25, 2023 •
edited
Loading

rust-analyzer/crates/hir/src/semantics.rs

Lines 450 to 452 in c06f088

pub fn resolve_bind_pat_to_const(&self, pat: &ast::IdentPat) -> Option<ModuleDef> {
self.imp.resolve_bind_pat_to_const(pat)
}

rust-analyzer/crates/hir/src/source_analyzer.rs

Lines 493 to 509 in c06f088

pub(crate) fn resolve_bind_pat_to_const(
&self,
db: &dyn HirDatabase,
pat: &ast::IdentPat,
) -> Option<ModuleDef> {
let pat_id = self.pat_id(&pat.clone().into())?;
let body = self.body()?;
let path = match &body[pat_id] {
Pat::Path(path) => path,
_ => return None,
};
let res = resolve_hir_path(db, &self.resolver, path)?;
match res {
PathResolution::Def(def) => Some(def),
_ => None,
}
}

As that function fails there, R resolves because we fall back to heuristic path resolution due to it being part of a multi segment path I think, while L is single segment so we assume its an ident pattern.

@HKalbasi
Copy link
Member

L is parsed as IdentPat

RANGE_PAT@617..625
  IDENT_PAT@617..618
    NAME@617..618
      IDENT@617..618 "L"
  DOT2EQ@618..621 "..="
  [email protected]
    [email protected]
      [email protected]
        [email protected]
          [email protected]
            [email protected] "x"
      COLON2@622..624 "::"
      PATH_SEGMENT@624..625
        NAME_REF@624..625
          IDENT@624..625 "R"

Isn't this wrong? The reference says that only path is allowed here. I think it would work correctly if it was parsed as PathPat, but it might become irrelevant once we lower them to hir.

@Veykril
Copy link
Member Author

Veykril commented May 26, 2023

Our grammar differs from the reference in a few parts and that's fine (and in parts deliberate), so I think we want to keep the parse tree as is.

@Veykril Veykril added the C-feature Category: feature request label May 26, 2023
@matklad
Copy link
Member

matklad commented May 26, 2023

yup, on the level of concrete syntax, .. is a binary operator on patterns:

let S {} .. _ = ();

[email protected]
  [email protected] "let"
  [email protected] " "
  [email protected]
    [email protected]
      [email protected]
        [email protected]
          [email protected]
            [email protected] "S"
      [email protected] " "
      [email protected]
        [email protected] "{"
        [email protected] "}"
    [email protected] " "
    [email protected] ".."
    [email protected] " "
    [email protected]
      [email protected] "_"
  [email protected] " "
  [email protected] "="
  [email protected] " "
  [email protected]
    [email protected] "("
    [email protected] ")"
  [email protected] ";"

The general philosophy is to parse what's natural on purely syntactic basis, and then place additional restrictions when checking ast/lowering to hir.

@Nadrieril Nadrieril added the A-pattern pattern handling related things label Jan 24, 2024
@Veykril
Copy link
Member Author

Veykril commented Oct 31, 2024 •
edited
Loading

Current state of this is the given fixme here

rust-analyzer/crates/hir-def/src/body/lower.rs

Line 1688 in 70348fa

// FIXME: implement in a way that also builds source map and calculates assoc resolutions in type inference.

Also of note is that 2d4d6b6 blurred the line between expressions and patterns for lowering now, where destructing expressions are lowered to patterns. This is demanding the other part, lowering patterns to expressions.

@alibektas alibektas linked a pull request Jan 21, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alibektas alibektas

Labels
A-hir hir and hir-def related A-pattern pattern handling related things C-feature Category: feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Lower range pattern bounds to expressions alibektas/rust-analyzer
5 participants
@matklad @Veykril @Nadrieril @alibektas @HKalbasi