Fix NLL issue 50716 and add a regression test.

[vakaras]

Fix for NLL issue #50716.

r? @nikomatsakis

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[pnkfelix]

I think the test needs to either be moved to the ui/ directory (and a corresponding .stderr file is then also added to the PR), or it needs an explicit #![feature(nll)]. As it stands right now I don’t think it’s actually testing the code you’ve added.

[vakaras]

Good catch @pnkfelix! I have updated the test.

[nikomatsakis]

Sorry for ping-ponging!

[nikomatsakis]

the compile-fail test directory is basically deprecated... I'd prefer this to be in src/test/ui/nll/ =)

[pnkfelix]

(I took the liberty of moving the test in the fashion that @nikomatsakis requested, and plan to r+ it if travis accepts it.)

I made the change niko requested.

[pnkfelix]

(I could have also removed the #![feature(nll)] attribute from the test and perhaps added an additional .nll.stderr file to account for differences in output. But for right now I sort of just want to get this landed and move along.)

[vakaras]

@pnkfelix Thanks!

[nikomatsakis]

@bors r+

[bors]

📌 Commit 415a10f has been approved by nikomatsakis

[bors]

🔒 Merge conflict

[nikomatsakis]

@bors r+

[bors]

📌 Commit 61f55d2 has been approved by nikomatsakis

[bors]

⌛ Testing commit 61f55d2d61cd9ca6e6965e3d4108d696d5afc785 with merge 89ff7f5155b59426b0428d4800bb6e96fb0bb8b4...

[bors]

💔 Test failed - status-appveyor

[vakaras]

@pnkfelix @nikomatsakis The error we got on bors looks very similar to the one I was getting during the Rust Fest:

error: internal compiler error: broken MIR in NodeId(3244) (""): errors selecting obligation: [FulfillmentError(Obligation(predicate=Binder(TraitPredicate(<Q as std::marker::Sized>)),depth=1),Unimplemented)]
  --> C:\Users\appveyor\.cargo\registry\src\git.luolix.top-1ecc6299db9ec823\linked-hash-map-0.3.0\src\lib.rs:86:1
   |
86 | struct Qey<Q: ?Sized>(Q);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error

Do you have any idea how to investigate it? ./x.py build and ./x.py test works on my machine without any issues.

[nikomatsakis]

@michaelwoerister do we use incremental on bors ? presumably not...

[nikomatsakis]

(No reason to blame incremental per se)

[nikomatsakis]

@vakaras I don't know but it does seem like it could be related to this PR — after all, you added an obligation to prove Sized, and it is failing without proving that.

[TimNN]

@vakaras: Do you think you'll be able to investigate this issue? It looks like compiling linked-hash-map@0.3.0 causes the issue (This happens in a cargotest, which isn't run by ./x.py test by default, I believe. Can you reproduce the issue if you build a local compiler and then build the crate manually?

[vakaras]

@TimNN I am not sure I understand well enough what is happening here to investigate this issue properly. However, I will try at least to reproduce the error on my machine.

[vakaras]

@TimNN Thanks! You were right, the following steps reproduce the issue:

git clone https://github.com/vakaras/rust.git -b issue-50716 issue-50716
cd issue-50716/
./x.py build
./x.py test src/tools/cargotest src/tools/cargo

I will try to investigate this issue.

[kennytm]

@bors r-

Wrongly requeued

[vakaras]

@nikomatsakis The minimal not working example (taken from linked-hash-map):

struct Qey<Q: ?Sized>(Q);

Do you have any idea what could be wrong here?

Full repro steps:

git clone https://github.com/vakaras/rust.git -b issue-50716 issue-50716
cd issue-50716/
./x.py build
./x.py test src/tools/cargotest src/tools/cargo
rustup toolchain link issue-50716 $PWD/build/x86_64-unknown-linux-gnu/stage2/
echo 'struct Qey<Q: ?Sized>(Q);' > code.rs
rustc +issue-50716 --crate-type lib code.rs
warning: struct is never used: `Qey`
 --> code.rs:1:1
  |
1 | struct Qey<Q: ?Sized>(Q);
  | ^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(dead_code)] on by default

error: internal compiler error: broken MIR in DefId(0/1:9 ~ code[8787]::Qey[0]::{{constructor}}[0]) (""): errors selecting obligation: [FulfillmentError(Obligation(predicate=Binder(TraitPredicate(<Q as std::marker::Sized>)),depth=1),Unimplemented)]
 --> code.rs:1:1
  |
1 | struct Qey<Q: ?Sized>(Q);
  | ^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

[nikomatsakis]

@vakaras I guess that the problem comes from the "auto-generated" MIR for the constructor of Qey, which looks like this:

fn Qey(_1: Q) -> Qey<Q>{
    let mut _0: Qey<Q>;                  // return place

    bb0: {                              
        (_0.0: Q) = move _1;             // bb0[0]: scope 0 at src/main.rs:1:1: 1:26
        return;                          // bb0[1]: scope 0 at src/main.rs:1:1: 1:26
    }
}

You can see that this does _0.0 = ... which -- indeed -- is not entirely valid, since we do not know that Q: Sized. (In fact, we do know this, because we would not allow a call Q(bar) unless bar were sized, but the current MIR doesn't represent that anyhow.)

I can think of two fixes.

One might be to skip borrowck for such generated code -- in fact, we should probably do that just for performance reasons.

The other would be to modify the generated code to carry an extended environment. While more correct, I'm not inclined to do that just because it's complicated.

To skip this we would probably want to modify do_mir_borrowck. We already have a "shortcircuit check" that just looks to see if MIR borrow check is enabled:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 79 to 84 in 2a1c4ee

	if !tcx.has_attr(def_id, "rustc_mir_borrowck") && !tcx.use_mir_borrowck() {
	return BorrowCheckResult {
	closure_requirements: None,
	used_mut_upvars: SmallVec::new(),
	};
	}

Maybe we should add some code that checks if the variable def_id (which uniquely identifies the code to be type-checked) comes from a "struct constructor" (which is what we call the auto-geneated tuple struct constructors). To do that, I would suggest adding a fn to TyCtxt similar to the existing is_closure:

rust/src/librustc/ty/util.rs

Lines 521 to 523 in 2a1c4ee

	pub fn is_closure(self, def_id: DefId) -> bool {
	self.def_key(def_id).disambiguated_data.data == DefPathData::ClosureExpr
	}

This might look like:

/// True if this def-id refers to the implicit constructor for a tuple struct like `struct Foo(u32)`
pub fn is_struct_constructor(self, def_id: DefId) -> bool {
        self.def_key(def_id).disambiguated_data.data == DefPathData::StructCtor
}

(What this is doing is looking up, via def_key, some information about what the def-id refers to.)

Then we can modify MIR borrowck to return early if tcx.is_struct_constructor(def_id) is true (with an explanatory comment citing this example!)

Also, let's add this as a test case =)

[vakaras]

@nikomatsakis I have tried to implement your suggestions here. However, it does not seem to fix the issue and for some reason affects the reported error spans in two tests.

[vakaras]

I think I finally understood why it still crashes. The fix in the pull request fixes only when NLL is used. However, when the old borrow checker is used, the MIR is still type checked by this code:

rust/src/librustc_mir/borrow_check/nll/type_check/mod.rs

Lines 1708 to 1740 in c208243

	fn run_pass<'a, 'tcx>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, src: MirSource, mir: &mut Mir<'tcx>) {
	let def_id = src.def_id;
	debug!("run_pass: {:?}", def_id);
	// When NLL is enabled, the borrow checker runs the typeck
	// itself, so we don't need this MIR pass anymore.
	if tcx.use_mir_borrowck() {
	return;
	}
	if tcx.sess.err_count() > 0 {
	// compiling a broken program can obviously result in a
	// broken MIR, so try not to report duplicate errors.
	return;
	}
	let param_env = tcx.param_env(def_id);
	tcx.infer_ctxt().enter(|infcx| {
	let _ = type_check_internal(
	&infcx,
	def_id,
	param_env,
	mir,
	&[],
	None,
	None,
	&mut |_| (),
	);
	// For verification purposes, we just ignore the resulting
	// region constraint sets. Not our problem. =)
	});
	}
	}

I will try to implement the same early return for this function too.

[nikomatsakis]

Nice comment =)

[nikomatsakis]

@bors r+

[bors]

📌 Commit 03ecd98 has been approved by nikomatsakis

[bors]

⌛ Testing commit 03ecd98 with merge cd494c1...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing cd494c1 to master...