Document how closure capturing interacts with discriminant reads #1837
Conversation
rustbot
added
the
S-waiting-on-review
src/types/closure.md
Outdated
| r[type.closure.capture.precision.discriminants] | ||
| ### Capturing for discriminant reads | ||
|
|
||
| If pattern matching requires inspecting a discriminant, the relevant place will get captured by `ImmBorrow`. |
There was a problem hiding this comment.
Unless we mutably access the contents, right? E.g.
let c = || match x {
(Example::A(ref mut _y), _) => println!("variant A"),
(Example::B(_), _) => println!("variant B"),
};Is that already implied by the other rules?
There was a problem hiding this comment.
ok yep, that's the "Shared prefix" thing
There was a problem hiding this comment.
Yup. Do you think this deserves a clarification? (if so, how would you word it)
There was a problem hiding this comment.
no, I think it's clear in the context
src/types/closure.md
Outdated
| c(); | ||
| ``` | ||
|
|
||
| Likewise, a slice pattern that matches slices of all possible lengths does not constitute a discriminant read. |
There was a problem hiding this comment.
We're talking about changing this behavior. cc @Nadrieril.
That said, I think we should merge it in this form, and then update this when we've changed this behavior.
There was a problem hiding this comment.
Yes, the idea to change this came from the PR that is being documented by this change.
traviscross
added
the
S-waiting-on-stabilization
src/types/closure.md
Outdated
| Matching against a [range pattern][patterns.range] constitutes a discriminant read, even if | ||
| the range matches all possible values. |
There was a problem hiding this comment.
I'm not quite following this terminology, particularly around the user of the term "discriminant". My understanding is that a discriminant is specifically a tag associated with an enum. I wouldn't consider an integer to have a discriminant. Can you clarify what this means?
There was a problem hiding this comment.
Right, this happened because the relevant rustc has a distinction between (a) binding sub-values to identifiers with various binding modes, and (b) discriminant reads—with other "shallow/immediate" reads, such as the length field of a slice, being shoe-horned into the category of discriminant reads.
From the perspective of reference text, this obviously doesn't make sense. I'll rephrase that as just "constitutes a read", I think, unless you have any better ideas on how to word this?
There was a problem hiding this comment.
Ah, I gave this another look and it's a "discriminant read" in the sense of type.closure.capture.precision.discriminants. I'll try to find a way to reword this all.
src/types/closure.md
Outdated
| Matching against a [slice pattern][patterns.slice] constitutes a discriminant read if | ||
| the slice pattern needs to inspect the length of the scrutinee. |
There was a problem hiding this comment.
Same question about the use of the term "discriminant".
|
I've reworded the slice and range sections to not use the "discriminant read" term, do you think this is clearer? |
src/types/closure.md
Outdated
| r[type.closure.capture.precision.discriminants.slice-patterns] | ||
| Matching against a [slice pattern][patterns.slice] constitutes a discriminant read if | ||
| the slice pattern needs to inspect the length of the scrutinee. | ||
| Matching against a [slice pattern][patterns.slice] performs a read if the slice pattern needs to inspect the length of the scrutinee. The read will cause the closure to borrow the relevant place by `ImmBorrow`. |
There was a problem hiding this comment.
Maybe say "performs a read of the pointer value" to avoid confusion about what is being read? Should we even say that this reads only the pointer metadata (not sure that's what we're doing today even)?
There was a problem hiding this comment.
Okay, how about this new revision?
|
This looks good for me, could someone with authority approve this plz? |
traviscross
force-pushed
the
discriminant-reads
branch
from
3f847d5 to
01c6c4c
Compare
This comment has been minimized.
This comment has been minimized.
|
Thanks. Reviewing. |
src/types/closure.md
Outdated
|
|
||
| ```rust,compile_fail,E0506 | ||
| let x: &mut [i32] = &mut [1, 2, 3]; | ||
| let c = || match x { // captures `*x` by ImmBorrow |
There was a problem hiding this comment.
Does it capture *x or does it capture x? It would seem to make more sense for it to be capturing the wide pointer itself rather than the pointee, and the MIR makes it look like that's what it's doing (i.e., within the closure, x: &&mut [i32]).
There was a problem hiding this comment.
I don't have the bandwidth to test this myself right now, but I assume you're hitting this optimization: https://github.com/rust-lang/rust/blob/016baacaf6ed5698625036e3afe3900535015680/compiler/rustc_hir_typeck/src/upvar.rs#L2543-L2572
My understanding, based on knowledge that has faded a bit already, is that the language semantics says we capture *x, but the actual compiled output optimizes that to capturing x instead, on account of that not being observable. I assume that if you stare 5–10 minutes at the doc comment I linked, you'll be able to come up with an example where the optimization doesn't apply and the capture of *x can be observed in the MIR.
There was a problem hiding this comment.
Thanks for those details. I've revised it to state explicitly that we capture the pointee in this case and added an admonition referencing the doc comment you mention.
There was a problem hiding this comment.
In reviewing this with @ehuss, we found, intriguingly, that prior to your PR rust-lang/rust#138961, it captures the pointer, and after your PR, it captures the pointee. E.g., with this code:
fn main() {
let x: &mut [u8] = &mut [];
let c = || match x {
[] => (),
_ => (),
};
c();
}Without the PR, we get this MIR for the closure:
fn main::{closure#0}(_1: &{closure@src/main.rs:3:13: 3:15}) -> () {
debug x => (*((*_1).0: &&mut [u8]));
let mut _0: ();
bb0: {
return;
}
}With the PR, we get this MIR instead:
fn main::{closure#0}(_1: &{closure@src/main.rs:3:13: 3:15}) -> () {
debug x => (*((*_1).0: &[u8]));
let mut _0: ();
bb0: {
return;
}
}
traviscross
force-pushed
the
discriminant-reads
branch
from
01c6c4c to
151b109
Compare
This comment has been minimized.
This comment has been minimized.
|
This PR was in good shape. Thanks to @meithecatte for that and to @Nadrieril for the review of it. I've pushed some mostly editorial adjustments. Please have a look, of course, for any errors. I'll ask @ehuss to have a look at well on our lang-docs call. Overall, though, this is OK to go forward, so we can mark this as approved and unblock the underlying stabilization. |
traviscross
removed
the
S-waiting-on-review
traviscross
force-pushed
the
discriminant-reads
branch
from
56cbaed to
b5660fe
Compare
This is the behavior after the bugfixes in rustc PR 138961.
To the language about a capture path, this branch adds, "..., as well as any further projections performed by matching against patterns", and it links to the section on capturing and wildcard patterns. The link is a bit unobvious, since the section discusses what's not captured. The "as well as" phrasing is a bit awkward, and it's introduced after a comma. That indicates a non-restrictive clause, which this is not. The next rule talks about what place projections are. It seems that it might be better to instead extend that language than to try to fit this in here. We can say that destructuring can effect a place projection, and that seems right. Let's do that.
Let's tighten up the wording a bit in the introduction to wildcard patterns and capturing to make it better match how we'll present things below. We'll remove the "for example..." ahead of the example, as we're not doing this below, and we'll separately extend the comments within the examples to include the relevant information.
In this branch, we're adding a statement in the wildcard section about how destructuring tuples, structs, and single-variant enums, by itself, does not constitute a read. Let's add some examples of this to demonstrate.
Rather than saying "this also includes", let's make this statement stand better on its own. This will be helpful when giving it a rule identifier, and given how this statement is some distance, visually, from the other statements due to the examples, it adds clarity.
Let's give the claim about the effect of destructuring with respect to reads and capturing a rule identifier.
When a fragment of a rule identifier matches a Rust identifier, we use an underscore rather than a dash as a separator. Let's do that here.
The normative rule about destructuring and capturing does not itself state the exception for `non_exhaustive` enums. That exception follows below. Let's add an admonition to link down to it so it's not missed. It's not great having this duplication; it'd be better if we weren't restating this fact twice. But it's not clear what to do about this. The first statement is making a broader statement about tuples, structs, and single-variant enums. The section that follows below is focused on discriminants. We'll think about this more later.
Let's clean up the wording of this claim a bit. Let's put capturing in the present tense, rather than in the future tense, as that's how we're going to continue below. Let's take out, as well, the explicit lead-in to the example, as we're not generally doing that in these sections.
Let's add a rule identifier for the rule about wildcards and the capturing of fields.
Let's remove the explicit lead-in to the example from the rule about (the lack of) partial captures of arrays and slices.
What this example is showing is how one field of a struct can be captured while another is not (when using rest patterns). However, the example above demonstrates this with tuples, and there are no interesting differences between tuple fields and struct fields in this case.
Some of the examples in this section did not really prove the claim
they were making; let's adjust them to do this. Let's also simplify
and minimize them to match the style of other recent examples we're
adding and that of the ones that will follow in the section below.
One particular stylistic conundrum is what to do about comments like
this:
// ERROR: Borrow of moved value.
Normally, our stylistic convention would be to capitalize after the
comma in this case and end with a period. That's what we'd do, e.g.,
in a similar case like this:
// OK: The value can be moved here.
But, of course, `rustc` doesn't capitalize and add a period to these
kind of error messages, making it tempting to follow that lead.
Since we already don't always use the same error messages that `rustc`
does -- it's not a goal to match those -- it seems better to be
internally consistent with our own documentation norms. Let's
capitalize and add the period. We'll later add this to the style
guide and work to align the document with this.
The rule, > If pattern matching requires inspecting a discriminant, the relevant > place will get captured by `ImmBorrow`. is unclear in two ways. For one, when does pattern matching "require inspecting" the discriminant? Second, what is the "relevant" place? Let's break this up into two rules (and add rule identifiers). The first rule will state that reading the discriminant captures the place containing the discriminant (by `ImmBorrow`). The second rule will state explicitly the baseline rule for when such reads happen.
The example here showed what isn't captured, but it didn't demonstrate what is captured. Let's show both. We'll specifically use a non-`Copy` type for this because [Rust PR #138961] affects the behavior in the non-`Copy` case. [Rust PR #138961]: rust-lang/rust#138961
Let's simplify the wording of the rule a bit, reiterate the effect of the rule (as we're doing elsewhere), and tighten up the example.
Let's clarify some wording in this rule, making it a bit more self-standing, and inline the link target as, when using the rule identifier for this, it's not meaningfully longer than doing it the other way.
Let's rename this rule identifier to the plural; we generally use the plural whenever it can make sense, and it does here. Let's make the wording of the rule more clear and self-standing. And let's tighten up the example.
Let's tighten up the text for this rule and condense it down into one sentence. It's a bit awkward to have another sentence just to say, "this is true even if...". Let's clean up the example as well.
The section here stated: > Matching against a slice pattern that needs to inspect the length of > the scrutinee performs a read of the pointer value in order to fetch > the length. The read will cause the closure to borrow the relevant > place by `ImmBorrow`. It then goes on to state exceptions for arrays matched against slice patterns and slice patterns containing only a rest pattern. As we saw in an earlier commit, it's better to not lean on the reader to infer the "relevant place" or when the length needs to be inspected. Let's elaborate those details and state the full rule in one go, upfront, and then state a separate guarantee that matching an array against a slice pattern does not do a read. We'll also fix a typo, add rule identifiers, and tighten up the examples.
The rules for the capturing behavior related to range patterns and slice patterns were comingled with the rules for capturing and discriminant reads. While these are spiritually related, they are distinct, so let's break these apart into separate sections.
For our purposes in describing capture paths, we define place projections. For the text that follows in the sections below to hold, we need destructuring to be treated as a place projection for this purpose. However, it might seem surprising for destructuring to be in this list. While it is believed to have the same effect as a projection expression, we might not consider an instance of pattern destructuring to be a projection expression exactly. To better contextualize this, let's add an admonition that mentions that pattern destructuring desugars into the kind of field accesses that we would more likely think about as projection operations.
traviscross
force-pushed
the
discriminant-reads
branch
from
b5660fe to
67115fd
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There's some surprising nuance to what gets captured when reading the length of a slice with a slice pattern. Let's add an admonition about this.
While we're here, let's fix a typo on the word "ancestors" in this section.
traviscross
force-pushed
the
discriminant-reads
branch
from
7eff36a to
8339682
Compare
6 participants
This is the behavior after the bugfixes in rust-lang/rust#138961. I have successfully ran
mdbook testwithRUSTUP_TOOLCHAINpointed at a stage1 built on top of the aforementioned PR – until it's merged, the CI here will fail.