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

Ensure borrows of fn/closure params do not outlive invocations #30341

Merged
merged 2 commits into from
Dec 17, 2015

Conversation

pnkfelix
Copy link
Member

Ensure borrows of fn/closure params do not outlive invocations.

Does this by adding a new CallSiteScope to the region (or rather code extent) hierarchy, which outlives even the ParameterScope (which in turn outlives the DestructionScope of a fn/closure's body).

Fix #29793

r? @nikomatsakis

@pnkfelix
Copy link
Member Author

@nikomatsakis by the way, as previously discussed, this PR removed BlockScope entirely (and puts in FnScope instead) ... but after doing so, I did think of one potential reason why we might keep BlockScope, or put it back in in the future. Namely, if we eventually adopt the ability to name the lifetimes of blocks, e.g. 'a: { ... 'b: { ... } ... }, then we will probably need BlockScope again (i think...). Or maybe I'm missing something.

Anyway, we don't seem to need it right now, so I took it out.

@@ -488,7 +481,10 @@ impl<'a> LifetimeContext<'a> {
// `self.labels_in_fn`.
extract_labels(self, fb);

self.visit_block(fb);
self.with(FnScope { fn_id: fn_id, body_id: fb.id, s: self.scope },
move |_old_scope, this| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need move here? I wouldn't expect so...?

@nikomatsakis
Copy link
Contributor

r+ modulo nits.

@pnkfelix pnkfelix force-pushed the call-site-scope branch 2 times, most recently from c1deeb9 to 5299441 Compare December 15, 2015 13:19
resolve_lifetime.rs: Switch from BlockScope to FnScope in ScopeChain
construction. Lifetimes introduced by a fn signature are scoped to the
call-site for that fn. (Note `add_scope_and_walk_fn` must only add
FnScope for the walk of body, *not* of the fn signature.)

region.rs: Introduce new CodeExtentData::CallSiteScope variant. Use
CodeExtentData as the cx.parent, rather than just a NodeId.  Change
DestructionScopeData to CallSiteScopeData.

regionck.rs: Thread call_site_scope via Rcx; constrain fn return
values.

(update; incorporated review feedback from niko.)
@pnkfelix
Copy link
Member Author

@bors r=nikomatsakis 5299441

@bors
Copy link
Contributor

bors commented Dec 15, 2015

🙀 5299441 is not a valid commit SHA. Please try again with 882bca8.

@pnkfelix
Copy link
Member Author

@bors r=nikomatsakis 5299441

update: see barosl/homu#106

@bors
Copy link
Contributor

bors commented Dec 15, 2015

🙀 5299441954b4678f23f3c0c0b2080b3430a2d028 is not a valid commit SHA. Please try again with 882bca8.

@pnkfelix
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 15, 2015

📌 Commit 882bca8 has been approved by nikomatsakis

bors added a commit that referenced this pull request Dec 16, 2015
Ensure borrows of fn/closure params do not outlive invocations.

Does this by adding a new CallSiteScope to the region (or rather code extent) hierarchy, which outlives even the ParameterScope (which in turn outlives the DestructionScope of a fn/closure's body).

Fix #29793

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Dec 16, 2015

⌛ Testing commit 5299441 with merge 4af4278...

@bors bors merged commit 5299441 into rust-lang:master Dec 17, 2015
@nikomatsakis nikomatsakis added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 8, 2016
@nikomatsakis
Copy link
Contributor

Marking as relnotes since this is a soundness fix that did cause some amount of downstream fallout (e.g., #30519).

@brson brson added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jan 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants