Implement slice pattern AST > HIR lowering #3062

jplatte · 2020-02-09T00:16:49Z

WIP. The necessary changes for parsing are implemented, but actual inference is not yet. Just wanted to upload what I've got so far so it doesn't get duplicated :)

Will fix #3043

jplatte · 2020-02-09T00:18:09Z

Also maybe somebody knows a better way to find the rest pattern in a slice pattern than the nested match I wrote?

matklad · 2020-02-09T09:17:15Z

crates/ra_hir_def/src/body/lower.rs

+            ast::Pat::SlicePat(p) => {
+                let mut args = p.args().peekable();
+                let prefix = args
+                    .peeking_take_while(|p| match p {


This bit mostly does syntactic analysis, so I think we should move it to ra_syntax, extensions.rs specifically.

struct SlicePatComponents { prefix: Vec<Pat>, // which name rustc uses for this? rest doesn't seem like a perfect one rest: Option<Pat>, suffix: Vec<Pat>, } impl ast::SlicePat { fn components(&self) -> SlicePatCompoents {... } }

Not sure exactly about the precise shape of the type we want here

Re. naming, from rust-lang/rust#67712:

Common to both flavors is they use the token .., referred as a "rest pattern" in a pattern context.

rustc uses the naming convention:

Slice { before / prefix: List<Pat>, slice: Option<Pat>, after / suffix: List<Pat>, },

where before and after are used in HIR and prefix and suffix are used in HAIR. I would recommend going with prefix and suffix (as I want to change HIR to that some day).

Common to both flavors is they use the token .., referred as a "rest pattern" in a pattern context.

This refers to the syntactic pattern production Pat = ... | Rest:".." ; and is not specific to slice patterns. The semantic {binding @}? @ .. is called a sub-slice. (syn also uses Rest.)

flodiebold · 2020-02-09T12:38:05Z

crates/ra_hir_def/src/expr.rs

@@ -381,6 +381,7 @@ pub struct RecordFieldPat {
 pub enum Pat {
    Missing,
    Wild,
+    DotDot,


Do we actually need a DotDot pattern, or is it just the same as Wild? 🤔

Wild is _ and matches a single item, .. is DotDot and matches 0, 1, or many items.

We probably could use a better name though...

They need to be distinguishable in both tuple (struct) patterns and slice patterns from my understanding, since e.g. &[_] and &[..] have entirely different meaning.

I could rename it to Rest, does that sound good? I think for the .. pattern in tuples / tuple structs, this also makes sense.

&[_] and &[..] have entirely different meaning.

But isn't that difference in meaning expressed in the separation between prefix, rest and suffix pattern? I.e. in my understanding, [_] is "prefix [_], rest none, suffix []", whereas [..] is "prefix [], rest _, suffix []".

That is to say, the .. pattern obviously exists in the AST, but I'm not convinced it needs to exist in the desugared HIR.

It does for tuple patterns, no? For the type (T, T, T) the pattern (x, ..) is valid but (x, _) isn't.

.. needs to be handled specially for tuple patterns. Just adding this as a pattern wouldn't help; tuple pattern inference needs to check whether there's a .. and act accordingly. So it makes more sense to restructure tuple patterns similarly to Pat::Slice. The same goes for .. in record patterns; there it wouldn't even fit into the existing structure because there's no name. In other words, .. isn't a pattern, it's a marker with special meaning in certain kinds of patterns. It can't stand alone.

Note that there's no such variant in rustc either:
https://github.com/rust-lang/rust/blob/7494250106003d698579edef215d0c01810b5156/src/librustc_hir/hir.rs#L867-L920

See https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ast_lowering/struct.LoweringContext.html#method.lower_pat_slice for lowering of slice patterns and https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ast_lowering/struct.LoweringContext.html#method.lower_pat_tuple for tuples.

matklad · 2020-02-09T12:58:55Z

sgtm

…

On Sun, 9 Feb 2020 at 13:57, Jonas Platte ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/ra_hir_def/src/expr.rs <#3062 (comment)> : > @@ -381,6 +381,7 @@ pub struct RecordFieldPat { pub enum Pat { Missing, Wild, + DotDot, I could rename it to Rest, does that sound good? I think for the .. pattern in tuple (structs), this also makes sense. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3062>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3MYIZHRF3KJ2HRFDLBTRB74UDANCNFSM4KR57VLQ> .

jplatte · 2020-02-10T21:40:59Z

I don't think I'm going to be working on this anymore, sorry. I don't understand whether I first need to refactor tuples now, and I'm also having a hard time understanding most of the code I've looked at in ra_syntax.

I hope the comments in this PR are helpful for whoever decides to work on this next.

flodiebold · 2020-02-10T21:55:18Z

Sorry, just to be clear, there's no need to refactor tuples, they're a completely separate matter. I think the only thing that needs to be done here is to remove Pat::DotDot and instead map ast::Pat::DotDotPat to Pat::Wild.

jplatte · 2020-02-10T22:34:57Z

Could adding that mapping not regress tuples as it's then interpreted the same as _?

Anyway, there's also the issue that I'm having trouble with the ast code. Your description of what has to be done over at #3043 seemed simple enough so I took a stab at it, but it seems to be more involved than I expected, and for now I want to use my time for other things. I might come back to this some time if nobody else implements it.

jplatte · 2020-02-10T22:37:34Z

So, thanks for your feedback, it was definitely useful. I just tried working on too many things at once.

flodiebold · 2020-02-10T22:42:18Z

Could adding that mapping not regress tuples as it's then interpreted the same as _?

.. in tuples is not currently working either, as far as I know, so it wouldn't change anything.

I'm having trouble with the ast code

That's confusing to me since as far as I can see you're done with the part that touches the AST code. But anyway, it's fine if you've got other things to do, of course :)

jplatte · 2020-02-10T23:09:47Z

That's confusing to me since as far as I can see you're done with the part that touches the AST code.

Okay I think I was just confused. Maybe I can finish this afterall.. 🤦‍♂️

jplatte · 2020-02-10T23:12:14Z

Adressed your feedback, will implement the change @matklad suggested next ~~and then move to inference.~~

jplatte · 2020-02-11T18:08:41Z

I'm done with AST > HIR lowering now, have addressed all the feedback from above. I'll stop here because I don't grok the inference code at all :/

flodiebold · 2020-02-11T18:41:22Z

crates/ra_syntax/src/ast.rs

@@ -217,7 +218,7 @@ fn test_doc_comment_multi_line_block_strips_suffix() {
 fn test_comments_preserve_trailing_whitespace() {
    let file = SourceFile::parse(
        r#"
-/// Representation of a Realm.   
+/// Representation of a Realm.


the trailing whitespace here was intentional and is being tested for 😅

My editor deleted it 🤦‍♂️

flodiebold · 2020-02-11T18:46:14Z

bors d+

LGTM, feel free to r+ when the build is fixed.

bors · 2020-02-11T18:46:15Z

✌️ jplatte can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

jplatte · 2020-02-11T21:46:20Z

bors r+

3062: Implement slice pattern AST > HIR lowering r=jplatte a=jplatte WIP. The necessary changes for parsing are implemented, but actual inference is not yet. Just wanted to upload what I've got so far so it doesn't get duplicated :) Will fix #3043 Co-authored-by: Jonas Platte <[email protected]>

bors · 2020-02-11T21:56:22Z

Build succeeded

Rust (macos-latest)
Rust (ubuntu-latest)
Rust (windows-latest)
TypeScript

matklad reviewed Feb 9, 2020

View reviewed changes

flodiebold reviewed Feb 9, 2020

View reviewed changes

jplatte closed this Feb 10, 2020

jplatte reopened this Feb 10, 2020

jplatte changed the title ~~Implement slice pattern inference~~ Implement slice pattern AST > HIR lowering Feb 11, 2020

jplatte marked this pull request as ready for review February 11, 2020 18:04

flodiebold reviewed Feb 11, 2020

View reviewed changes

Implement slice pattern AST > HIR lowering

a3b104a

bors bot merged commit a3b104a into rust-lang:master Feb 11, 2020

jplatte deleted the slice_patterns branch February 11, 2020 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement slice pattern AST > HIR lowering #3062

Implement slice pattern AST > HIR lowering #3062

jplatte commented Feb 9, 2020

jplatte commented Feb 9, 2020

matklad Feb 9, 2020

jplatte Feb 9, 2020

Centril Feb 11, 2020

flodiebold Feb 9, 2020

matklad Feb 9, 2020

jplatte Feb 9, 2020

jplatte Feb 9, 2020 •

edited

Loading

flodiebold Feb 9, 2020 •

edited

Loading

flodiebold Feb 9, 2020

jplatte Feb 9, 2020

flodiebold Feb 9, 2020

flodiebold Feb 9, 2020

Centril Feb 11, 2020

matklad commented Feb 9, 2020 via email

jplatte commented Feb 10, 2020

flodiebold commented Feb 10, 2020

jplatte commented Feb 10, 2020

jplatte commented Feb 10, 2020

flodiebold commented Feb 10, 2020

jplatte commented Feb 10, 2020

jplatte commented Feb 10, 2020 •

edited

Loading

jplatte commented Feb 11, 2020

flodiebold Feb 11, 2020

jplatte Feb 11, 2020

flodiebold commented Feb 11, 2020

bors bot commented Feb 11, 2020

jplatte commented Feb 11, 2020

bors bot commented Feb 11, 2020

Implement slice pattern AST > HIR lowering #3062

Implement slice pattern AST > HIR lowering #3062

Conversation

jplatte commented Feb 9, 2020

jplatte commented Feb 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

flodiebold Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Feb 9, 2020 via email

jplatte commented Feb 10, 2020

flodiebold commented Feb 10, 2020

jplatte commented Feb 10, 2020

jplatte commented Feb 10, 2020

flodiebold commented Feb 10, 2020

jplatte commented Feb 10, 2020

jplatte commented Feb 10, 2020 • edited Loading

jplatte commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flodiebold commented Feb 11, 2020

bors bot commented Feb 11, 2020

jplatte commented Feb 11, 2020

bors bot commented Feb 11, 2020

Build succeeded

jplatte Feb 9, 2020 •

edited

Loading

flodiebold Feb 9, 2020 •

edited

Loading

jplatte commented Feb 10, 2020 •

edited

Loading