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

Add RFC to feature gate some slice patterns #164

Merged
merged 1 commit into from Sep 3, 2014

Conversation

Projects
None yet
7 participants
@brson
Copy link
Contributor

brson commented Jul 14, 2014

text/

tracking issue: rust-lang/rust#16951


# Summary

Rust's support for pattern matching on sices has grown steadily and incrementally without a lot of oversight,

This comment has been minimized.

@lilyball

lilyball Jul 14, 2014

Contributor

Typo: "sices"

# Summary

Rust's support for pattern matching on sices has grown steadily and incrementally without a lot of oversight,
and we have concern that Rust is doing too much here, that the complexity is not worth it. This RFC proposes

This comment has been minimized.

@lilyball

lilyball Jul 14, 2014

Contributor

Who is "we"? Is this the general opinion of the Mozilla employees? Is this your personal opinion?

@lilyball

This comment has been minimized.

Copy link
Contributor

lilyball commented Jul 14, 2014

There was an extensive discussion recently about this issue (no link handy). I didn't follow it through to the end, but I thought it had established that a head-position slice ([..xs, 0, 0]) was no more difficult than a tail-position slice ([0, 0, ..xs]). What's the logic for gating the head-position slice?

I also had the impression that at least one person believed that a mid-position slice could be properly checked without as much complexity as initially thought, though as I said I didn't follow the discussion through to the end so I don't know if that was actually determined.

Finally, how does our exhaustiveness check for slice patterns work today? This RFC suggests that it is known to work properly for tail-position slices, but not known to work properly for any other slice type, but what's the actual failure mode?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 15, 2014

cc @jakub-

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 15, 2014

@kballard Discussion you're referring to is #144.

I made the mistake of filing that RFC without actually putting much/any thought into how slice patterns worked in the compiler. I closed it because I determined that we can make them work and indeed have no reason to believe that either the exhaustiveness or codegen parts are incorrect after the recent changes (though not sure about the latter, haven't spent much time there yet). I do not think there are any open issues for it at the moment.

There's a long discussion in that RFC, parts of which seem to suggest that the feature is genuinely useful. I think feature gating would be fine, though (after all, we're not removing it).

first, the implementation of Rust's match machinery is notoriously complex

True. Slice patterns are only part of the problem, though. They are treated by the match infrastructure a little differently than other types of patterns in that they complicate the compiled decision tree but so do guards.

Second, slice matching (...) while also being of only moderate utility (there are many types of collections - slices just happen to be built into the language).

Yeah, I think I agree but the same argument also applies to strings. Historically, rustc has removed match support for types that were no longer blessed by the language (~str, ~[T]) so if post-DST it'd be possible to do so with slices as well then maybe it's a good idea.

Finally, the exhaustiveness check is not correct for slice patterns - because of their complexity; it's not known that it
+can be done correctly, nor whether it is worth the effort even if.

I don't think this is true any more. It's my fault I raised this alarm last time.

Again, this is just feature gating so I wouldn't mind. It's a safer bet for most features the Rust team is uncertain about to be feature gated. But there's a balance there and a risk in growing the number of gated features to the point where most trivial Rust programs require at least one. However, last time I did run numbers on this one though it turned out slice patterns were only used a couple of times in the major Rust codebases.

@lilyball

This comment has been minimized.

Copy link
Contributor

lilyball commented Jul 15, 2014

@jakub- I get the argument that "it's just feature-gating", but I don't see the benefit in adding a feature gate where there wasn't one previously if the feature currently works and there's no compelling reason to remove it. Adding a feature gate makes sense in the following situations:

  • Creating a new feature
  • The feature does not currently work properly, but it can't be outright removed at the moment. For example, record-style enums (where the variants have named fields) is gated because it doesn't work right, although offhand I'm not sure why the feature hasn't been removed (I'm guessing someone thinks it could possibly be fixed?)
  • The feature works, but it's missing what may be considered vital functionality. I'm thinking of something like f128.

I don't think any of that applies to slices. The only remaining reason is as a prelude to removing the functionality for reasons other than it not working, and I don't see there being any compelling reason to remove slice patterns. They are useful.

However, last time I did run numbers on this one though it turned out slice patterns were only used a couple of times in the major Rust codebases.

Did you ever run the numbers on what these codebases looked like back when ~[T] patterns were still allowed? My impression is it was a lot more common with ~[T] patterns and a lot of that had to get removed and replaced with uglier workarounds.

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 15, 2014

Did you ever run the numbers on what these codebases looked like back when ~[T] patterns were still allowed? My impression is it was a lot more common with ~[T] patterns and a lot of that had to get removed and replaced with uglier workarounds.

No, I did not. You have a point here.

@huonw

This comment has been minimized.

Copy link
Member

huonw commented Jul 15, 2014

There's at least one more condition in which things should be feature gated:

  • sugary non-core feature which we don't know to work well (i.e. no-one's put the effort into to verify/investigate) and with a history of having issues and being hairy code. More problems may appear, so we don't want to be stuck with it forever, but don't want to block the 1.0 release on checking it.
@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Jul 15, 2014

I'd be fine with feature gating just out of caution, if there's any reason to be cautious, but otherwise I think that, arrays being a language primitive and a pretty important thing in general, our support for working with them should be as expressive and powerful as we can make it.

Second, slice matching in particular, is difficult to implement, while also being of only moderate utility (there are many types of collections - slices just happen to be built into the language).

Arrays are built into the language (for good reason), while slices are references to arrays, existentially quantified over their length (DST). I.e. they arise from the combination of basic language features, rather than being one collection type among many singled out for special support by the language.

(This is from the perspective of current/future Rust; obviously historically this wasn't the case.)

@zwarich

This comment has been minimized.

Copy link
Contributor

zwarich commented Jul 16, 2014

There doesn't seem to be much motivation for slice patterns in their current form; they do make some things simpler, but when it was brought up before there weren't many uses of them found. The ~[T] replacement case is more compelling as a feature, but it would require a complex contract of unsafe code between pattern matching and the library. I have very little influence on the Rust language, but it seems to me like the winds are not blowing in the direction of having such contracts between the compiler and the library.

As discussed in #144, exhaustiveness checking in the case of a single unbounded .. is the same sort of propositional satisfiability problem that our existing exhaustiveness checking is, regardless of where it occurs. Adding additional fixed-size .. ranges shouldn't be a problem either for exhaustiveness checking, since they just expand to sequences like _, _, _. However, there is still a reasonable complication for match codegen, since codegen has to create new slices representing these ranges. In other cases of pattern matching, the match components already exist as part of the structure of the data type.

@lilyball

This comment has been minimized.

Copy link
Contributor

lilyball commented Jul 16, 2014

@zwarich I think we could restore the subset of ~[T] matching for borrowing elements and slices (as opposed to moving values), pretty easily. Once Vec<T> implements Deref<[T]> (which I don't think is still an official plan, but AFAIK everyone is on board with it), we could add a rule where anything that can auto-deref to produce a &[T] can then be matched with a slice pattern. Of course, the reason to do this instead of just matching on a slice directly is when you're matching on some value that contains a Vec (which doesn't give you the opportunity to slice it before-hand).

This kind of matching obviously doesn't solve everything, but it would allow me to restore some of the vector matching code in one of my projects that I lost when we could no longer match on ~[T].

A similar approach could then be use to allow matching on String, which would actually work much better because we never could move out of a ~str in the first place. So in this same project, all of the matching code I had to rewrite when we lost ~str patterns would be restored if I could match on a String as if it were a &str.

The only real weirdness to this approach that comes to mind is when using foo@ bindings on the value, e.g. foo@"string". That would have to bind to the original un-dereferenced value, but it's not obvious from the pattern that it's doing so. However, I think that's not that big of a deal. It occurs to me this could also plausibly be fixed by re-using the ref keyword. ref foo binds foo to a reference to the value, but ref PAT could instead mean that the value matched is a reference of some kind (i.e. a smart pointer, not necessarily a &T) that, when dereferenced, matches PAT. That would make it obvious that the pattern is in fact using Deref.

@zwarich

This comment has been minimized.

Copy link
Contributor

zwarich commented Jul 17, 2014

@kballard I thought that people generally wanted the ~[] case back for bind-by-move. Is that not the case? It's the bind-by-move that seems unlikely with no builtin form of owned array.

@lilyball

This comment has been minimized.

Copy link
Contributor

lilyball commented Jul 17, 2014

@zwarich Getting it back for bind-by-move is indeed something that people (including I) would love to have. My overall point is that getting it back in the limited form of slices would be better than what we have today.

Also FWIW, the ref PAT variant at the end of my post also has the benefit of not conflicting with any potential future bind-by-move functionality.

@alexcrichton alexcrichton merged commit 720e3ad into rust-lang:master Sep 3, 2014

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 3, 2014

This was discussed in today's weekly meeting and it was decided to merge this as-is.

@chriskrycho chriskrycho referenced this pull request Feb 10, 2017

Closed

Document all features in the reference #38643

0 of 17 tasks complete

@chriskrycho chriskrycho referenced this pull request Mar 11, 2017

Closed

Document all features #9

18 of 48 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.