-
Notifications
You must be signed in to change notification settings - Fork 13.3k
librustc: Ensure that proc upvars have static lifetime. #15190
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
Conversation
@@ -533,6 +533,10 @@ fn spawn_process_os(cfg: ProcessConfig, | |||
|
|||
let dirp = cfg.cwd.map(|c| c.with_ref(|p| p)).unwrap_or(ptr::null()); | |||
|
|||
let cfg = unsafe { | |||
mem::transmute::<ProcessConfig,ProcessConfig<'static>>(cfg) |
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.
Why is this safe? (Would it be possible to add a brief comment?)
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.
Sure, I can do that.
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 would personally rather this have the two procedures to using closures instead. It will involve an option dance here and there, however.
I'm all for burning down backcompat-lang issues, but this seems very close to the category of "closing a bug to close a bug," as in this doesn't seem to actually fix the problem. This is continuing to paste over how I would imagine that this should fall out of implementing unboxed closures, so it may not be super pressing to actually fix now. It would be nice for the PR message to mention this. For example, this comment:
seems to not be correct. There is no fundamental reason why a boxed once closure (proc) shouldn't be able to capture references and have lifetime bounds. The mentioned issue here merely states that the analysis involved with I suppose that I would simply rather the commit message to state that this is pasting over a very real and plausible use case that we are just not supporting right now, but it is planned to support it in the future. |
@alexcrichton |
Ok, could you update the commit message with that course of action? I'm ok with merging this in the meantime. |
Since procs do not have lifetime bounds, we must do this to maintain safety. This can break code that incorrectly captured references in procedure types. Change such code to not do this, perhaps with a trait object instead. A better solution would be to add higher-rank lifetime support to procs. However, this would be a lot of work for a feature we want to remove in favor of unboxed closures. The corresponding "real fix" is rust-lang#15067. Closes rust-lang#14036. [breaking-change]
Since procs do not have lifetime bounds, we must do this to maintain safety. This can break code that incorrectly captured references in procedure types. Change such code to not do this, perhaps with a trait object instead. Closes #14036. [breaking-change] r? @alexcrichton
Since procs do not have lifetime bounds, we must do this to maintain
safety.
This can break code that incorrectly captured references in procedure
types. Change such code to not do this, perhaps with a trait object
instead.
Closes #14036.
[breaking-change]
r? @alexcrichton