Skip to content

Distinguish between constants and bindings in patterns #1618

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

Closed
matklad opened this issue Jul 29, 2019 · 4 comments · Fixed by #3262
Closed

Distinguish between constants and bindings in patterns #1618

matklad opened this issue Jul 29, 2019 · 4 comments · Fixed by #3262
Labels
E-hard E-has-instructions Issue has some instructions and pointers to code to get started

Comments

@matklad
Copy link
Member

matklad commented Jul 29, 2019

In rust, a pattern like x in match foo() { x => () } might mean one of two things:

  • an cont x: i32 = 92, in which case we match against a literal constant.
  • a fresh binding, in which case we should bind the name x

The rule for distinguishing the two cases is: "if there's a constant in scope, the pattern is constant, otherwise it is a binding". We currently treat all such cases as bindings.

I think this should be fixed here:

https://github.com/rust-analyzer/rust-analyzer/blob/ad63fbe61a2e024bab8baadc22863a0795d20a95/crates/ra_hir/src/expr/scope.rs#L90-L100

However, we can't do that with the current code, because we don't have access to Resolver there, and so can't check if a name binds to a constant already. The problem with using Resolver here is that resolver already knows about expression scopes.... So looks like we should use ModuleItemMap directly instead.

We also should record the fact that binding is a constant somewhere. A table in InferenceResult seems like a good place for this:

https://github.com/rust-analyzer/rust-analyzer/blob/ad63fbe61a2e024bab8baadc22863a0795d20a95/crates/ra_hir/src/ty/infer.rs#L110-L123

This also makes me think that maybe we should redo our Expr infra and resolve expressions during lowering, and not duting type inference....

@matklad matklad added E-hard E-has-instructions Issue has some instructions and pointers to code to get started labels Jul 29, 2019
@flodiebold
Copy link
Member

This also makes me think that maybe we should redo our Expr infra and resolve expressions during lowering, and not during type inference....

Agreed, we're probably unnecessarily lazy in this case... (though some paths of course can only be resolved during inference, so we need to handle partially resolved paths similarly to how rustc does it (that doesn't affect this issue though, one-element paths should always be resolvable during lowering))

@matklad
Copy link
Member Author

matklad commented Jul 29, 2019

One thing that is easier with the current setup is that we can get scopes for completion easily.

I think the query setup we want is

lower_with_scopes(def) -> (Body, Scopes)
lower(def) -> Body

such that we don't remmeber scopes unnceseccary, but also don't repeat special logic for completion

@matklad
Copy link
Member Author

matklad commented Feb 17, 2020

Hm, I've looked recently into this and an (simpler?) idea I had is to add a targeted hack somewhere around here:

https://github.com/rust-analyzer/rust-analyzer/blob/1b73abd1c3c9185f4a1f62c5e657e07daf3d4774/crates/ra_hir_ty/src/infer/pat.rs#L114

Check if the pattern "resolves" and, if it does, just substitute it with a Pat::Path, and then procede as usual.

@flodiebold
Copy link
Member

I think it would still mean that that name would resolve to the pattern inside the match arm 😕
E.g. in
match ... { None => None, ... } the second None would wrongly have the type of the match expression.

bors bot added a commit that referenced this issue Feb 22, 2020
3262: Fix handling of const patterns r=matklad a=flodiebold

E.g. in `match x { None => ... }`, `None` is a path pattern (resolving to the
option variant), not a binding. To determine this, we need to try to resolve the
name during lowering. This isn't too hard since we already need to resolve names
for macro expansion anyway (though maybe a bit hacky).

Fixes #1618.

Co-authored-by: Florian Diebold <[email protected]>
@bors bors bot closed this as completed in f1f45f9 Feb 22, 2020
SomeoneToIgnore pushed a commit to SomeoneToIgnore/rust-analyzer that referenced this issue Feb 22, 2020
3262: Fix handling of const patterns r=matklad a=flodiebold

E.g. in `match x { None => ... }`, `None` is a path pattern (resolving to the
option variant), not a binding. To determine this, we need to try to resolve the
name during lowering. This isn't too hard since we already need to resolve names
for macro expansion anyway (though maybe a bit hacky).

Fixes rust-lang#1618.

Co-authored-by: Florian Diebold <[email protected]>
bors bot added a commit that referenced this issue Feb 23, 2020
3278: Show more inlay hints r=matklad a=SomeoneToIgnore

Closes #3273

As suggested in #1606 (comment) , there is a simpler way to handle inlay hints after #1618 is closed.

This PR uses the approach suggested (which results in more type hints for various bindings) and also adds more name hints for function parameters.

Examples can be found in the issue:
* #3273 (comment)
* #3273 (comment)

Co-authored-by: Kirill Bulatov <[email protected]>
cjhopman pushed a commit to cjhopman/rust-analyzer that referenced this issue Apr 10, 2020
E.g. in `match x { None => ... }`, `None` is a path pattern (resolving to the
option variant), not a binding. To determine this, we need to try to resolve the
name during lowering. This isn't too hard since we already need to resolve names
for macro expansion anyway (though maybe a bit hacky).

Fixes rust-lang#1618.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-hard E-has-instructions Issue has some instructions and pointers to code to get started
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants