-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
default binding modes: add pat_binding_modes #43399
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
src/librustc/hir/lowering.rs
Outdated
@@ -2191,7 +2191,7 @@ impl<'a> LoweringContext<'a> { | |||
let next_ident = self.str_to_ident("__next"); | |||
let next_pat = self.pat_ident_binding_mode(e.span, | |||
next_ident, | |||
hir::BindByValue(hir::MutMutable)); | |||
hir::BindByValue(hir::MutMutable)); // FIXME(tschottdorf): vetted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is fine, it occurs before type-check even starts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks great! I left suggestions at all the problematic places (I think!).
src/librustc/hir/pat_util.rs
Outdated
@@ -128,9 +128,11 @@ impl hir::Pat { | |||
contains_bindings | |||
} | |||
|
|||
// FIXME(tschottdorf): this method will have to go away. The callers are usually | |||
// trying to print nice errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think this method can probably stay; it's probably a bit too specific as it is, actually, for most if not all callers, but in any case it's more about the syntax of what the user wrote than the semantics. In other words, we want to detect stuff like let x = ...
and make a nice error message that talks about x
; this generally would work equally well for let ref x = ...
anyhow, and it definitely works for the x
in let Some(x) = ...
, even if it winds up being a ref-binding. I guess really we probably ought to audit the callers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked through the callers.
if let Some(simple_name) = pattern.simple_name() {
labels.push((pattern.span, format!("consider giving `{}` a type", simple_name)));
} else {
labels.push((pattern.span, format!("consider giving the pattern a type")));
}
let (error_var, span_label_var) = if let Some(simple_name) = arg.pat.simple_name() {
(format!("the type of `{}`", simple_name), format!("the type of `{}`", simple_name))
} else {
("parameter type".to_owned(), "type".to_owned())
};
// Check that argument is Sized.
// The check for a non-trivial pattern is a hack to avoid duplicate warnings
// for simple cases like `fn foo(x: Trait)`,
// where we would error once on the parameter as a whole, and once on the binding `x`.
if arg.pat.simple_name().is_none() {
fcx.require_type_is_sized(arg_ty, decl.output.span(), traits::MiscObligation);
}
Doesn't seem that I need to change anything here after all.
src/librustc/hir/pat_util.rs
Outdated
@@ -167,7 +169,7 @@ impl hir::Pat { | |||
pub fn contains_ref_binding(&self) -> Option<hir::Mutability> { | |||
let mut result = None; | |||
self.each_binding(|mode, _, _, _| { | |||
if let hir::BindingMode::BindByRef(m) = mode { | |||
if let hir::BindingMode::BindByRef(m) = mode { // FIXME(tschottdorf): vetted, but probably problematic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this worries me. A quick scan through the uses shows some that may be concerning. For example, this bit of logic in _match.rs
looks worrisome, and from what I can tell both uses of this function are doing the same sort of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this can be fixed before we have (the logic to compute) pat_adjustments
in place. We have to know whether we're introducing implicit refs whenever contains_ref_binding
is called. For now, I renamed to contains_explicit_ref_binding
and added FIXMEs
.
src/librustc/hir/print.rs
Outdated
@@ -1651,12 +1651,12 @@ impl<'a> State<'a> { | |||
PatKind::Wild => self.s.word("_")?, | |||
PatKind::Binding(binding_mode, _, ref path1, ref sub) => { | |||
match binding_mode { | |||
hir::BindByRef(mutbl) => { | |||
hir::BindByRef(mutbl) => { // FIXME(tschottdorf): vetted and problematic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these uses are OK. The goal of this code is to print back out what the user explicit wrote, not the capture "semantic meaning", so we should just dump what the user wrote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
PatKind::Binding(bind_mode, ..) => { | ||
if bind_mode == hir::BindByValue(hir::MutMutable) { | ||
PatKind::Binding(bm, ..) => { | ||
// FIXME(tschottdorf): no tables in scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just add an argument to the fn; it's local to this file, and the callers can use self.tables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was easy, done.
src/librustc/middle/region.rs
Outdated
@@ -890,7 +890,7 @@ fn resolve_local<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, | |||
/// | box P& | |||
fn is_binding_pat(pat: &hir::Pat) -> bool { | |||
match pat.node { | |||
PatKind::Binding(hir::BindByRef(_), ..) => true, | |||
PatKind::Binding(hir::BindByRef(_), ..) => true, // FIXME(tschottdorf): vetted and perhaps problematic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think this is OK, but it's a subtle one. This has to do with when we will consider temporaries on the right-hand-side (that is, the initializer) to live for a longer time. So in particular if you write something like:
let (ref x, ref y) = (Foo { .. }, Bar { .. });
the temporary value created for the initializer (the tuple) would be extended to live until the end of the enclosing block (as opposed to being dropped after the let
is complete).
To create an implicit ref, however, you must have a borrowed value on the RHS already. So that means you would be doing something like:
let Foo { x, .. } = &Foo { x: ..., ... };
in place of
let Foo { ref x, .. } = Foo { ... }
In the former case (the implicit ref version), the temporary is created by the &
expression, and indeed its lifetime would be extended to the end of the block, but due to a different rule, not is_binding_pat
.
It's probably worth copying this comment into the source as a note for why it's ok to just look for explicit ref
keywords here.
@@ -930,7 +931,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { | |||
Some(ImmutabilityBlame::LocalDeref(node_id)) => { | |||
let let_span = self.tcx.hir.span(node_id); | |||
match self.local_binding_mode(node_id) { | |||
hir::BindingMode::BindByRef(..) => { | |||
hir::BindingMode::BindByRef(..) => { // FIXME(tschottdorf): vetted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use a distinct type for the values stored in the table -- i.e., ty::BindingMode::BindByRef
or whatever. Were you considering doing that as part of this PR? It would cover a lot of these "vetted" cases -- i.e., a grep for hir::BindingMode
would yield far fewer results. (Ideally, we would give them distinct names, too...)
Maybe hir::BindingAnnotation
and ty::BindingMode
or something like that? Doesn't seem great; I'm shooting for names that convey that what is in HIR is not the final mode, but just covers what the user actually wrote. I envision:
pub enum BindingAnnotation {
// No binding annotation given: this means that the final binding mode
// will depend on whether we have skipped through a `&` refrence
// when matching. For example, the `x` in `Some(x)` will have binding
// mode `None`; if you do `let Some(x) = &Some(22)`, it will ultimately be
// inferred to be by-reference.
None,
// Annotated with `mut x` -- could be either ref or not, similar to `None`.
Mutable,
// Annotated as `ref`, like `ref x`
Ref,
// Annotated as `ref mut x`.
RefMut,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you considering doing that as part of this PR?
Yeah, just did that.
I like BindingAnnotation
, actually, and it's pretty hard to "misuse" its variants. BindingMode
should work for now, too. I probably put the new ty::BindingMode
in the wrong place and perhaps set up things a little awkwardly (in particular, do I want to keep ty::BindingMode
as in the new version, or should it drop the hir::Mutability
and be more of a Mutable/Ref/RefMut
enum?), waiting for your linting there.
src/librustc_lint/unused.rs
Outdated
p.each_binding(|mode, id, _, path1| { | ||
p.each_binding(|bm, id, _, path1| { | ||
// FIXME(tschottdorf): this fires the assertion. Why? | ||
// let bm = *cx.tables.pat_binding_modes.get(&p.id).expect("missing binding mode"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. That is indeed confusing; it would be helpful if you did something like this:
p.each_binding(|bm, id, span, path1| { // <-- I added `span` in place of `_`
let bm = match cx.tables.pat_bindings_modes.get(&p.id) {
Some(&bm) => bm,
None => span_bug!(span, "missing binding mode")
};
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, the devil is in the details. Crashes no more!
- let bm = match cx.tables.pat_binding_modes.get(&p.id) {
+ let bm = match cx.tables.pat_binding_modes.get(&id) {
ping @tschottdorf, just wanted to make sure this is still on your radar! |
It is, no worries :) my open source day is Friday, though. |
230ad2f
to
5861dca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikomatsakis ready for another look. I kept the changes in a second commit for easier reviewing on your end, but the intention is that it would be squashed before merge.
PatKind::Binding(bind_mode, ..) => { | ||
if bind_mode == hir::BindByValue(hir::MutMutable) { | ||
PatKind::Binding(bm, ..) => { | ||
// FIXME(tschottdorf): no tables in scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was easy, done.
src/librustc_lint/unused.rs
Outdated
p.each_binding(|mode, id, _, path1| { | ||
p.each_binding(|bm, id, _, path1| { | ||
// FIXME(tschottdorf): this fires the assertion. Why? | ||
// let bm = *cx.tables.pat_binding_modes.get(&p.id).expect("missing binding mode"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, the devil is in the details. Crashes no more!
- let bm = match cx.tables.pat_binding_modes.get(&p.id) {
+ let bm = match cx.tables.pat_binding_modes.get(&id) {
@@ -930,7 +931,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { | |||
Some(ImmutabilityBlame::LocalDeref(node_id)) => { | |||
let let_span = self.tcx.hir.span(node_id); | |||
match self.local_binding_mode(node_id) { | |||
hir::BindingMode::BindByRef(..) => { | |||
hir::BindingMode::BindByRef(..) => { // FIXME(tschottdorf): vetted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you considering doing that as part of this PR?
Yeah, just did that.
I like BindingAnnotation
, actually, and it's pretty hard to "misuse" its variants. BindingMode
should work for now, too. I probably put the new ty::BindingMode
in the wrong place and perhaps set up things a little awkwardly (in particular, do I want to keep ty::BindingMode
as in the new version, or should it drop the hir::Mutability
and be more of a Mutable/Ref/RefMut
enum?), waiting for your linting there.
src/librustc/hir/pat_util.rs
Outdated
@@ -128,9 +128,11 @@ impl hir::Pat { | |||
contains_bindings | |||
} | |||
|
|||
// FIXME(tschottdorf): this method will have to go away. The callers are usually | |||
// trying to print nice errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked through the callers.
if let Some(simple_name) = pattern.simple_name() {
labels.push((pattern.span, format!("consider giving `{}` a type", simple_name)));
} else {
labels.push((pattern.span, format!("consider giving the pattern a type")));
}
let (error_var, span_label_var) = if let Some(simple_name) = arg.pat.simple_name() {
(format!("the type of `{}`", simple_name), format!("the type of `{}`", simple_name))
} else {
("parameter type".to_owned(), "type".to_owned())
};
// Check that argument is Sized.
// The check for a non-trivial pattern is a hack to avoid duplicate warnings
// for simple cases like `fn foo(x: Trait)`,
// where we would error once on the parameter as a whole, and once on the binding `x`.
if arg.pat.simple_name().is_none() {
fcx.require_type_is_sized(arg_ty, decl.output.span(), traits::MiscObligation);
}
Doesn't seem that I need to change anything here after all.
src/librustc/hir/pat_util.rs
Outdated
@@ -167,7 +169,7 @@ impl hir::Pat { | |||
pub fn contains_ref_binding(&self) -> Option<hir::Mutability> { | |||
let mut result = None; | |||
self.each_binding(|mode, _, _, _| { | |||
if let hir::BindingMode::BindByRef(m) = mode { | |||
if let hir::BindingMode::BindByRef(m) = mode { // FIXME(tschottdorf): vetted, but probably problematic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this can be fixed before we have (the logic to compute) pat_adjustments
in place. We have to know whether we're introducing implicit refs whenever contains_ref_binding
is called. For now, I renamed to contains_explicit_ref_binding
and added FIXMEs
.
src/librustc/hir/print.rs
Outdated
@@ -1651,12 +1651,12 @@ impl<'a> State<'a> { | |||
PatKind::Wild => self.s.word("_")?, | |||
PatKind::Binding(binding_mode, _, ref path1, ref sub) => { | |||
match binding_mode { | |||
hir::BindByRef(mutbl) => { | |||
hir::BindByRef(mutbl) => { // FIXME(tschottdorf): vetted and problematic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
0cefe37
to
7631905
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Left a nit but I don't feel inclined to block this PR on it.
src/librustc/hir/mod.rs
Outdated
// mode `None`; if you do `let Some(x) = &Some(22)`, it will | ||
// ultimately be inferred to be by-reference. | ||
// | ||
// Note that implicit reference skipping is not implemented yet (#42640). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Nice comment. Would be better to use ///
if bm == hir::BindByValue(hir::MutMutable) { // FIXME(tschottdorf): vetted, maybe problematic | ||
PatKind::Binding(..) => { | ||
let bm = *tables.pat_binding_modes.get(&p.id).expect("missing binding mode"); | ||
if bm == ty::BindByValue(hir::MutMutable) { // FIXME(tschottdorf): vetted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove this FIXME
src/librustc_lint/unused.rs
Outdated
p.each_binding(|_, id, span, path1| { | ||
let bm = match cx.tables.pat_binding_modes.get(&id) { | ||
Some(&bm) => bm, | ||
None => span_bug!(span, "missing binding mode"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we ever get to the bottom of this? I guess so...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had used id instead of p.id.
// FIXME(tschottdorf): don't call contains_explicit_ref_binding, which | ||
// is problematic as the HIR is being scraped, but ref bindings may be | ||
// implicit after #42640. We need to make sure that pat_adjustments | ||
// (once introduced) is populated by the time we get here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is annoying. The comments in #23116 are somewhat helpful in bringing this back into cache. I have a feeling that scraping the HIR is actually just fine, but I have to think about it and convince myself (and if not, decide what to do...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No they're not. The problem is that if we have something like
let Foo(x) = f()[0];
Then if the pattern matches by reference, we want to match f()[0]
as a lexpr, so we can't allow it to be coerced. But if the pattern matches by value, f()[0]
is still syntactically a lexpr, but we do want to allow coercions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can be okay with allowing coercions to happen if there are no explicit ref mut
patterns - all implicit ref mut
patterns must occur behind a reference, so they will have the "correct" variance and lifetime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that the following pattern would be legal:
struct Foo(Bar);
struct Bar(u32);
impl Deref for Foo {
type Target = Bar;
fn deref(&self) -> &Bar { &self.0 }
}
impl DerefMut for Foo {
fn deref_mut(&mut self) -> &mut Bar { &mut self.0 }
}
fn foo(x: &mut Foo) {
{
let Bar(z): &mut Bar = x;
*z = 42;
}
assert_eq!(foo.0.0, 42);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can be okay with allowing coercions to happen if there are no explicit ref mut patterns - all implicit ref mut patterns must occur behind a reference, so they will have the "correct" variance and lifetime.
Yes, this was my thinking. I think that the current rules are "stricter than necessary" in this respect (as the comments indeed indicate).
This means that the following pattern would be legal:
I am not sure what your code is showing. In particular, was assert_eq!(foo.0.0, 42);
meant to be x.0.0
? Also, did you envision a problem with this code being accepted? (I would expect it to work, naively.)
7631905
to
7308476
Compare
@bors r+ |
@bors: r=nikomatsakis |
📌 Commit 7308476 has been approved by |
⌛ Testing commit 730847605fbd49adfce51d7c78dc92b3c0151bf2 with merge 3c6a6070d206e0a38319f9ea0c463c2e292e3515... |
💔 Test failed - status-appveyor |
@bors retry
|
⌛ Testing commit 730847605fbd49adfce51d7c78dc92b3c0151bf2 with merge 7bec830bb1436bcfbe519c1d7d3713dc0024c223... |
💔 Test failed - status-appveyor |
https://ci.appveyor.com/project/rust-lang/rust/build/1.0.4120/job/nef70pywepcisqra
|
I seem to have accidentally renamed |
7308476
to
1b0ceb4
Compare
This PR kicks off the implementation of the [default binding modes RFC][1] by introducing the `pat_binding_modes` typeck table mentioned in the [mentoring instructions][2]. `pat_binding_modes` is populated in `librustc_typeck/check/_match.rs` and used wherever the HIR would be scraped prior to this PR. Unfortunately, one blemish, namely a two callers to `contains_explicit_ref_binding`, remains. This will likely have to be removed when the second part of [1], the `pat_adjustments` table, is tackled. Appropriate comments have been added. See rust-lang#42640. [1]: rust-lang/rfcs#2005 [2]: rust-lang#42640 (comment)
1b0ceb4
to
851c770
Compare
Cleaned up. |
It would be nice if you would add my comment about binding modes as a code comment. |
@arielb1 happy to do so, could you provide the comment? I'm not sure what exactly you'd want to see there at this point. |
@bors r+ |
📌 Commit 8f67f1e has been approved by |
@bors retry (let's see if this works) bors is stuck waiting for 43009 to build which has been merged 2 days ago...? |
⌛ Testing commit 8f67f1e with merge 35f97a3596a8a00effec4e5d6448e423de9c0d15... |
…sakis default binding modes: add pat_binding_modes This PR kicks off the implementation of the [default binding modes RFC][1] by introducing the `pat_binding_modes` typeck table mentioned in the [mentoring instructions][2]. It is a WIP because I wasn't able to avoid all uses of the binding modes as not all call sites are close enough to the typeck tables. I added marker comments to any line matching `BindByRef|BindByValue` so that reviewers are aware of all of them. I will look into changing the HIR (as suggested in [2]) to not carry a `BindingMode` unless one was explicitly specified, but this PR is good for a first round of comments. The actual changes are quite small and CI will fail due to overlong lines caused by the marker comments. See #42640. cc @nikomatsakis [1]: rust-lang/rfcs#2005 [2]: #42640 (comment)
☀️ Test successful - status-appveyor, status-travis |
This PR kicks off the implementation of the default binding modes RFC by
introducing the
pat_binding_modes
typeck table mentioned in the mentoringinstructions.
It is a WIP because I wasn't able to avoid all uses of the binding modes as
not all call sites are close enough to the typeck tables. I added marker
comments to any line matching
BindByRef|BindByValue
so that reviewersare aware of all of them.
I will look into changing the HIR (as suggested in 2) to not carry a
BindingMode
unless one was explicitly specified, but this PR is good fora first round of comments.
The actual changes are quite small and CI will fail due to overlong lines
caused by the marker comments.
See #42640.
cc @nikomatsakis