Ensure StorageDead is created even if variable initialization fails #51109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
r? @nikomatsakis cc @arielb1 Will this do the right thing in terms of drops, too? |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Looks like this flips the drop order: Is this critical or can it be ignored? |
|
@manuels Looks like we have to schedule the |
|
If this passed all tests, but still changes the drop order, can we add the test listed here, or some simplified variant of it? |
|
Sorry, I think my comment was misleading: this is a test that alreay exists. I just quoted it here. I will have a long weekend starting this afternoon to work on this issue and implement @eddyb's suggestion. |
|
I'm afraid, I am not able to find the time working on this issue, so if anybody wants to fix it, please go ahead! |
|
@manuels Did you get anywhere with splitting the two drop kinds? Do you have an in-progress branch? |
|
Not really, also because I did not really know where to insert |
|
@manuels I would say call for both when previously only a single call existed, and instead of there being logic to decide which is needed, it would check if the |
|
That's what I tried, actually, but maybe I got the logic wrong in `schedule_drop()`, because a lot of tests failed.
…On June 5, 2018 11:37:47 PM CEST, Eduard-Mihai Burtescu ***@***.***> wrote:
@manuels I would say call for both when previously only a single call
existed, and instead of there being logic to decide which is needed, it
would check if the `DropKind` given applies at all.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#51109 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
|
@manuels Do you have what you tried anywhere? Or can you just push it to this PR? |
|
@eddyb I pushed a WIP commit. |
eddyb
reviewed
Jun 6, 2018
| @@ -188,8 +189,17 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
| self.user_assert_ty(block, ty, var, irrefutable_pat.span); | |||
| } | |||
|
|
|||
| self.schedule_drop_for_binding(var, irrefutable_pat.span, OutsideGuard); | |||
| self.into(&place, block, initializer) | |||
| let block = self.into(&place, block, initializer); | |||
There was a problem hiding this comment.
This needs to be in between the storage and the value drop.
There was a problem hiding this comment.
Right, I remember I was playing around in the end and this is what I tried last.
eddyb
reviewed
Jun 6, 2018
| let local_id = self.var_local_id(var, for_guard); | ||
| let var_ty = self.local_decls[local_id].ty; | ||
| let hir_id = self.hir.tcx().hir.node_to_hir_id(var); | ||
| let region_scope = self.hir.region_scope_tree.var_scope(hir_id.local_id); | ||
| self.schedule_drop(span, region_scope, &Place::Local(local_id), var_ty); | ||
| // where is the DropKind? | ||
| // region_scope.drops[].kind |
There was a problem hiding this comment.
Not sure if the comment is relevant still.
eddyb
reviewed
Jun 6, 2018
src/librustc_mir/build/mod.rs
Outdated
| argument_scope, &place, ty); | ||
| argument_scope, &place, ty, drop_kind); | ||
|
|
||
| //let drop_kind = DropKind::Value { cached_block: CachedBlock::default() }; |
There was a problem hiding this comment.
For arguments only DropKind::Value is needed.
eddyb
reviewed
Jun 6, 2018
src/librustc_mir/build/scope.rs
Outdated
| } else { | ||
| // Only temps and vars need their storage dead. | ||
|
|
||
| if let DropKind::Storage = drop_kind { |
There was a problem hiding this comment.
I'd suggest using a match and making both variants explicit, with Value first (just to make the diff more readable).
|
@manuels Left a few comments. Most of it seems good! You also need to rebase on master. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
eddyb
reviewed
Jun 6, 2018
|
|
||
| let drop_kind = DropKind::Value { cached_block: CachedBlock::default() }; | ||
| this.schedule_drop(expr_span, scope, &Place::Local(result), value.ty, drop_kind); | ||
| this.schedule_drop(expr_span, scope, &Place::Local(result), value.ty, | ||
| drop_kind); |
There was a problem hiding this comment.
With this formatting, you can probably just put the value in the call itself, without having a drop_kind variable.
|
@manuels You need to run |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@manuels Can you use |
Sure, I just did it like that because the @rust-highfive told me to:
Not sure what you mean by that. |
|
I do not understand why I'm getting this error now: |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
That's not that great of advice. GitHub handles amends and force pushes just as well as multiple commits. The only exception is when logically separated/stacked changes are made. So fixing a previous commit should just be amending the commit, adding onto the PR (e.g. implementing another sub-feature) can be done in separate commits.
So as far as I can tell, you used Run While there is a way to do this on the command-line, I'm not sure how to actually do it correctly, because you have to somehow do something like In general, I recommend committing using |
e9ec847 to
676d611
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #51426) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Git is freaking me out! |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@manuels Can you get on IRC? It'd be easier to discuss things in real-time. |
|
Ping from triage @manuels! It's been a while since we heard from you, will you have time to work on this again? |
|
Hey, @pietroalbini, sorry, I am quite busy with other stuff right now. So if anybody would like to work on this bug, feel free to do so! |
|
Weekly ping from triage @manuels, just making sure you don't forget about this! (please reply so we don't close the PR next week) |
|
Well, actually I think it would be fine if you close it. I won't have the time to fix it because it became more complicated than I thought and I won't have the time to start learning how the compiler handles all the lifetimes. |
|
Sure, thank you for your work anyway! If you'll want to continue this PR in the future please reopen it! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch fixes a bug where the compiler does not create a
StorageDeadfor a variable that fails to initialize due tobreak.@eddyb has suggested to flip
Builder::schedule_drop_for_binding()and the call toBuilder::into()on IRC.Fixes #49232