Remove PatWildMulti #29486

petrochenkov · 2015-10-31T00:50:40Z

Motivation:

It is not actually a pattern
It is not actually needed, except for...

Drawback:

Slice patterns like [a, _.., b] are pretty-printed as [a, .., b]. Great loss :(

plugin-[breaking-change], as always

rust-highfive · 2015-10-31T00:50:44Z

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

sanxiyn · 2015-11-01T09:24:55Z

Doesn't this also break printing of Variant(..) vs Variant(_)?

Manishearth · 2015-11-01T10:21:34Z

So does the _ pattern work as a .. pattern now? I feel such a change might need an RfC.

I'm not sure if I'm too happy about the change either, from a language point of view. I feel that _ and .. are very different in intention: The first means that "I know there is a field here, but I don't care about it", the second means "I only care about the specified fields". Which sounds the same, but isn't -- it's got the same nuanced distinction between an exhaustive match with noop arms and a match with a wildcard.

When you use _, you're saying you don't care about that specific field. When you use .., you're saying you don't care about that specific field and any other fields which may be added in the future. Just like how the exhaustive match with noop arms means that "I don't care about these variants", and a match with a wildcard means "I don't care about these variants and any variants which may be added in the future".

Manishearth · 2015-11-01T10:32:49Z

(also, @sanxiyn is correct)

Manishearth · 2015-11-01T11:04:41Z

I just had a look at the API again, I was wrong, PatEnum and PatStruct use a different mechanism to handle .., so this change looks good to me.

Also opened #29501 since the docs had a mistake.

Manishearth · 2015-11-01T11:12:15Z

src/librustc/middle/stability.rs

@@ -543,11 +543,6 @@ pub fn check_pat(tcx: &ty::ctxt, pat: &hir::Pat,
        // Foo(a, b, c)
        hir::PatEnum(_, Some(ref pat_fields)) => {
            for (field, struct_field) in pat_fields.iter().zip(&v.fields) {
-                // a .. pattern is fine, but anything positional is


Could we preserve this comment somehow? Mention on this that when the second field is None, it's a .. pattern, which is okay.

@Manishearth
It's an erroneous comment, PatWildMulti couldn't be here and Variant(..) pattern is hir::PatEnum(_, None)

It's an erroneous comment, but it addresses a real issue, that of Variant(..) patterns.

I'm saying we move this comment up and have it talk of why PatEnum(_, None) is ignored.

I misread it, sorry, will add a comment now.

Updated with a (pretty obvious) comment.

Manishearth · 2015-11-01T11:16:50Z

r=me with that nit.

cc @nrc

petrochenkov · 2015-11-01T13:17:40Z

And yeah, it may look confusing, but it's a purely technical internal change, it doesn't break anything or change semantics.

Manishearth · 2015-11-01T13:36:47Z

@bors-servo r+ rollup

bors · 2015-11-01T13:36:48Z

📌 Commit 306b0ef has been approved by Manishearth

Motivation: - It is not actually a pattern - It is not actually needed, except for... Drawback: - Slice patterns like `[a, _.., b]` are pretty-printed as `[a, .., b]`. Great loss :( plugin-[breaking-change], as always

bors · 2015-11-01T13:36:50Z

⌛ Testing commit 306b0ef with merge 6d43fef...

bors · 2015-11-01T16:19:03Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

nrc · 2015-11-02T19:09:37Z

I would have liked to accept only the change to the HIR and not change the AST. There is no requirement that the HIR can be round-tripped via pretty printing or even that pretty printed HIR is compilable, so simplification there is welcome. However, it is a goal of the AST to be as close as possible to the source text - if there is different source text (which is the case here) then this should be reflected in the AST. I expect the AST to move more in this direction in the future (one could imagine a syntax extension which adds semantic differences here where there are none in Rust).

I'd prefer to see the AST change reversed (unless I'm misunderstanding), but it is pretty minor, so I don't think it is too important for now (I expect that we will revamp the AST in the future anyway).

Manishearth · 2015-11-02T19:11:32Z

@nrc Instead of reversing the AST change, perhaps we should add a bool flag to PatVec, to be in line with the other places .. appears?

nrc · 2015-11-02T19:16:46Z

In general I prefer enums to bools since the names are more informative/documenting, but consistency is important, so that would work for me.

petrochenkov · 2015-11-02T19:32:06Z

To what extent is AST supposed to mirror the source text? Currently, for example, presence of trailing separators in various lists isn't tracked in AST but at the same time affects pretty-printing.
[a, .., c] is a purely syntactic shortcut for (almost never used) [a, _.., c] and the distinction between them is not something that can be useful or should be tracked IMO, unless we target for 100% correspondence between the source and AST.

nrc · 2015-11-02T20:15:09Z

@petrochenkov "supposed" is hard to answer because the role of the AST is in flux. In the past it was used by the frontend, tools, syntax extensions, and the compiler so there was a trade-off between being easy to parse, easy to operate on syntactically, and concise so the compiler can operate on it most easily. We have no separated out the AST and HIR so the HIR can be optimised for the compiler and maximally concise (this PR being an example of exactly the kind of change I want to see in the HIR). While the AST is focussed more on being easily usable in tools and syntax extensions - that means (I think) being closer to the source text than it is currently (for example, I want to be able to find comments in the AST). Pretty printing is one example of a tool that needs close correspondence between the AST and source. So, I would like to aim for '100%' correspondence between the source and AST even though that is not the current state, nor has it been a goal in the past.

Note on '100%' and trailing separators. I haven't thought in detail about how the AST should represent these kind of details - I don't think comments should be in the AST, and nor should trailing separators (probably). I am considering some kind of model where we keep the token stream around and support easy indexing of the token stream from the AST (we would also move all span info to the token stream and out of the AST). Then that kind of syntactic trivia is not in the AST, but easily accessible from it. As I haven't figured out the detail of the design, I'm not sure whether PartWildMulti is something best expressed by this method or directly in the AST

Manishearth · 2015-11-02T20:22:32Z

I am considering some kind of model where we keep the token stream around and support easy indexing of the token stream from the AST

This reminds me of some of the zero copy and tendril work that was being done for html5ever.

Remove PatWildMulti

3468b8d

rust-highfive assigned arielb1 Oct 31, 2015

Manishearth reviewed Nov 1, 2015
View reviewed changes

Add comment for stability.rs

306b0ef

bors merged commit 306b0ef into rust-lang:master Nov 1, 2015

bors mentioned this pull request Nov 1, 2015

Preliminary support for MIR in trans #29217

Merged

petrochenkov deleted the multiwild branch November 22, 2015 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove PatWildMulti #29486

Remove PatWildMulti #29486

petrochenkov commented Oct 31, 2015

rust-highfive commented Oct 31, 2015

sanxiyn commented Nov 1, 2015

Manishearth commented Nov 1, 2015

Manishearth commented Nov 1, 2015

Manishearth commented Nov 1, 2015

Manishearth Nov 1, 2015

petrochenkov Nov 1, 2015

Manishearth Nov 1, 2015

petrochenkov Nov 1, 2015

petrochenkov Nov 1, 2015

Manishearth commented Nov 1, 2015

petrochenkov commented Nov 1, 2015

Manishearth commented Nov 1, 2015

bors commented Nov 1, 2015

bors commented Nov 1, 2015

bors commented Nov 1, 2015

nrc commented Nov 2, 2015

Manishearth commented Nov 2, 2015

nrc commented Nov 2, 2015

petrochenkov commented Nov 2, 2015

nrc commented Nov 2, 2015

Manishearth commented Nov 2, 2015

Remove PatWildMulti #29486

Remove PatWildMulti #29486

Conversation

petrochenkov commented Oct 31, 2015

rust-highfive commented Oct 31, 2015

sanxiyn commented Nov 1, 2015

Manishearth commented Nov 1, 2015

Manishearth commented Nov 1, 2015

Manishearth commented Nov 1, 2015

Manishearth Nov 1, 2015

Choose a reason for hiding this comment

petrochenkov Nov 1, 2015

Choose a reason for hiding this comment

Manishearth Nov 1, 2015

Choose a reason for hiding this comment

petrochenkov Nov 1, 2015

Choose a reason for hiding this comment

petrochenkov Nov 1, 2015

Choose a reason for hiding this comment

Manishearth commented Nov 1, 2015

petrochenkov commented Nov 1, 2015

Manishearth commented Nov 1, 2015

bors commented Nov 1, 2015

bors commented Nov 1, 2015

bors commented Nov 1, 2015

nrc commented Nov 2, 2015

Manishearth commented Nov 2, 2015

nrc commented Nov 2, 2015

petrochenkov commented Nov 2, 2015

nrc commented Nov 2, 2015

Manishearth commented Nov 2, 2015