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

Double-indirection with Cow causes inconsistent behavior of the borrow checker #43058

Closed
darkwisebear opened this issue Jul 4, 2017 · 8 comments
Assignees
Labels
A-borrow-checker Area: The borrow checker A-NLL Area: Non-lexical lifetimes (NLL) A-typesystem Area: The type system C-bug Category: This is a bug. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. P-low Low priority

Comments

@darkwisebear
Copy link

darkwisebear commented Jul 4, 2017

UPDATE: This needs a test! Mentoring instructions can be found here.


While working on some code using std::borrow::Cow I encountered some strange behavior. Here is a simplified version of the code I was using:

use std::borrow::Cow;

#[derive(Clone, Debug)]
struct S<'a> {
    name: Cow<'a, str>
}

#[derive(Clone, Debug)]
struct T<'a> {
    s: Cow<'a, [S<'a>]>
}

fn main() {
    let s1 = [S { name: Cow::Borrowed("Test1") }, S { name: Cow::Borrowed("Test2") }];
    let b1 = T { s: Cow::Borrowed(&s1) };
    let s2 = [S { name: Cow::Borrowed("Test3") }, S { name: Cow::Borrowed("Test4") }];
    let b2 = T { s: Cow::Borrowed(&s2) };
    
    let mut v = Vec::new();
    v.push(b1);
    v.push(b2);
    
    println!("{:?}", v);
}

This fails with

error[E0597]: s2 does not live long enough

Removing b1,s1 or b2,s2 from the code fixes the error. However, since the very same pattern isused for both variable pairs it seems to be inconsistent behavior of the borrow checker.

Meta

Versions checked:
rustc 1.18.0 (03fc9d6 2017-06-06)
binary: rustc
commit-hash: 03fc9d6
commit-date: 2017-06-06
host: x86_64-pc-windows-gnu
release: 1.18.0
LLVM version: 3.9

rustc 1.19.0-beta.2 (a175ee5 2017-06-15)
rustc 1.20.0-nightly (734c836 2017-07-03)

@kennytm
Copy link
Member

kennytm commented Jul 5, 2017

The problem is that the lifetime of s1 is strictly longer than s2. (I think this is because Cow::Owned(<B as ToOwned>::Owned) somehow causes B i.e. [S<'a>] to be invariant.) Not sure if non-lexical lifetime can fix this.

This could be fixed by moving the let s2 = line above let b1 =.

@Mark-Simulacrum Mark-Simulacrum added A-borrow-checker Area: The borrow checker A-typesystem Area: The type system labels Jul 19, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 28, 2017
@arielb1 arielb1 added this to the NLL prototype milestone Nov 26, 2017
@arielb1
Copy link
Contributor

arielb1 commented Nov 26, 2017

@kennytm

This is because Cow is invariant in its lifetime.

NLL should fix this. I tagged this as NLL-prototype so we can have a test and demonstration that this is indeed fixed.

However, compiling this with NLL currently causes an ICE, which I think should be fixed before the prototype:

thread 'rustc' panicked at 'region constraints already solved', /tmp/tmp.gJ7nOH7fpQ/rust/src/libcore/option.rs:874:4
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/sys_common/backtrace.rs:68
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/sys_common/backtrace.rs:57
   2: std::panicking::default_hook::{{closure}}
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/panicking.rs:381
   3: std::panicking::default_hook
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/panicking.rs:391
   4: std::panicking::rust_panic_with_hook
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/panicking.rs:577
   5: std::panicking::begin_panic
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/panicking.rs:538
   6: std::panicking::begin_panic_fmt
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/panicking.rs:522
   7: rust_begin_unwind
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/panicking.rs:498
   8: core::panicking::panic_fmt
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libcore/panicking.rs:71
   9: core::option::expect_failed
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libcore/option.rs:874
  10: rustc::infer::InferCtxt::start_snapshot
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libcore/option.rs:302
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/infer/mod.rs:1537
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libcore/cell.rs:1090
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/infer/mod.rs:1535
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/infer/mod.rs:790
  11: rustc::infer::InferCtxt::probe
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/infer/mod.rs:904
  12: rustc::traits::project::opt_normalize_projection_type
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/traits/project.rs:1002
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/traits/project.rs:807
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/traits/project.rs:541
  13: rustc::traits::project::normalize_projection_type
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/traits/project.rs:398
  14: <rustc::traits::project::AssociatedTypeNormalizer<'a, 'b, 'gcx, 'tcx> as rustc::ty::fold::TypeFolder<'gcx, 'tcx>>::fold_ty
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/traits/project.rs:318
  15: rustc::ty::structural_impls::<impl rustc::ty::fold::TypeFoldable<'tcx> for &'tcx rustc::ty::TyS<'tcx>>::fold_with
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/structural_impls.rs:695
  16: rustc::traits::project::normalize
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/traits/project.rs:266
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/traits/project.rs:225
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/traits/project.rs:209
  17: rustc::traits::fully_normalize
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/traits/mod.rs:605
  18: rustc_mir::transform::nll::constraint_generation::ConstraintGeneration::add_drop_live_constraint
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_mir/transform/nll/constraint_generation.rs:158
  19: rustc_mir::transform::nll::constraint_generation::ConstraintGeneration::add_liveness_constraints::{{closure}}
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_mir/transform/nll/constraint_generation.rs:81
  20: rustc_mir::util::liveness::LivenessResult::simulate_block
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_mir/util/liveness.rs:151
  21: rustc_mir::transform::nll::constraint_generation::generate_constraints
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_mir/transform/nll/constraint_generation.rs:76
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_mir/transform/nll/constraint_generation.rs:54
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_mir/transform/nll/constraint_generation.rs:35
  22: rustc_mir::transform::nll::compute_regions
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_mir/transform/nll/mod.rs:78
  23: rustc_mir::borrow_check::do_mir_borrowck
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_mir/borrow_check.rs:116
  24: <std::thread::local::LocalKey<T>>::with
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_mir/borrow_check.rs:65
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/infer/mod.rs:439
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/context.rs:1453
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/thread/local.rs:377
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/thread/local.rs:288
  25: rustc::ty::context::tls::enter
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/context.rs:1450
  26: rustc::ty::context::GlobalCtxt::enter_local
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/context.rs:1261
  27: rustc::infer::InferCtxtBuilder::enter
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/infer/mod.rs:439
  28: rustc_mir::borrow_check::mir_borrowck
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_mir/borrow_check.rs:63
  29: rustc::ty::maps::<impl rustc::ty::maps::queries::mir_borrowck<'tcx>>::compute_result   
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/maps/plumbing.rs:382
  30: rustc::dep_graph::graph::DepGraph::with_task_impl
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/dep_graph/graph.rs:273
  31: rustc::dep_graph::graph::DepGraph::with_task
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/dep_graph/graph.rs:189
  32: rustc_errors::Handler::track_diagnostics
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/maps/plumbing.rs:472
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_errors/lib.rs:564
  33: rustc::ty::maps::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::cycle_check
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/maps/plumbing.rs:465
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/maps/plumbing.rs:120
  34: rustc::ty::maps::<impl rustc::ty::maps::queries::mir_borrowck<'tcx>>::force
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/maps/plumbing.rs:464
  35: rustc::ty::maps::<impl rustc::ty::maps::queries::mir_borrowck<'tcx>>::try_get
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/maps/plumbing.rs:300
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/maps/plumbing.rs:506
  36: rustc::ty::maps::TyCtxtAt::mir_borrowck
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/maps/plumbing.rs:545
  37: rustc::ty::maps::<impl rustc::ty::context::TyCtxt<'a, 'tcx, 'lcx>>::mir_borrowck
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/maps/plumbing.rs:538
  38: rustc_driver::driver::phase_3_run_analysis_passes::{{closure}}::{{closure}}
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_driver/driver.rs:1072
  39: rustc::util::common::time
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/util/common.rs:120
  40: <std::thread::local::LocalKey<T>>::with
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc_driver/driver.rs:1070
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/librustc/ty/context.rs:1453
             at /tmp/tmp.gJ7nOH7fpQ/rust/src/libstd/thread/local.rs:377

@nikomatsakis
Copy link
Contributor

Compiling with nll-master currently gives this:

lunch-box. rustc --stage1 ~/tmp/issue-43058.rs  -Znll -Zborrowck=mir
error[E0597]: borrowed value does not live long enough
  --> /home/nmatsakis/tmp/issue-43058.rs:24:2
   |
16 |     let s2 = [S { name: Cow::Borrowed("Test3") }, S { name: Cow::Borrowed("Test4") }];
   |         -- temporary value created here
...
24 | }
   |  ^ temporary value dropped here while still borrowed
   |
   = note: consider using a `let` binding to increase its lifetime

error: aborting due to previous error

@pnkfelix pnkfelix added the A-NLL Area: Non-lexical lifetimes (NLL) label Dec 21, 2017
@nikomatsakis nikomatsakis removed this from the NLL prototype milestone Jan 3, 2018
@nikomatsakis nikomatsakis added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Apr 3, 2018
@nikomatsakis
Copy link
Contributor

Update: This code is currently accepted:

https://play.rust-lang.org/?gist=0afae43679e86fef1dcedc90b4035ef3&version=nightly

@nikomatsakis nikomatsakis added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Apr 3, 2018
@nikomatsakis
Copy link
Contributor

Marking as E-needstest and E-mentor:

Since this code works now, what we need to do is add it to our test-suite. To do that, you would create a file in src/test/ui/nll named issue-43058.rs containing the example I linked to here. There are general instructions for adding new tests here.

lloydmeta added a commit to lloydmeta/rust that referenced this issue Apr 4, 2018
kennytm added a commit to kennytm/rust that referenced this issue Apr 4, 2018
…matsakis

Add a test for the fix to issue rust-lang#43058

Followed the instructions laid out here rust-lang#43058 (comment)
bors added a commit that referenced this issue Apr 4, 2018
Rollup of 25 pull requests

Successful merges:

 - #49179 (Handle future deprecation annotations )
 - #49512 (Add support for variant and types fields for intra links)
 - #49515 (fix targetted value background)
 - #49516 (Add missing anchor for union type fields)
 - #49532 (Add test for rustdoc ignore test)
 - #49533 (Add #[must_use] to a few standard library methods)
 - #49540 (Fix miri Discriminant() for non-ADT)
 - #49559 (Introduce Vec::resize_with method (see #41758))
 - #49570 (avoid IdxSets containing garbage above the universe length)
 - #49577 (Stabilize String::replace_range)
 - #49599 (Fix typo)
 - #49603 (Fix url for intra link provided method)
 - #49607 (Stabilize iterator methods in 1.27)
 - #49609 (run-pass/attr-stmt-expr: expand test cases)
 - #49612 (Fix "since" version for getpid feature.)
 - #49618 (Fix build error when compiling libcore for 16bit targets)
 - #49619 (tweak core::fmt docs)
 - #49637 (Stabilize parent_id())
 - #49639 (Update Cargo)
 - #49628 (Re-write the documentation index)
 - #49594 (Add some performance guidance to std::fs and std::io docs)
 - #49625 (miri: add public alloc_kind accessor)
 - #49634 (Add a test for the fix to issue #43058)
 - #49641 (Regression test for #46314)
 - #49547 (Unignore borrowck test)

Failed merges:
@pnkfelix
Copy link
Member

Re-triaging for #56754. NLL-fixed-by-NLL, so leaving open. Assigning to self for the main remaining task of adding the test. But also tagging as P-low since I don't think that is a high priority task.

@pnkfelix pnkfelix added P-low Low priority NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. and removed NLL-deferred labels Dec 19, 2018
@pnkfelix pnkfelix self-assigned this Dec 19, 2018
@pnkfelix
Copy link
Member

Ah, the test was added in PR #49634

@pnkfelix
Copy link
Member

NLL (migrate mode) is enabled in all editions as of PR #59114. Verified that test case compiles in Nightly 2015 edition; closing as fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-NLL Area: Non-lexical lifetimes (NLL) A-typesystem Area: The type system C-bug Category: This is a bug. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. P-low Low priority
Projects
None yet
Development

No branches or pull requests

6 participants