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

Tracking issue: Rvalue static promotion #38865

Closed
nrc opened this issue Jan 6, 2017 · 24 comments
Closed

Tracking issue: Rvalue static promotion #38865

nrc opened this issue Jan 6, 2017 · 24 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nrc
Copy link
Member

nrc commented Jan 6, 2017

RFC: https://github.com/rust-lang/rfcs/blob/master/text/1414-rvalue_static_promotion.md

RFC PR: rust-lang/rfcs#1414

Promote constexpr rvalues to values in static memory instead of stack slots, and expose those in the language by being able to directly create 'static references to them. This would allow code like let x: &'static u32 = &42 to work.

Related issues (closed in favor of this one):

@nrc nrc added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 6, 2017
@eddyb
Copy link
Member

eddyb commented Jan 6, 2017

If anyone is interested, all you have to do is remove this & false and write tests.

@eddyb eddyb added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jan 6, 2017
@meetmangukiya
Copy link

I'd like to take a shot at this

@theduke
Copy link
Contributor

theduke commented Jan 10, 2017

Just removing the match statement mentioned by @eddyb results in errors in core, all (at least until abort) related to & mut[]:

Is there something going on with [] being recognized as promotable to static?

I'll start getting familiar with the code. 🔬

error: borrowed value does not live long enough
   --> src/libcore/slice.rs:837:60
    |
837 |             ops::RangeInclusive::Empty { .. } => Some(&mut []),
    |                                                            ^^- temporary value only lives until here
    |                                                            |
    |                                                            does not live long enough
    |
note: borrowed value must be valid for the anonymous lifetime #1 defined on the body at 835:58...
   --> src/libcore/slice.rs:835:59
    |
835 |       fn get_mut(self, slice: &mut [T]) -> Option<&mut [T]> {
    |  ___________________________________________________________^ starting here...
836 | |         match self {
837 | |             ops::RangeInclusive::Empty { .. } => Some(&mut []),
838 | |             ops::RangeInclusive::NonEmpty { end, .. } if end == usize::max_value() => None,
839 | |             ops::RangeInclusive::NonEmpty { start, end } => (start..end + 1).get_mut(slice),
840 | |         }
841 | |     }
    | |_____^ ...ending here

error: borrowed value does not live long enough
   --> src/libcore/slice.rs:854:55
    |
854 |             ops::RangeInclusive::Empty { .. } => &mut [],
    |                                                       ^-
    |                                                       ||
    |                                                       |temporary value only lives until here
    |                                                       does not live long enough
    |
note: borrowed value must be valid for the anonymous lifetime #1 defined on the body at 852:67...
   --> src/libcore/slice.rs:852:68
    |
852 |       unsafe fn get_unchecked_mut(self, slice: &mut [T]) -> &mut [T] {
    |  ____________________________________________________________________^ starting here...
853 | |         match self {
854 | |             ops::RangeInclusive::Empty { .. } => &mut [],
855 | |             ops::RangeInclusive::NonEmpty { start, end } => {
856 | |                 (start..end + 1).get_unchecked_mut(slice)
857 | |             }
858 | |         }
859 | |     }
    | |_____^ ...ending here

error: borrowed value does not live long enough
   --> src/libcore/slice.rs:875:55
    |
875 |             ops::RangeInclusive::Empty { .. } => &mut [],
    |                                                       ^-
    |                                                       ||
    |                                                       |temporary value only lives until here
    |                                                       does not live long enough
    |
note: borrowed value must be valid for the anonymous lifetime #1 defined on the body at 873:52...
   --> src/libcore/slice.rs:873:53
    |
873 |       fn index_mut(self, slice: &mut [T]) -> &mut [T] {
    |  _____________________________________________________^ starting here...
874 | |         match self {
875 | |             ops::RangeInclusive::Empty { .. } => &mut [],
876 | |             ops::RangeInclusive::NonEmpty { end, .. } if end == usize::max_value() => {
877 | |                 panic!("attempted to index slice up to maximum usize");
878 | |             },
879 | |             ops::RangeInclusive::NonEmpty { start, end } => (start..end + 1).index_mut(slice),
880 | |         }
881 | |     }
    | |_____^ ...ending here

error: borrowed value does not live long enough
   --> src/libcore/slice.rs:932:40
    |
932 |     fn default() -> &'a mut [T] { &mut [] }
    |                                        ^^ - temporary value only lives until here
    |                                        |
    |                                        does not live long enough
    |
note: borrowed value must be valid for the lifetime 'a as defined on the body at 932:32...
   --> src/libcore/slice.rs:932:33
    |
932 |     fn default() -> &'a mut [T] { &mut [] }
    |                                 ^^^^^^^^^^^

error: borrowed value does not live long enough
    --> src/libcore/slice.rs:1424:49
     |
1424 |             Some(mem::replace(&mut self.v, &mut []))
     |                                                 ^^ does not live long enough
1425 |         }
     |         - temporary value only lives until here
     |
note: borrowed value must be valid for the lifetime 'a as defined on the body at 1419:48...
    --> src/libcore/slice.rs:1419:49
     |
1419 |       fn finish(&mut self) -> Option<&'a mut [T]> {
     |  _________________________________________________^ starting here...
1420 | |         if self.finished {
1421 | |             None
1422 | |         } else {
1423 | |             self.finished = true;
1424 | |             Some(mem::replace(&mut self.v, &mut []))
1425 | |         }
1426 | |     }
     | |_____^ ...ending here

error: borrowed value does not live long enough
    --> src/libcore/slice.rs:1444:58
     |
1444 |                 let tmp = mem::replace(&mut self.v, &mut []);
     |                                                          ^^ - temporary value only lives until here
     |                                                          |
     |                                                          does not live long enough
     |
note: borrowed value must be valid for the lifetime 'a as defined on the body at 1434:46...
    --> src/libcore/slice.rs:1434:47
     |
1434 |     fn next(&mut self) -> Option<&'a mut [T]> {
     |                                               ^
     = note: consider using a `let` binding to increase its lifetime

error: borrowed value does not live long enough
    --> src/libcore/slice.rs:1479:58
     |
1479 |                 let tmp = mem::replace(&mut self.v, &mut []);
     |                                                          ^^ - temporary value only lives until here
     |                                                          |
     |                                                          does not live long enough
     |
note: borrowed value must be valid for the lifetime 'a as defined on the body at 1469:51...
    --> src/libcore/slice.rs:1469:52
     |
1469 |     fn next_back(&mut self) -> Option<&'a mut [T]> {
     |                                                    ^
     = note: consider using a `let` binding to increase its lifetime

error: borrowed value does not live long enough
    --> src/libcore/slice.rs:1873:54
     |
1873 |             let tmp = mem::replace(&mut self.v, &mut []);
     |                                                      ^^ - temporary value only lives until here
     |                                                      |
     |                                                      does not live long enough
     |
note: borrowed value must be valid for the lifetime 'a as defined on the body at 1868:46...
    --> src/libcore/slice.rs:1868:47
     |
1868 |     fn next(&mut self) -> Option<&'a mut [T]> {
     |                                               ^
     = note: consider using a `let` binding to increase its lifetime

error: borrowed value does not live long enough
    --> src/libcore/slice.rs:1901:27
     |
1901 |             self.v = &mut [];
     |                           ^^- temporary value only lives until here
     |                           |
     |                           does not live long enough
     |
note: borrowed value must be valid for the lifetime 'a as defined on the body at 1898:55...
    --> src/libcore/slice.rs:1898:56
     |
1898 |     fn nth(&mut self, n: usize) -> Option<&'a mut [T]> {
     |                                                        ^
     = note: consider using a `let` binding to increase its lifetime

error: borrowed value does not live long enough
    --> src/libcore/slice.rs:1908:54
     |
1908 |             let tmp = mem::replace(&mut self.v, &mut []);
     |                                                      ^^ - temporary value only lives until here
     |                                                      |
     |                                                      does not live long enough
     |
note: borrowed value must be valid for the lifetime 'a as defined on the body at 1898:55...
    --> src/libcore/slice.rs:1898:56
     |
1898 |     fn nth(&mut self, n: usize) -> Option<&'a mut [T]> {
     |                                                        ^
     = note: consider using a `let` binding to increase its lifetime

error: borrowed value does not live long enough
    --> src/libcore/slice.rs:1936:54
     |
1936 |             let tmp = mem::replace(&mut self.v, &mut []);
     |                                                      ^^ - temporary value only lives until here
     |                                                      |
     |                                                      does not live long enough
     |
note: borrowed value must be valid for the lifetime 'a as defined on the body at 1930:51...
    --> src/libcore/slice.rs:1930:52
     |
1930 |     fn next_back(&mut self) -> Option<&'a mut [T]> {
     |                                                    ^
     = note: consider using a `let` binding to increase its lifetime

error: aborting due to 11 previous errors

error: Could not compile `core`.

@eddyb
Copy link
Member

eddyb commented Jan 10, 2017

It's special-cased still in the RFC, it's the only mutable borrow that is promoted, and handling it at the same time as everything else didn't work out, so what you're seeing is entirely expected.

@brson
Copy link
Contributor

brson commented Feb 15, 2017

Is anybody working on this? Seems like any easy thing to push through. Seems like 1.19 is the earliest this could stablize if it makes it to 1.17 beta.

@eddyb
Copy link
Member

eddyb commented Feb 15, 2017

My assessment is still correct AFAIK, not sure what happened to @theduke.

@badtuple
Copy link

Seems like a couple people started and then dropped this...but if no one is working on it still then I'll do it tonight when I get home.

@eddyb
Copy link
Member

eddyb commented Feb 19, 2017

FWIW, if you do want to feature-gate this, you'll have to put a condition in the place of & false (as opposed to just removing it), but it should still be that same place you need to change.

@brson
Copy link
Contributor

brson commented Feb 24, 2017

Go for it @badtuple !

@topecongiro
Copy link
Contributor

Hello. Is @badtuple still working on this? If not, I would like to take a shot at this.

frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 18, 2017
Add feature gate for rvalue-static-promotion

Probably needs more tests (which ones?) and there may be other things that need to be done. Also not sure whether the version that introduces the flag is really `1.15.1`.

See rust-lang/rfcs#1414.

Updates rust-lang#38865.
arielb1 pushed a commit to arielb1/rust that referenced this issue Mar 18, 2017
Add feature gate for rvalue-static-promotion

Probably needs more tests (which ones?) and there may be other things that need to be done. Also not sure whether the version that introduces the flag is really `1.15.1`.

See rust-lang/rfcs#1414.

Updates rust-lang#38865.
arielb1 pushed a commit to arielb1/rust that referenced this issue Mar 19, 2017
Add feature gate for rvalue-static-promotion

Probably needs more tests (which ones?) and there may be other things that need to be done. Also not sure whether the version that introduces the flag is really `1.15.1`.

See rust-lang/rfcs#1414.

Updates rust-lang#38865.
frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 19, 2017
Add feature gate for rvalue-static-promotion

Probably needs more tests (which ones?) and there may be other things that need to be done. Also not sure whether the version that introduces the flag is really `1.15.1`.

See rust-lang/rfcs#1414.

Updates rust-lang#38865.
@leoyvens
Copy link
Contributor

To make it in 1.19 as per the milestone predictions this needs to be stabilized in the cycle that starts tomorrow. This has been implemented and I see no outstanding issues, this just needs someone to start the FCP.

@Veedrac
Copy link
Contributor

Veedrac commented May 5, 2017

#28546 was closed in favour of this, but that issue was in some ways more general because an arbitrary fn() can in theory be converted to &static Fn(). See this comment which redirects to this discussion. As I understand it, this proposal doesn't include that.

Note that doing this more general solution is likely to be faster, since it doesn't involve double-indirection.

@eddyb
Copy link
Member

eddyb commented May 5, 2017

@Veedrac This proposal gives you &'static typeof(foo) from &foo if foo is a function definition, and if typeof(foo): Fn() then you can coerce it to &'static Fn().

@Veedrac
Copy link
Contributor

Veedrac commented May 5, 2017

@eddyb Unless I'm misunderstanding, though, it doesn't allow for

fn foo() {}
let bar: fn() = foo;
&bar as &'static Fn();

@eddyb
Copy link
Member

eddyb commented May 5, 2017

@Veedrac According to #28546 (comment) that issue wasn't about fn pointers.

And the example you gave is &fn() -> &Trait where fn(): Trait, which cannot shed the extra indirection: it's an instance of &T -> &Trait where T: Trait.
I suspect you want a fn() -> &'static Fn() coercion where the function pointer is used as the data pointer, i.e. as if fn(A) -> R were &'static Code<(A,), R> or something similar.

The problem then is the size and alignment information in the vtable - the only way I can think of to avoid abstractions accidentally "copying" 0 bytes of data behind the data pointer (and using the same vtable pointer), without introducing more special traits (e.g?Sizeable, ?Move, both, etc.), would be to set the size as usize::MAX or thereabouts.

@Veedrac
Copy link
Contributor

Veedrac commented May 5, 2017

@eddyb I think you understand the suggestion correctly. I'm not following what you're saying the problem with copies is, though. Surely "Code" is just an unsized, non-Copy type, so you can't copy it around without specifically-defined functions?

@eddyb
Copy link
Member

eddyb commented May 5, 2017

@Veedrac I meant "copy" as in "the memcpy required for moves", and if you only ever started with references, I suppose you can't end up there, IIRC @nikomatsakis thought the same.
A similar point is made by proponents of using types like &OpaqueData in FFI, I suppose.

I'd comment on #1037 and/or open a new issue/RFC about it if I were you - personally I don't see any problem with a coercion from fn() (not any specific Code-like schemes).

@eddyb
Copy link
Member

eddyb commented Jul 22, 2017

@rfcbot fcp merge

I propose that we stabilize this, as the feature gate has been implemented, the underlying promotion mechanism has been in the compiler for years and I can't think of any outstanding problem with it right now, and it makes #40036 implementable in a backwards-compatible manner.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 22, 2017
@rfcbot
Copy link

rfcbot commented Jul 22, 2017

Team member @eddyb has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 22, 2017
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jul 23, 2017

@eddyb and I discussed a bit this statement of theirs:

and it makes #40036 implementable in a backwards-compatible manner.

and I just wanted to elaborate on what it means for future reference. In short, today, you get an error for something like this:

let x = Some(&0);

which yields:

error[E0597]: borrowed value does not live long enough
 --> src/main.rs:2:19
  |
2 |   let x = Some(&0);
  |                 - ^ temporary value dropped here while still borrowed
  |                 |
  |                 temporary value created here
3 | }
  | - temporary value needs to live until here
  |
  = note: consider using a `let` binding to increase its lifetime

As the error says, this is because the temporary for &0 gets freed after the let completes, but the binding will hold onto that reference until the end of the block. In a static context, however, Some(&0) is currently accepted:

const FOO: Option<&'static i32> = Some(&0);
fn main() { }

If we were to adopt the changes described in #40036, however, then this code would be in error again -- the max lifetime of &0 would be "local" to the const evaluation (it would be a value that gets dropped as part of constant evaluation, and hence we wouldn't be allowed to retain a reference to it in the final value). This would be a regression.

Rvalue promotion resolves by accepting both variants: since 0 is something we could rvalue promote, the max lifetime is 'static, and the temporary created by &0 is exempt from the normal lifetime limits.

@aturon
Copy link
Member

aturon commented Aug 2, 2017

(Checking the box for @pnkfelix, who is away)

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Aug 2, 2017
@rfcbot
Copy link

rfcbot commented Aug 2, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot
Copy link

rfcbot commented Aug 12, 2017

The final comment period is now complete.

@isaacthefallenapple
Copy link

I just stumbled on this:

#![allow(dead_code)]

struct X;

impl X {
    const fn do_nothing(self) -> Self {
        self
    }
}

// Compiles just fine
fn works() -> &'static X {
    &X
}

// Fails to compile, saying that `X.do_nothing()` creates a temporary value owned by the function
fn doesnt_work() -> &'static X {
    &X.do_nothing()
}

// This on the other hand works
fn works_again() -> &'static X {
    const x: &X = &X.do_nothing();
    x
}

Is there a reason this doesn't/shouldn't work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests