Ensure the types of methods are well-formed #28669

arielb1 · 2015-09-26T00:03:55Z

Before calling a fn, we check that its argument and return types are WF.

The previous code only checked the trait-ref, which was not enough
in several cases.

As this is a soundness fix, it is a [breaking-change]. Some new annotations are needed, which I think are because of #18653 and the imperfection of projection_must_outlive (that can probably be worked around by moving the wf obligation later).

Fixes #28609

r? @nikomatsakis

bors · 2015-09-26T14:55:39Z

☔ The latest upstream changes (presumably #28629) made this pull request unmergeable. Please resolve the merge conflicts.

arielb1 · 2015-09-26T16:10:54Z

Minimal example for the rustc regression:

pub trait Lift<'tcx> {
    type Lifted /* adding :'tcx solves this */;
}

pub fn lift<'tcx,T:?Sized+Lift<'tcx>>(value: &T) -> T::Lifted {
    loop {}
}

pub fn my_lift_to_tcx<'tcx, T: Lift<'tcx>>(this: &T)
{   
    let mut result = Vec::new(); // adding a type annotation here solves this
    result.push(lift(this));
}

arielb1 · 2015-09-26T16:14:45Z

The warning is

elevator.rs:12:12: 12:28 warning: the parameter type `T` may not live long enoug
h [E0309]
elevator.rs:12     result.push(lift(this));
                          ^~~~~~~~~~~~~~~~
elevator.rs:12:12: 12:28 help: run `rustc --explain E0309` to see a detailed explanation
elevator.rs:12:12: 12:28 help: consider adding an explicit lifetime bound `T: 'tcx`...
elevator.rs:12:12: 12:28 note: this warning results from recent bug fixes and clarifications; it will become a HARD ERROR in the next release. See RFC 1214 for details.
elevator.rs:12     result.push(lift(this));
                          ^~~~~~~~~~~~~~~~
elevator.rs:12:12: 12:28 note: ...so that the reference type `&collections::vec::Vec<<T as Lift<'_>>::Lifted>` does not outlive the data it points at
elevator.rs:12     result.push(lift(this));
                          ^~~~~~~~~~~~~~~~

I think the reference lifetime somehow becomes underconstrained.

nikomatsakis · 2015-09-29T17:58:28Z

src/test/compile-fail/wf-static-method.rs

+
+// check that static methods don't get to assume `Self` is well-formed
+
+// TODO: I (arielb1) isn't sure whether this is a bug or feature.


Note tidy error: /home/travis/build/rust-lang/rust/src/test/compile-fail/wf-static-method.rs:13: TODO is deprecated; use FIXME

The question was meant to you - I should have asked it more explicitly.

brson · 2015-09-29T20:47:05Z

Running on crater

brson · 2015-09-30T19:07:33Z

Toolchains built, testing crates.

brson · 2015-09-30T21:48:40Z

Crater says.

arielb1 · 2015-10-01T13:28:15Z

regression summary

rust_ini uses the soundness hole to get a working find_equiv - zonyitoo/rust-ini#19.
acacia is just plain RFC1214-noncompliant (milibopp/acacia#71), and for some reason cargo attempts to compile both nalgebra 0.2.3 and nalgebra 0.3.0, which fails. There are no new errors.

I think these are both acceptable

brson · 2015-10-02T18:15:24Z

sgtm

Let's at least give the authors of these two crates a head's up. cc @zonyitoo @aepsil0n this soundness fix will break some of your stuff.

zonyitoo · 2015-10-02T19:04:35Z

@brson Thanks! I will update the rust-ini as soon as possible.

nikomatsakis · 2015-10-02T19:19:59Z

src/test/compile-fail/wf-static-method.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// check that static methods don't get to assume `Self` is well-formed


So I've been meaning to get back to you on this test. I'm not quite sure what this is testing... it seems like the Self type, if WF, would imply that 'b: 'a, but the various tests are erroring because 'a: 'b doesn't hold. Is this testing what you think it is?

Also, within an impl block, part of RFC 1214 was that we DO get to assume that the Self (and other input types) to the trait are well-formed, which is why we check that the trait ref is WF when we project. This was used to reduce the overhead on associated type values, but I imagine it should apply to static fns as well.

I got the order wrong. I also added a test on the caller side.

By RFC1214: Before calling a fn, we check that its argument and return types are WF. This check takes place after all higher-ranked lifetimes have been instantiated. Checking the argument types ensures that the implied bounds due to argument types are correct. Checking the return type ensures that the resulting type of the call is WF. The previous code only checked the trait-ref, which was not enough in several cases. As this is a soundness fix, it is a [breaking-change]. Fixes rust-lang#28609

looks like some mix of rust-lang#18653 and `projection_must_outlive`, but that needs to be investigated further (crater run?)

also, ensure that callers are checked.

milibopp · 2015-10-03T11:34:48Z

@arielb1 already gave me a heads up. Will fix as soon as I find the time.

nikomatsakis · 2015-10-03T20:24:57Z

@bors r+

bors · 2015-10-03T20:24:57Z

📌 Commit 2f23e17 has been approved by nikomatsakis

@nikomatsakis

By RFC1214: > Before calling a fn, we check that its argument and return types are WF. The previous code only checked the trait-ref, which was not enough in several cases. As this is a soundness fix, it is a [breaking-change]. Some new annotations are needed, which I think are because of #18653 and the imperfection of `projection_must_outlive` (that can probably be worked around by moving the wf obligation later). Fixes #28609 r? @nikomatsakis

bors · 2015-10-03T22:07:38Z

⌛ Testing commit 2f23e17 with merge 130851e...

bors · 2015-10-03T23:57:14Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

eefriedman · 2015-10-04T00:31:25Z

@arielb1 Is there an issue tracking the pinyin regression (https://tools.taskcluster.net/task-inspector/#UFk2m1UJRweSxQNABRB84w/0)? It appears to be real (although I'm not sure it's relevant).

eefriedman · 2015-10-05T22:37:23Z

Filed #28853 to track the pinyin regression.

@pnkfelix

…felix This rather crucial requirement was not checked. In most cases, that didn't cause any trouble because the argument types are required to outlive the call and are subtypes of a subformula of the callee type. However, binary ops are taken by ref only indirectly, without it being marked in the argument types, which led to the argument types not being constrained anywhere causing spurious errors (as these are basically unconstrainable, I don't think this change can break code). Of course, the old way was also incorrent with contravariance, but that is still unsound for other reasons. This also improves rustc::front to get RUST_LOG to *somewhat* work. Fixes rust-lang#28999. That issue is one of the several regression introduced by rust-lang#28669. r? @pnkfelix

rust-highfive assigned nikomatsakis Sep 26, 2015

arielb1 force-pushed the well-formed-methods branch from 8352807 to 98fe9eb Compare September 26, 2015 18:13

nikomatsakis reviewed Sep 29, 2015
View reviewed changes

arielb1 force-pushed the well-formed-methods branch from 98fe9eb to 1c3645e Compare September 29, 2015 20:14

nikomatsakis reviewed Oct 2, 2015
View reviewed changes

Ariel Ben-Yehuda added 2 commits October 2, 2015 23:40

fix fallout

ce70207

looks like some mix of rust-lang#18653 and `projection_must_outlive`, but that needs to be investigated further (crater run?)

arielb1 force-pushed the well-formed-methods branch 4 times, most recently from d6924fe to db22da0 Compare October 3, 2015 09:03

use the correct subtyping order in a test

2f23e17

also, ensure that callers are checked.

arielb1 force-pushed the well-formed-methods branch from db22da0 to 2f23e17 Compare October 3, 2015 09:04

bors merged commit 2f23e17 into rust-lang:master Oct 3, 2015

arielb1 mentioned this pull request Oct 12, 2015

require a method callee's type to outlive the call #29006

Merged

nikomatsakis mentioned this pull request Nov 25, 2015

#28669 caused regression compiling pinyin crate on nightly #28853

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure the types of methods are well-formed #28669

Ensure the types of methods are well-formed #28669

arielb1 commented Sep 26, 2015

bors commented Sep 26, 2015

arielb1 commented Sep 26, 2015

arielb1 commented Sep 26, 2015

nikomatsakis Sep 29, 2015

arielb1 Sep 29, 2015

brson commented Sep 29, 2015

brson commented Sep 30, 2015

brson commented Sep 30, 2015

arielb1 commented Oct 1, 2015

brson commented Oct 2, 2015

zonyitoo commented Oct 2, 2015

nikomatsakis Oct 2, 2015

arielb1 Oct 2, 2015

milibopp commented Oct 3, 2015

nikomatsakis commented Oct 3, 2015

bors commented Oct 3, 2015

bors commented Oct 3, 2015

bors commented Oct 3, 2015

eefriedman commented Oct 4, 2015

eefriedman commented Oct 5, 2015


		// check that static methods don't get to assume `Self` is well-formed

		// TODO: I (arielb1) isn't sure whether this is a bug or feature.

Ensure the types of methods are well-formed #28669

Ensure the types of methods are well-formed #28669

Conversation

arielb1 commented Sep 26, 2015

bors commented Sep 26, 2015

arielb1 commented Sep 26, 2015

arielb1 commented Sep 26, 2015

nikomatsakis Sep 29, 2015

Choose a reason for hiding this comment

arielb1 Sep 29, 2015

Choose a reason for hiding this comment

brson commented Sep 29, 2015

brson commented Sep 30, 2015

brson commented Sep 30, 2015

arielb1 commented Oct 1, 2015

regression summary

brson commented Oct 2, 2015

zonyitoo commented Oct 2, 2015

nikomatsakis Oct 2, 2015

Choose a reason for hiding this comment

arielb1 Oct 2, 2015

Choose a reason for hiding this comment

milibopp commented Oct 3, 2015

nikomatsakis commented Oct 3, 2015

bors commented Oct 3, 2015

bors commented Oct 3, 2015

bors commented Oct 3, 2015

eefriedman commented Oct 4, 2015

eefriedman commented Oct 5, 2015