Desugared x.index(y) is not equivalent to x[y]

Opened by Huon Wilson at 2022-03-22 08:15:35

use std::ops::Index;

fn main() {
    let _sugar = &"a".to_owned()[..];
    let _desugar1 = "a".to_owned().index(..);
    let _desugar2 = &*"a".to_owned().index(..);
}

<anon>:5:21: 5:35 error: borrowed value does not live long enough
<anon>:5     let _desugar1 = "a".to_owned().index(..);
                             ^~~~~~~~~~~~~~
<anon>:5:46: 6:49 note: reference must be valid for the block suffix following statement 1 at 5:45...
<anon>:5     let _desugar1 = "a".to_owned().index(..);
<anon>:6     let _desugar2 = &*"a".to_owned().index(..);}
<anon>:5:5: 5:46 note: ...but borrowed value is only valid for the statement at 5:4
<anon>:5     let _desugar1 = "a".to_owned().index(..);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:5:5: 5:46 help: consider using a `let` binding to increase its lifetime
<anon>:5     let _desugar1 = "a".to_owned().index(..);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:6:23: 6:37 error: borrowed value does not live long enough
<anon>:6     let _desugar2 = &*"a".to_owned().index(..);}
                               ^~~~~~~~~~~~~~
<anon>:6:48: 6:49 note: reference must be valid for the block suffix following statement 2 at 6:47...
<anon>:6     let _desugar2 = &*"a".to_owned().index(..);}
                                                        ^
<anon>:6:5: 6:48 note: ...but borrowed value is only valid for the statement at 6:4
<anon>:6     let _desugar2 = &*"a".to_owned().index(..);}
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:6:5: 6:48 help: consider using a `let` binding to increase its lifetime
<anon>:6     let _desugar2 = &*"a".to_owned().index(..);}
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

If the let _desugars are commented out, it compiles fine.

It seems a method call is treated differently to the [] syntax. This may be a purposeful consequence of the design of temporary lifetimes, I don't know.

let a = &"a".to_owned(); works because let ref a = "a".to_owned(); does, AFAIK.

This might be a bug caused by missing checks for overloaded operators, depends when the destructors run. Well, destructors look fine, i.e. the lifetime of the temporary being indexed is extended.
Eduard-Mihai Burtescu at 2015-12-21 13:17:50
triage: P-medium
Niko Matsakis at 2016-01-07 22:17:57
triage: P-high

The high priority is just to investigate and make sure we understand what is going on.
Niko Matsakis at 2016-01-07 22:18:25
@pnkfelix Offers to take this one.
Brian Anderson at 2016-06-23 16:28:54
This is (indeed) by design.

(Ideally we would figure out a way to tweak the Rvalue lifetime rules so that the two forms are equivalent, but I do not know if that is feasible.)

The reason this is occurring: when determining the lifetime for an rvalue, there are a couple different tricks for extending the lifetimes of temporary rvalues to last as long as the scope of some variable binding, rather than the statement the rvalues happen to appear within. (These still need to be documented somewhere outside of the code, see also #12032.)

The particular detail here is we descend expressions according to a grammar to determine which rvalues should have their lifetimes extended, and right now, that grammar looks like this:
```
ET ::= * ET      // dereference
    | ET[...]    // index
    | ET.f       // field access
    | ( ET )     // parenthesized
    | <rvalue>
```
Note that the above does not have method call ET.m(...).

That's the heart of why this is happening: the temporary lifetime rules are based on the syntax that has not had the index expressions desugared to method calls, so you end up with more convenient rules in terms of how long the receiver lives in those cases.

I think it might be interesting to try to extend the rules here so that the grammar looks like this:
```
ET ::= ...        // as above
    | ET.m(...)   // *only* for m that takes `&self` 
                  // or `&mut self`, *not* `self`-by-value
```
Update: we probably would also want the lifetime 'a in &'a self/&'a mut self to be invariant and in the return type of the called method. The reason for the restriction to invariant 'a is that we want to capture the cases where 'a is in the return type in a way that actually corresponds to a reference being passed back to the caller. E.g. we don't want to fall into a trap where the 'a is variant and is able to correspond to a shorter lifetime than that of the whole block, since in that case, extending the lifetime could cause existing code to break (in the sense that it may inject borrowck rejections).

Its also possible that the extension under consideration belongs in the E& grammar rather than the ET grammar. (See comments in code.)
Felix S Klock II at 2016-07-12 17:11:23
(removing P-high on basis of prior investigation.)
Felix S Klock II at 2016-07-12 17:11:41
triage: P-medium
Felix S Klock II at 2016-07-12 17:12:00
Hmm, my idea from above won't work because we do not have a TyCtxt available to consult at the time that we do region resolution (which is where we compute the extents of these temporary r-values).
Felix S Klock II at 2016-07-13 12:12:19
@pnkfelix Could we compute the extents later, before regionck, but after type inference?
Eduard-Mihai Burtescu at 2016-07-13 12:15:32
@eddyb it may be possible, but it would require a pretty serious refactoring of the code, since we compute these extents in the same middle::region::resolve_crate pass that figures out a lot of other region information (all gathered into the RegionMap structure) which is then used as input for creating the TyCtxt itself.
Felix S Klock II at 2016-07-13 12:21:22
@eddyb (plus I'm not sure its actually what we want ... part of the overall idea has been to have a relatively simple syntactic rule for deriving the temporary r-value lifetimes. The idea I wrote above is ... somewhat in conflict with that goal.)
Felix S Klock II at 2016-07-13 12:34:18
@pnkfelix @eddyb I think that the extension would be subsumed by the proposal in rust-lang/rfcs#66 -- however, the need to refactor (and compute the temporary lifetimes during typeck) is the main reason that this RFC has not been implemented.
Niko Matsakis at 2016-08-03 14:04:19
@pnkfelix Argh. I just revisited this for some reason. Those temporary rules were designed before deref and [] were overloadable; annoying that they still apply now. I would indeed like to revisit them, but not sure if we can.
Niko Matsakis at 2016-11-04 17:30:48
I am going to unassign @pnkfelix since I don't think they're actively investigating.
Mark Rousskov at 2017-05-15 21:20:26

Triage: rustc: 1.32.0 / rust version 1.34.0-nightly (c1c3c4e95 2019-01-29)

this compiles and does not error on 2018, but if we try to print _desugar2, it errors with

error[E0716]: temporary value dropped while borrowed
 --> src/main.rs:6:23
  |
6 |     let _desugar2 = &*"a".to_owned().index(..);
  |                       ^^^^^^^^^^^^^^          - temporary value is freed at the end of this statement
  |                       |
  |                       creates a temporary which is freed while still in use
7 |     println!("{:?}", _desugar2);
  |                      --------- borrow later used here
  |
  = note: consider using a `let` binding to create a longer lived value

error: aborting due to previous error

</details>

2015 errors without trying to print:

error[E0597]: borrowed value does not live long enough
 --> src/main.rs:5:21
  |
5 |     let _desugar1 = "a".to_owned().index(..);
  |                     ^^^^^^^^^^^^^^          - temporary value dropped here while still borrowed
  |                     |
  |                     temporary value does not live long enough
6 |     let _desugar2 = &*"a".to_owned().index(..);
7 | }
  | - temporary value needs to live until here
  |
  = note: consider using a `let` binding to increase its lifetime

error[E0597]: borrowed value does not live long enough
 --> src/main.rs:6:23
  |
6 |     let _desugar2 = &*"a".to_owned().index(..);
  |                       ^^^^^^^^^^^^^^          - temporary value dropped here while still borrowed
  |                       |
  |                       temporary value does not live long enough
7 | }
  | - temporary value needs to live until here
  |
  = note: consider using a `let` binding to increase its lifetime

error: aborting due to 2 previous errors

</details>

memoryruins at 2019-01-30 21:44:21

Triage: no change

Latest example code is:

use std::ops::Index;

fn main() {
    let _sugar = &"a".to_owned()[..];
    let _desugar1 = "a".to_owned().index(..);
    let _desugar2 = &*"a".to_owned().index(..);
    println!("{:?}", _desugar2);
}

Maayan Hanin at 2022-03-22 08:15:35