Stabilize `destructuring_assignment` by jhpratt · Pull Request #90521 · rust-lan...
source link: https://github.com/rust-lang/rust/pull/90521
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
(rust-highfive has picked a reviewer for you, use r? to override)
How do docs look currently? Are the reference and book updated?
r? rust-lang/lang-team
r? @pnkfelix (picking someone who is both on lang and compiler team)
This looks correct to me.
Do we have tests and overall plan regarding the interaction between destructing assignment and let else? With both features on track to stabilization, I feel we should figure out their interaction at least on some level before stabilization, least we realize we need some future-proofing somewhere after stabilization.
I can't speak as to the status of tests and whatnot, but I would not think there's any interaction at all. After all, let-else inherently requires let, which is not present for destructing assignment.
After all, let-else inherently requires let, which is not present for destructing assignment.
I don’t think it inherently requires let. I can imagine the following working:
let mut x = …;
…
Some(x) = foo() else { return };Anecdotally, couple of time I wanted to use let-else, I needed exactly this form, to update an existing mutable binding.
I'll try to see if I can come up with some regex to find a relevant test. If not I'll just add a test myself.
The stabilization report doesn't mention destructing references. Is the plan to stabilize that later?
let mut x = …; … Some(x) = foo() else { return };
@matklad This code is not accepted by the parser at the moment, but I don't see any reason why that couldn't be added later.
After a quick check, confirming that the parser doesn't accept this. I don't see any potential for compatibility concerns; it seems reasonable that this would be accepted (both syntactically and semantically) in the future.
@matklad We already have such syntactic ambiguities with let-else today:
let () = if true {} else { return }However, as with the example you gave, it involves an expression of type (), and it either won't typecheck or won't have a refutable pattern. Either way, how it parses doesn't seem especially critical; it could even just be rejected as ambiguous and require parentheses to disambiguate.
I think I see at least some syntactic ambiguities of dangling-else variety:
let Some(x) = Some(y) = foo() else { return };
Some(x) = Some(y) = foo() else { return };
I don't think it makes sense to decide what are we going to do with them right now, but I do think that we should have an unresolved question somewhere, which at least outlines design space and good and poor interactions.
To clarify, the above is not an exhaustive list of potential problems, it's rather a counterexample to "it'll obviously just work together".
I wanted to check how (a,b) = (b,a) compares to mem::swap, it looks like mem::swap applies optimizations when T is 32 bytes or bigger but destructuring_assignment doesn't.
Let's keep this PR related to the stabilization and potential compatibility concerns. For optimization issues with destructuring_assignment it would probably be best to open a new, explicit issue.
@jhpratt can now approve this pull request
LGTM. Thanks @jhpratt ! (And sorry for how much time I've taken to look at this.)
Thanks @pnkfelix. No worries about the delay; I was under the impression this was waiting on docs anyways.
@bors r=jackh726,pnkfelix
Commit a3d1edb has been approved by jackh726,pnkfelix
@bors r-
I think a test needs an update?
#91897 (comment)
force-pushed the stabilize-destructuring_assignment
branch
from
a3d1edb
to
d95f749
last month
Surprised that running x t --bless wasn't sufficient. Either way, I've done it on the clippy tests specifically. Let's give this another shot.
@bors r=jackh726,pnkfelix
Commit d95f749 has been approved by jackh726,pnkfelix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK